| Message ID | 20230407033341.5139-1-haibo.li@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ARM:unwind:fix unwind abort for uleb128 case | expand |
On Fri, Apr 7, 2023 at 5:33 AM Haibo Li <haibo.li@mediatek.com> wrote: > When unwind instruction is 0xb2,the subsequent instructions > are uleb128 bytes. > For now,it uses only the first uleb128 byte in code. > > For vsp increments of 0x204~0x400,use one uleb128 byte like below: > 0xc06a00e4 <unwind_test_work>: 0x80b27fac > Compact model index: 0 > 0xb2 0x7f vsp = vsp + 1024 > 0xac pop {r4, r5, r6, r7, r8, r14} > > For vsp increments larger than 0x400,use two uleb128 bytes like below: > 0xc06a00e4 <unwind_test_work>: @0xc0cc9e0c > Compact model index: 1 > 0xb2 0x81 0x01 vsp = vsp + 1032 > 0xac pop {r4, r5, r6, r7, r8, r14} > The unwind works well since the decoded uleb128 byte is also 0x81. > > For vsp increments larger than 0x600,use two uleb128 bytes like below: > 0xc06a00e4 <unwind_test_work>: @0xc0cc9e0c > Compact model index: 1 > 0xb2 0x81 0x02 vsp = vsp + 1544 > 0xac pop {r4, r5, r6, r7, r8, r14} > In this case,the decoded uleb128 result is 0x101(vsp=0x204+(0x101<<2)). > While the uleb128 used in code is 0x81(vsp=0x204+(0x81<<2)). > The unwind aborts at this frame since it gets incorrect vsp. > > To fix this,add uleb128 decode to cover all the above case. > > Signed-off-by: Haibo Li <haibo.li@mediatek.com> [Added people such as Catalin, Ard and Anurag who wrote the lion's share of actual algorithms in this file] I would just link the wikipedia in the patch commit log actually: Link: https://en.wikipedia.org/wiki/LEB128 for poor souls like me who need a primer on this encoding. It's great if you also have a reference to the spec where you found this, but I take your word for that this appears in code. Did compilers always emit this? Then we should have a Cc stable to this patch. Unfortunately the link in the top of the file is dead. > +static unsigned long unwind_decode_uleb128(struct unwind_ctrl_block *ctrl) So this decodes max an unsigned long? Are we sure that will always suffice? > +{ > + unsigned long result = 0; > + unsigned long insn; > + unsigned long bytes = 0; > + > + do { > + insn = unwind_get_byte(ctrl); > + result |= (insn & 0x7f) << (bytes * 7); > + bytes++; > + if (bytes == sizeof(result)) > + break; > + } while (!!(insn & 0x80)); I suppose the documentation is in the commit message, but something terse and nice that make us understand this code would be needed here as well. Could you fold in a comment of how the do {} while-loop works and th expected outcome? Something like: "unwind_get_byte() will advance ctrl one instruction at a time, we loop until we get an instruction byte where bit 7 is not set." Is there a risk that this will loop forever or way too long if it happens to point at some corrupted memory containing say 0xff 0xff 0xff ...? Since we're decoding a 32 bit unsigned long maybe break the loop after max 5 bytes (35 bits)? Or are we sure this will not happen? > @@ -361,7 +376,7 @@ static int unwind_exec_insn(struct unwind_ctrl_block *ctrl) > if (ret) > goto error; > } else if (insn == 0xb2) { > - unsigned long uleb128 = unwind_get_byte(ctrl); > + unsigned long uleb128 = unwind_decode_uleb128(ctrl); Is unsigned long always enough? We are sure? Yours, Linus Walleij
> On Fri, Apr 7, 2023 at 5:33 AM Haibo Li <haibo.li@mediatek.com> wrote: > > > When unwind instruction is 0xb2,the subsequent instructions > > are uleb128 bytes. > > For now,it uses only the first uleb128 byte in code. > > > > For vsp increments of 0x204~0x400,use one uleb128 byte like below: > > 0xc06a00e4 <unwind_test_work>: 0x80b27fac > > Compact model index: 0 > > 0xb2 0x7f vsp = vsp + 1024 > > 0xac pop {r4, r5, r6, r7, r8, r14} > > > > For vsp increments larger than 0x400,use two uleb128 bytes like below: > > 0xc06a00e4 <unwind_test_work>: @0xc0cc9e0c > > Compact model index: 1 > > 0xb2 0x81 0x01 vsp = vsp + 1032 > > 0xac pop {r4, r5, r6, r7, r8, r14} > > The unwind works well since the decoded uleb128 byte is also 0x81. > > > > For vsp increments larger than 0x600,use two uleb128 bytes like below: > > 0xc06a00e4 <unwind_test_work>: @0xc0cc9e0c > > Compact model index: 1 > > 0xb2 0x81 0x02 vsp = vsp + 1544 > > 0xac pop {r4, r5, r6, r7, r8, r14} > > In this case,the decoded uleb128 result is 0x101(vsp=0x204+(0x101<<2)). > > While the uleb128 used in code is 0x81(vsp=0x204+(0x81<<2)). > > The unwind aborts at this frame since it gets incorrect vsp. > > > > To fix this,add uleb128 decode to cover all the above case. > > > > Signed-off-by: Haibo Li <haibo.li@mediatek.com> > > [Added people such as Catalin, Ard and Anurag who wrote the lion's > share of actual algorithms in this file] > > I would just link the wikipedia in the patch commit log actually: > > Link:https://en.wikipedia.org/wiki/LEB128 > > for poor souls like me who need a primer on this encoding. > > It's great if you also have a reference to the spec where you > found this, but I take your word for that this appears in code. > Did compilers always emit this? Then we should have a Cc stable > to this patch. Unfortunately the link in the top of the file is dead. Yes.I also study uleb128 enc/dec format from this link. In experiment,Both Clang and GCC produces unwind instructions using ULEB128 > > > +static unsigned long unwind_decode_uleb128(struct unwind_ctrl_block > *ctrl) > > So this decodes max an unsigned long? Are we sure that will always > suffice? For now,the maximum thread size of arm is 16KB(KASAN on). From below experiment(worse case while impossible),two uleb128 bytes is sufficent for 16KB stack. 0xc06a00e4 <unwind_test_work>: @0xc0cc9e0c Compact model index: 1 0xb2 0xff 0x1e vsp = vsp + 16384 0xac pop {r4, r5, r6, r7, r8, r14} From below experiment,the code picks maximum 4 uleb128 encoded bytes, correspoding to vsp increments of 1073742336,the unwind_decode_uleb128 returns 0xFFFFFFF. So unsigned long is suffice. 0xc06a00e4 <unwind_test_work>: @0xc0cc9e0c Compact model index: 1 0xb2 0xff 0xff 0xff 0x7f vsp = vsp + 1073742336 0xac pop {r4, r5, r6, r7, r8, r14} > > > +{ > > + unsigned long result = 0; > > + unsigned long insn; > > + unsigned long bytes = 0; > > + > > + do { > > + insn = unwind_get_byte(ctrl); > > + result |= (insn & 0x7f) << (bytes * 7); > > + bytes++; > > + if (bytes == sizeof(result)) > > + break; > > + } while (!!(insn & 0x80)); > > I suppose the documentation is in the commit message, but something terse > and nice that make us understand this code would be needed here as well. > Could you fold in a comment of how the do {} while-loop works and th expected > outcome? Something like: > > "unwind_get_byte() will advance ctrl one instruction at a time, we loop > until we get an instruction byte where bit 7 is not set." > I will add a comment in later patch. > Is there a risk that this will loop forever or way too long if it happens > to point at some corrupted memory containing say 0xff 0xff 0xff ...? > > Since we're decoding a 32 bit unsigned long maybe break the loop after max > 5 bytes (35 bits)? Or are we sure this will not happen? in case of some corrupted memory containing say 0xff 0xff 0xff ...,the loop breaks after max 4 bytes(decode as max 28 bits) > > > @@ -361,7 +376,7 @@ static int unwind_exec_insn(struct > unwind_ctrl_block *ctrl) > > if (ret) > > goto error; > > } else if (insn == 0xb2) { > > - unsigned long uleb128 = unwind_get_byte(ctrl); > > + unsigned long uleb128 = unwind_decode_uleb128(ctrl); > > Is unsigned long always enough? We are sure? For the patch,it can cover single frame up to 1073742336 Bytes.So it is enough. > > Yours, > Linus Walleij
On Wed, Apr 12, 2023 at 4:44 AM Haibo Li <haibo.li@mediatek.com> wrote: > > Since we're decoding a 32 bit unsigned long maybe break the loop after max > > 5 bytes (35 bits)? Or are we sure this will not happen? > in case of some corrupted memory containing say 0xff 0xff 0xff ...,the loop breaks after > max 4 bytes(decode as max 28 bits) You're obviously right, I must have been too tired to understand the ==sizeof() break; Thanks! Linus Walleij
On 07/04/2023 05:33, Haibo Li wrote: > When unwind instruction is 0xb2,the subsequent instructions > are uleb128 bytes. > For now,it uses only the first uleb128 byte in code. > > For vsp increments of 0x204~0x400,use one uleb128 byte like below: > 0xc06a00e4 <unwind_test_work>: 0x80b27fac > Compact model index: 0 > 0xb2 0x7f vsp = vsp + 1024 > 0xac pop {r4, r5, r6, r7, r8, r14} > > For vsp increments larger than 0x400,use two uleb128 bytes like below: > 0xc06a00e4 <unwind_test_work>: @0xc0cc9e0c > Compact model index: 1 > 0xb2 0x81 0x01 vsp = vsp + 1032 > 0xac pop {r4, r5, r6, r7, r8, r14} > The unwind works well since the decoded uleb128 byte is also 0x81. > > For vsp increments larger than 0x600,use two uleb128 bytes like below: > 0xc06a00e4 <unwind_test_work>: @0xc0cc9e0c > Compact model index: 1 > 0xb2 0x81 0x02 vsp = vsp + 1544 > 0xac pop {r4, r5, r6, r7, r8, r14} > In this case,the decoded uleb128 result is 0x101(vsp=0x204+(0x101<<2)). > While the uleb128 used in code is 0x81(vsp=0x204+(0x81<<2)). > The unwind aborts at this frame since it gets incorrect vsp. > > To fix this,add uleb128 decode to cover all the above case. > > Signed-off-by: Haibo Li <haibo.li@mediatek.com> > --- > arch/arm/kernel/unwind.c | 19 +++++++++++++++++-- > 1 file changed, 17 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/kernel/unwind.c b/arch/arm/kernel/unwind.c > index 53be7ea6181b..e5796a5acba1 100644 > --- a/arch/arm/kernel/unwind.c > +++ b/arch/arm/kernel/unwind.c > @@ -20,7 +20,6 @@ > #warning Change compiler or disable ARM_UNWIND option. > #endif > #endif /* __CHECKER__ */ > - Why delete this line ? > #include <linux/kernel.h> > #include <linux/init.h> > #include <linux/export.h> > @@ -308,6 +307,22 @@ static int unwind_exec_pop_subset_r0_to_r3(struct unwind_ctrl_block *ctrl, > return URC_OK; > } > > +static unsigned long unwind_decode_uleb128(struct unwind_ctrl_block *ctrl) > +{ > + unsigned long result = 0; > + unsigned long insn; > + unsigned long bytes = 0; Alphabetical order please. > + > + do { > + insn = unwind_get_byte(ctrl); > + result |= (insn & 0x7f) << (bytes * 7); > + bytes++; > + if (bytes == sizeof(result)) > + break; > + } while (!!(insn & 0x80)); > + > + return result; > +} Please add a blank line for readability. > /* > * Execute the current unwind instruction. > */ > @@ -361,7 +376,7 @@ static int unwind_exec_insn(struct unwind_ctrl_block *ctrl) > if (ret) > goto error; > } else if (insn == 0xb2) { > - unsigned long uleb128 = unwind_get_byte(ctrl); > + unsigned long uleb128 = unwind_decode_uleb128(ctrl); > > ctrl->vrs[SP] += 0x204 + (uleb128 << 2); > } else { Great job! I'm aligned with Linus Walleij's feedback about the need of few comments to explain the decode loop, even if your code is clear, light and robust.
> On 07/04/2023 05:33, Haibo Li wrote: > > When unwind instruction is 0xb2,the subsequent instructions are > > uleb128 bytes. > > For now,it uses only the first uleb128 byte in code. > > > > For vsp increments of 0x204~0x400,use one uleb128 byte like below: > > 0xc06a00e4 <unwind_test_work>: 0x80b27fac > > Compact model index: 0 > > 0xb2 0x7f vsp = vsp + 1024 > > 0xac pop {r4, r5, r6, r7, r8, r14} > > > > For vsp increments larger than 0x400,use two uleb128 bytes like below: > > 0xc06a00e4 <unwind_test_work>: @0xc0cc9e0c > > Compact model index: 1 > > 0xb2 0x81 0x01 vsp = vsp + 1032 > > 0xac pop {r4, r5, r6, r7, r8, r14} > > The unwind works well since the decoded uleb128 byte is also 0x81. > > > > For vsp increments larger than 0x600,use two uleb128 bytes like below: > > 0xc06a00e4 <unwind_test_work>: @0xc0cc9e0c > > Compact model index: 1 > > 0xb2 0x81 0x02 vsp = vsp + 1544 > > 0xac pop {r4, r5, r6, r7, r8, r14} > > In this case,the decoded uleb128 result is 0x101(vsp=0x204+(0x101<<2)). > > While the uleb128 used in code is 0x81(vsp=0x204+(0x81<<2)). > > The unwind aborts at this frame since it gets incorrect vsp. > > > > To fix this,add uleb128 decode to cover all the above case. > > > > Signed-off-by: Haibo Li <haibo.li@mediatek.com> > > --- > > arch/arm/kernel/unwind.c | 19 +++++++++++++++++-- > > 1 file changed, 17 insertions(+), 2 deletions(-) > > > > diff --git a/arch/arm/kernel/unwind.c b/arch/arm/kernel/unwind.c index > > 53be7ea6181b..e5796a5acba1 100644 > > --- a/arch/arm/kernel/unwind.c > > +++ b/arch/arm/kernel/unwind.c > > @@ -20,7 +20,6 @@ > > #warning Change compiler or disable ARM_UNWIND option. > > #endif > > #endif /* __CHECKER__ */ > > - > > Why delete this line ? It may be changed by mistake.I will restore it. > > > #include <linux/kernel.h> > > #include <linux/init.h> > > #include <linux/export.h> > > @@ -308,6 +307,22 @@ static int > unwind_exec_pop_subset_r0_to_r3(struct unwind_ctrl_block *ctrl, > > return URC_OK; > > } > > > > +static unsigned long unwind_decode_uleb128(struct unwind_ctrl_block > > +*ctrl) { > > + unsigned long result = 0; > > + unsigned long insn; > > + unsigned long bytes = 0; > > Alphabetical order please. get it. > > > + > > + do { > > + insn = unwind_get_byte(ctrl); > > + result |= (insn & 0x7f) << (bytes * 7); > > + bytes++; > > + if (bytes == sizeof(result)) > > + break; > > + } while (!!(insn & 0x80)); > > + > > + return result; > > +} > > Please add a blank line for readability. OK. > > > /* > > * Execute the current unwind instruction. > > */ > > @@ -361,7 +376,7 @@ static int unwind_exec_insn(struct > unwind_ctrl_block *ctrl) > > if (ret) > > goto error; > > } else if (insn == 0xb2) { > > - unsigned long uleb128 = unwind_get_byte(ctrl); > > + unsigned long uleb128 = unwind_decode_uleb128(ctrl); > > > > ctrl->vrs[SP] += 0x204 + (uleb128 << 2); > > } else { > > Great job! I'm aligned with Linus Walleij's feedback about the need of few > comments to explain the decode loop, even if your code is clear, light and > robust. Thanks for reviewing the patch.I will add the comment in later patch. >
diff --git a/arch/arm/kernel/unwind.c b/arch/arm/kernel/unwind.c index 53be7ea6181b..e5796a5acba1 100644 --- a/arch/arm/kernel/unwind.c +++ b/arch/arm/kernel/unwind.c @@ -20,7 +20,6 @@ #warning Change compiler or disable ARM_UNWIND option. #endif #endif /* __CHECKER__ */ - #include <linux/kernel.h> #include <linux/init.h> #include <linux/export.h> @@ -308,6 +307,22 @@ static int unwind_exec_pop_subset_r0_to_r3(struct unwind_ctrl_block *ctrl, return URC_OK; } +static unsigned long unwind_decode_uleb128(struct unwind_ctrl_block *ctrl) +{ + unsigned long result = 0; + unsigned long insn; + unsigned long bytes = 0; + + do { + insn = unwind_get_byte(ctrl); + result |= (insn & 0x7f) << (bytes * 7); + bytes++; + if (bytes == sizeof(result)) + break; + } while (!!(insn & 0x80)); + + return result; +} /* * Execute the current unwind instruction. */ @@ -361,7 +376,7 @@ static int unwind_exec_insn(struct unwind_ctrl_block *ctrl) if (ret) goto error; } else if (insn == 0xb2) { - unsigned long uleb128 = unwind_get_byte(ctrl); + unsigned long uleb128 = unwind_decode_uleb128(ctrl); ctrl->vrs[SP] += 0x204 + (uleb128 << 2); } else {
When unwind instruction is 0xb2,the subsequent instructions are uleb128 bytes. For now,it uses only the first uleb128 byte in code. For vsp increments of 0x204~0x400,use one uleb128 byte like below: 0xc06a00e4 <unwind_test_work>: 0x80b27fac Compact model index: 0 0xb2 0x7f vsp = vsp + 1024 0xac pop {r4, r5, r6, r7, r8, r14} For vsp increments larger than 0x400,use two uleb128 bytes like below: 0xc06a00e4 <unwind_test_work>: @0xc0cc9e0c Compact model index: 1 0xb2 0x81 0x01 vsp = vsp + 1032 0xac pop {r4, r5, r6, r7, r8, r14} The unwind works well since the decoded uleb128 byte is also 0x81. For vsp increments larger than 0x600,use two uleb128 bytes like below: 0xc06a00e4 <unwind_test_work>: @0xc0cc9e0c Compact model index: 1 0xb2 0x81 0x02 vsp = vsp + 1544 0xac pop {r4, r5, r6, r7, r8, r14} In this case,the decoded uleb128 result is 0x101(vsp=0x204+(0x101<<2)). While the uleb128 used in code is 0x81(vsp=0x204+(0x81<<2)). The unwind aborts at this frame since it gets incorrect vsp. To fix this,add uleb128 decode to cover all the above case. Signed-off-by: Haibo Li <haibo.li@mediatek.com> --- arch/arm/kernel/unwind.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-)