2023-04-14 06:16:58

by Haibo Li

[permalink] [raw]
Subject: [PATCH v3] ARM:unwind:fix unwind abort for uleb128 case

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]>
---
v3:
-As AngeloGioacchino Del Regno suggested,improve the comment and compress the code

v2:
- As Linus Walleij and Alexandre Mergnat suggested,add comments for unwind_decode_uleb128
- As Alexandre Mergnat suggested,change variables declaration in Alphabetical order
---
arch/arm/kernel/unwind.c | 25 ++++++++++++++++++++++++-
1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/arch/arm/kernel/unwind.c b/arch/arm/kernel/unwind.c
index 53be7ea6181b..9d2192156087 100644
--- a/arch/arm/kernel/unwind.c
+++ b/arch/arm/kernel/unwind.c
@@ -308,6 +308,29 @@ 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 bytes = 0;
+ unsigned long insn;
+ unsigned long result = 0;
+
+ /*
+ * unwind_get_byte() will advance `ctrl` one instruction at a time, so
+ * loop until we get an instruction byte where bit 7 is not set.
+ *
+ * Note: This decodes a maximum of 4 bytes to output 28 bits data where
+ * max is 0xfffffff: that will cover a vsp increment of 1073742336, hence
+ * it is sufficient for unwinding the stack.
+ */
+ do {
+ insn = unwind_get_byte(ctrl);
+ result |= (insn & 0x7f) << (bytes * 7);
+ bytes++;
+ } while (!!(insn & 0x80) && (bytes != sizeof(result)));
+
+ return result;
+}
+
/*
* Execute the current unwind instruction.
*/
@@ -361,7 +384,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


2023-04-14 07:22:51

by Linus Walleij

[permalink] [raw]
Subject: Re: [PATCH v3] ARM:unwind:fix unwind abort for uleb128 case

On Fri, Apr 14, 2023 at 8:10 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]>

Way to go, remember to collect all the Reviewed-by tags before
you put the patch into Russell's patch tracker.

Yours,
Linus Walleij

2023-04-17 09:48:48

by Haibo Li

[permalink] [raw]
Subject: Re: [PATCH v3] ARM:unwind:fix unwind abort for uleb128 case

> On Fri, Apr 14, 2023 at 8:10 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]>

>

> Way to go, remember to collect all the Reviewed-by tags before you put the

> patch into Russell's patch tracker.

>

> Yours,

> Linus Walleij

Thanks for reminding.I put the patch into Russell's patch tracker just now,

together with the three Reviewed-by tags from you,

Alexandre Mergnat and AngeloGioacchino Del Regno.

Thank you all.