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 <[email protected]>
---
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__ */
-
#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 {
--
2.25.1
On Fri, Apr 7, 2023 at 5:33 AM Haibo Li <[email protected]> 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 <[email protected]>
[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 <[email protected]> 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 <[email protected]>
>
> [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 <[email protected]> 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 <[email protected]>
> ---
> 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.
--
Regards,
Alexandre
> 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 <[email protected]>
> > ---
> > 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.
>