[PATCH] AArch64 pauth: Indicate addresses in backtrace for kernel

[PATCH] AArch64 pauth: Indicate addresses in backtrace for kernel

[Kuan-Ying Lee via Gdb-patches]

Armv8.3-a Pointer Authentication cause the function return address to
be changed. GDB need to use address bit[55] to know which mode is active
and mask/unmask the link register in order to get backtrace.

If address is in kernel mode, we mask the address. If address is in user mode,
we need to unmask the address.
---
 gdb/aarch64-tdep.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 4b5af4616af..d4bb4305cea 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -257,7 +257,10 @@ aarch64_frame_unmask_lr (struct gdbarch_tdep *tdep,
     {
       int cmask_num = AARCH64_PAUTH_CMASK_REGNUM (tdep->pauth_reg_base);
       CORE_ADDR cmask = frame_unwind_register_unsigned (this_frame, cmask_num);
-      addr = addr & ~cmask;
+      if (addr & 0x0080000000000000ULL)
+        addr = addr | cmask;
+      else
+        addr = addr & ~cmask;
 
       /* Record in the frame that the link register required unmasking.  */
       set_frame_previous_pc_masked (this_frame);
-- 
2.25.1

Re: [PATCH] AArch64 pauth: Indicate addresses in backtrace for kernel

[Luis Machado via Gdb-patches]

On 10/25/21 8:47 AM, Kuan-Ying Lee via Gdb-patches wrote:
> Armv8.3-a Pointer Authentication cause the function return address to
> be changed. GDB need to use address bit[55] to know which mode is active
> and mask/unmask the link register in order to get backtrace.
> 
> If address is in kernel mode, we mask the address. If address is in user mode,
> we need to unmask the address.
> ---
>   gdb/aarch64-tdep.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> index 4b5af4616af..d4bb4305cea 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -257,7 +257,10 @@ aarch64_frame_unmask_lr (struct gdbarch_tdep *tdep,
>       {
>         int cmask_num = AARCH64_PAUTH_CMASK_REGNUM (tdep->pauth_reg_base);
>         CORE_ADDR cmask = frame_unwind_register_unsigned (this_frame, cmask_num);
> -      addr = addr & ~cmask;
> +      if (addr & 0x0080000000000000ULL)
> +        addr = addr | cmask;
> +      else
> +        addr = addr & ~cmask;
>   
>         /* Record in the frame that the link register required unmasking.  */
>         set_frame_previous_pc_masked (this_frame);
> 

Could you please share more information about this problem? Why is it 
GDB needs to do things differently for a kernel mode and user mode 
address? What is the test setup?

If we entered the above conditional block, that means DWARF has told GDB 
that LR is masked (ra_state_regnum), and so it needs to be unmasked.

Given this is generic AArch64 code, we don't want to risk breaking 
existing use cases, and I'd like to understand what is not being handled 
properly.

We have another gdbarch method that handles sign-extending kernel mode 
addresses (gdbarch_significant_addr_bit), but it is not clear if that 
could be used here without some examples.

Thanks,
Luis

Re: [PATCH] AArch64 pauth: Indicate addresses in backtrace for kernel

[Kuan-Ying Lee via Gdb-patches]

On Mon, 2021-10-25 at 20:07 +0800, Luis Machado wrote:
> On 10/25/21 8:47 AM, Kuan-Ying Lee via Gdb-patches wrote:
> > Armv8.3-a Pointer Authentication cause the function return address
> > to
> > be changed. GDB need to use address bit[55] to know which mode is
> > active
> > and mask/unmask the link register in order to get backtrace.
> > 
> > If address is in kernel mode, we mask the address. If address is in
> > user mode,
> > we need to unmask the address.
> > ---
> >   gdb/aarch64-tdep.c | 5 ++++-
> >   1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> > index 4b5af4616af..d4bb4305cea 100644
> > --- a/gdb/aarch64-tdep.c
> > +++ b/gdb/aarch64-tdep.c
> > @@ -257,7 +257,10 @@ aarch64_frame_unmask_lr (struct gdbarch_tdep
> > *tdep,
> >       {
> >         int cmask_num = AARCH64_PAUTH_CMASK_REGNUM (tdep-
> > >pauth_reg_base);
> >         CORE_ADDR cmask = frame_unwind_register_unsigned
> > (this_frame, cmask_num);
> > -      addr = addr & ~cmask;
> > +      if (addr & 0x0080000000000000ULL)
> > +        addr = addr | cmask;
> > +      else
> > +        addr = addr & ~cmask;
> >   
> >         /* Record in the frame that the link register required
> > unmasking.  */
> >         set_frame_previous_pc_masked (this_frame);
> > 
> 

Hi Luis,

Thanks for your reply.

I think I use the misunderstanding words 'mask' and 'unmask'.
I will use 'unmangle' in the v2 commit message instead.

Backtrace needs to be printed as unmangle address not only in kernel
mode but also user mode.

However, when we use arm64, kernel address will start with 0xffff....
and user mode address will start with 0x0000....
High bits in kernel address and user mode address are different.

Thus, I think we should do the following two things.
1. If address is in kernel mode. Unmangle the kernel address by 'addr |
cmask'.
2. If address is in user mode. Unmangle the user mode address by 'addr
& ~cmask'

We can see this kernel patch [1] use different unmangle method for user
mode and kernel mode.

After my patch, we can parse kernel backtrace as below. (address starts
with 0xffff...)
(gdb)bt
#0  0xffffffdc252a5c44 in crash_setup_regs (newregs=0xffffffdc252b54a8,
oldregs=0x0)
    at /mfs/mtkslt1092/mtk21026/CAS_REAL/alps-mp-s0_mp1
--2021_09_28_11_00/merged/kernel-5.10/arch/arm64/include/asm/kexec.h:45
#1  ipanic (this=<optimized out>, event=<optimized out>, ptr=<optimized
out>)
    at /mfs/mtkslt1092/mtk21026/CAS_REAL/alps-mp-s0_mp1
--2021_09_28_11_00/merged/kernel-
5.10/drivers/misc/mediatek/aee/mrdump/mrdump_panic.c:259
#2  0xffffffdc291acbe8 [PAC] in notifier_call_chain (nl=<optimized
out>, val=<optimized out>, v=<optimized out>, nr_to_call=<optimized
out>,
    nr_calls=0x0) at /mfs/mtkslt1092/mtk21026/CAS_REAL/alps-mp-s0_mp1
--2021_09_28_11_00/merged/kernel-5.10/kernel/notifier.c:83
#3  atomic_notifier_call_chain (nh=<optimized out>, val=0,
v=0xffffffdc2c3aa1c8 <panic.buf>)
    at /mfs/mtkslt1092/mtk21026/CAS_REAL/alps-mp-s0_mp1
--2021_09_28_11_00/merged/kernel-5.10/kernel/notifier.c:217
#4  0xffffffdc291acbe8 [PAC] in notifier_call_chain (nl=<optimized
out>, val=<optimized out>, v=<optimized out>, nr_to_call=<optimized
out>,
    nr_calls=0x0) at /mfs/mtkslt1092/mtk21026/CAS_REAL/alps-mp-s0_mp1
--2021_09_28_11_00/merged/kernel-5.10/kernel/notifier.c:83
#5  atomic_notifier_call_chain (nh=<optimized out>,
val=18446743919823523840, v=0x0)
    at /mfs/mtkslt1092/mtk21026/CAS_REAL/alps-mp-s0_mp1
--2021_09_28_11_00/merged/kernel-5.10/kernel/notifier.c:217
#6  0xffffffdc2914eb90 [PAC] in panic (fmt=<optimized out>)
    at /mfs/mtkslt1092/mtk21026/CAS_REAL/alps-mp-s0_mp1
--2021_09_28_11_00/merged/kernel-5.10/kernel/panic.c:272
#7  0xffffffdc29c2eb78 [PAC] in sysrq_handle_crash (key=<optimized
out>)
    at /mfs/mtkslt1092/mtk21026/CAS_REAL/alps-mp-s0_mp1
--2021_09_28_11_00/merged/kernel-5.10/drivers/tty/sysrq.c:158
#8  0xffffffdc29c2fab8 [PAC] in __handle_sysrq (key=99,
check_mask=<optimized out>)
    at /mfs/mtkslt1092/mtk21026/CAS_REAL/alps-mp-s0_mp1
--2021_09_28_11_00/merged/kernel-5.10/drivers/tty/sysrq.c:602

This user mode backtrace as below is from [2].
(gdb) bt
0  0x0000000000400490 in puts@plt ()
1  0x00000000004005dc in foo ("hello") at cbreak-lib.c:6
2  0x0000000000400604 [PAC] in bar () at cbreak-lib.c:12
3  0x0000000000400620 [PAC] in main2 () at cbreak.c:17
4  0x00000000004005b4 in main () at cbreak-3.c:10

> Could you please share more information about this problem? Why is
> it 
> GDB needs to do things differently for a kernel mode and user mode 
> address? What is the test setup?

Explanation is on the above.

> 
> If we entered the above conditional block, that means DWARF has told
> GDB 
> that LR is masked (ra_state_regnum), and so it needs to be unmasked.

Yes, it needs to be 'unmangled'.

However, kernel address and user mode address need different unmangle
methods.
> 
> Given this is generic AArch64 code, we don't want to risk breaking 
> existing use cases, and I'd like to understand what is not being
> handled 
> properly.
> 
I am not sure if changing code like this has risk or not.
Need some gdb experts for review.

Any suggestion and reviews are welcome.
Thanks. :)
> We have another gdbarch method that handles sign-extending kernel
> mode 
> addresses (gdbarch_significant_addr_bit), but it is not clear if
> that 
> could be used here without some examples.

I think we cannot use significant bit when enable PAC feature.
If enable PAC feature, we need to use bit[55] to know if address is in
kernel address. (Can refer to [3])

[1] arm64: mask PAC bits of __builtin_return_address
 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=689eae42afd7a916634146edca38463769969184

[2] AArch64 pauth: Indicate unmasked addresses in backtrace

https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=3d31bc39e655ea39721754fa0ea539a8a0c9b26c

[3] 
https://developer.arm.com/documentation/102433/0100/Return-oriented-programming

Thanks,
Kuan-Ying Lee
> 
> Thanks,
> Luis

Re: [PATCH] AArch64 pauth: Indicate addresses in backtrace for kernel

[Luis Machado via Gdb-patches]

Hi,

On 10/26/21 9:22 AM, Kuan-Ying Lee wrote:
> On Mon, 2021-10-25 at 20:07 +0800, Luis Machado wrote:
>> On 10/25/21 8:47 AM, Kuan-Ying Lee via Gdb-patches wrote:
>>> Armv8.3-a Pointer Authentication cause the function return address
>>> to
>>> be changed. GDB need to use address bit[55] to know which mode is
>>> active
>>> and mask/unmask the link register in order to get backtrace.
>>>
>>> If address is in kernel mode, we mask the address. If address is in
>>> user mode,
>>> we need to unmask the address.
>>> ---
>>>    gdb/aarch64-tdep.c | 5 ++++-
>>>    1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
>>> index 4b5af4616af..d4bb4305cea 100644
>>> --- a/gdb/aarch64-tdep.c
>>> +++ b/gdb/aarch64-tdep.c
>>> @@ -257,7 +257,10 @@ aarch64_frame_unmask_lr (struct gdbarch_tdep
>>> *tdep,
>>>        {
>>>          int cmask_num = AARCH64_PAUTH_CMASK_REGNUM (tdep-
>>>> pauth_reg_base);
>>>          CORE_ADDR cmask = frame_unwind_register_unsigned
>>> (this_frame, cmask_num);
>>> -      addr = addr & ~cmask;
>>> +      if (addr & 0x0080000000000000ULL)
>>> +        addr = addr | cmask;
>>> +      else
>>> +        addr = addr & ~cmask;
>>>    
>>>          /* Record in the frame that the link register required
>>> unmasking.  */
>>>          set_frame_previous_pc_masked (this_frame);
>>>
>>
> 
> Hi Luis,
> 
> Thanks for your reply.
> 
> I think I use the misunderstanding words 'mask' and 'unmask'.
> I will use 'unmangle' in the v2 commit message instead.

No problem. That part was OK.

> 
> Backtrace needs to be printed as unmangle address not only in kernel
> mode but also user mode.
> 
> However, when we use arm64, kernel address will start with 0xffff....
> and user mode address will start with 0x0000....
> High bits in kernel address and user mode address are different.
> 
> Thus, I think we should do the following two things.
> 1. If address is in kernel mode. Unmangle the kernel address by 'addr |
> cmask'.
> 2. If address is in user mode. Unmangle the user mode address by 'addr
> & ~cmask'
> 
> We can see this kernel patch [1] use different unmangle method for user
> mode and kernel mode.

Thanks for the explanation. That makes sense now, and the current code 
is only handling usermode addresses.

I was slightly confused with bit 55, but I see it is a VA range select bit.

> 
> After my patch, we can parse kernel backtrace as below. (address starts
> with 0xffff...)
> (gdb)bt
> #0  0xffffffdc252a5c44 in crash_setup_regs (newregs=0xffffffdc252b54a8,
> oldregs=0x0)
>      at /mfs/mtkslt1092/mtk21026/CAS_REAL/alps-mp-s0_mp1
> --2021_09_28_11_00/merged/kernel-5.10/arch/arm64/include/asm/kexec.h:45
> #1  ipanic (this=<optimized out>, event=<optimized out>, ptr=<optimized
> out>)
>      at /mfs/mtkslt1092/mtk21026/CAS_REAL/alps-mp-s0_mp1
> --2021_09_28_11_00/merged/kernel-
> 5.10/drivers/misc/mediatek/aee/mrdump/mrdump_panic.c:259
> #2  0xffffffdc291acbe8 [PAC] in notifier_call_chain (nl=<optimized
> out>, val=<optimized out>, v=<optimized out>, nr_to_call=<optimized
> out>,
>      nr_calls=0x0) at /mfs/mtkslt1092/mtk21026/CAS_REAL/alps-mp-s0_mp1
> --2021_09_28_11_00/merged/kernel-5.10/kernel/notifier.c:83
> #3  atomic_notifier_call_chain (nh=<optimized out>, val=0,
> v=0xffffffdc2c3aa1c8 <panic.buf>)
>      at /mfs/mtkslt1092/mtk21026/CAS_REAL/alps-mp-s0_mp1
> --2021_09_28_11_00/merged/kernel-5.10/kernel/notifier.c:217
> #4  0xffffffdc291acbe8 [PAC] in notifier_call_chain (nl=<optimized
> out>, val=<optimized out>, v=<optimized out>, nr_to_call=<optimized
> out>,
>      nr_calls=0x0) at /mfs/mtkslt1092/mtk21026/CAS_REAL/alps-mp-s0_mp1
> --2021_09_28_11_00/merged/kernel-5.10/kernel/notifier.c:83
> #5  atomic_notifier_call_chain (nh=<optimized out>,
> val=18446743919823523840, v=0x0)
>      at /mfs/mtkslt1092/mtk21026/CAS_REAL/alps-mp-s0_mp1
> --2021_09_28_11_00/merged/kernel-5.10/kernel/notifier.c:217
> #6  0xffffffdc2914eb90 [PAC] in panic (fmt=<optimized out>)
>      at /mfs/mtkslt1092/mtk21026/CAS_REAL/alps-mp-s0_mp1
> --2021_09_28_11_00/merged/kernel-5.10/kernel/panic.c:272
> #7  0xffffffdc29c2eb78 [PAC] in sysrq_handle_crash (key=<optimized
> out>)
>      at /mfs/mtkslt1092/mtk21026/CAS_REAL/alps-mp-s0_mp1
> --2021_09_28_11_00/merged/kernel-5.10/drivers/tty/sysrq.c:158
> #8  0xffffffdc29c2fab8 [PAC] in __handle_sysrq (key=99,
> check_mask=<optimized out>)
>      at /mfs/mtkslt1092/mtk21026/CAS_REAL/alps-mp-s0_mp1
> --2021_09_28_11_00/merged/kernel-5.10/drivers/tty/sysrq.c:602
> 
> This user mode backtrace as below is from [2].
> (gdb) bt
> 0  0x0000000000400490 in puts@plt ()
> 1  0x00000000004005dc in foo ("hello") at cbreak-lib.c:6
> 2  0x0000000000400604 [PAC] in bar () at cbreak-lib.c:12
> 3  0x0000000000400620 [PAC] in main2 () at cbreak.c:17
> 4  0x00000000004005b4 in main () at cbreak-3.c:10
> 
>> Could you please share more information about this problem? Why is
>> it
>> GDB needs to do things differently for a kernel mode and user mode
>> address? What is the test setup?
> 
> Explanation is on the above.
> 
>>
>> If we entered the above conditional block, that means DWARF has told
>> GDB
>> that LR is masked (ra_state_regnum), and so it needs to be unmasked.
> 
> Yes, it needs to be 'unmangled'.
> 
> However, kernel address and user mode address need different unmangle
> methods.
>>
>> Given this is generic AArch64 code, we don't want to risk breaking
>> existing use cases, and I'd like to understand what is not being
>> handled
>> properly.
>>
> I am not sure if changing code like this has risk or not.
> Need some gdb experts for review.

That's what I'm trying to assess with your help. I'll go through the 
patch once again with a better understanding of your use case.

> 
> Any suggestion and reviews are welcome.
> Thanks. :)
>> We have another gdbarch method that handles sign-extending kernel
>> mode
>> addresses (gdbarch_significant_addr_bit), but it is not clear if
>> that
>> could be used here without some examples.
> 
> I think we cannot use significant bit when enable PAC feature.
> If enable PAC feature, we need to use bit[55] to know if address is in
> kernel address. (Can refer to [3])
> 
> [1] arm64: mask PAC bits of __builtin_return_address
>   
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=689eae42afd7a916634146edca38463769969184
> 
> [2] AArch64 pauth: Indicate unmasked addresses in backtrace
> 
> https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=3d31bc39e655ea39721754fa0ea539a8a0c9b26c
> 
> [3]
> https://developer.arm.com/documentation/102433/0100/Return-oriented-programming
> 
> Thanks,
> Kuan-Ying Lee
>>
>> Thanks,
>> Luis
> 
> 
> 

Re: [PATCH] AArch64 pauth: Indicate addresses in backtrace for kernel

[Luis Machado via Gdb-patches]

Hi!

Second time's the charm.

On 10/25/21 8:47 AM, Kuan-Ying Lee via Gdb-patches wrote:
> Armv8.3-a Pointer Authentication cause the function return address to
> be changed. GDB need to use address bit[55] to know which mode is active
> and mask/unmask the link register in order to get backtrace.
> 
> If address is in kernel mode, we mask the address. If address is in user mode,
> we need to unmask the address.
> ---
>   gdb/aarch64-tdep.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> index 4b5af4616af..d4bb4305cea 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -257,7 +257,10 @@ aarch64_frame_unmask_lr (struct gdbarch_tdep *tdep,
>       {
>         int cmask_num = AARCH64_PAUTH_CMASK_REGNUM (tdep->pauth_reg_base);
>         CORE_ADDR cmask = frame_unwind_register_unsigned (this_frame, cmask_num);
> -      addr = addr & ~cmask;
> +      if (addr & 0x0080000000000000ULL)

I think we should define this constant in aarch64-tdep.h to make it more 
obvious:

#define AARCH64_PAC_VA_RANGE_BIT 55
#define AARCH64_PAC_VA_RANGE_MASK (1ULL << AARCH64_PAC_VA_RANGE_BIT)

> +        addr = addr | cmask;
> +      else
> +        addr = addr & ~cmask;

For the unmasking of the address, it would be nice to put this into a 
separate function that unmasks an address given a particular mask value. 
Something like this:

static CORE_ADDR
aarch64_unmask_address (CORE_ADDR address, CORE_ADDR mask)
{
   /* Unmask kernel mode and user mode addresses appropriately based on
      the VA range bit.  */
   if (address & AARCH64_PAC_VA_RANGE_MASK)
     address | mask;
   else
     address & ~mask;

   return address;
}

If we ever need to unmask kernel/user addresses somewhere else in the 
code, we can just call this function from now on.

Could you please send a v2 of the patch with the suggested changes?

Thanks for the patch.

Re: [PATCH] AArch64 pauth: Indicate addresses in backtrace for kernel

[Kuan-Ying Lee via Gdb-patches]

On Tue, 2021-10-26 at 20:46 +0800, Luis Machado wrote:
> Hi!
> 
> Second time's the charm.
> 
> On 10/25/21 8:47 AM, Kuan-Ying Lee via Gdb-patches wrote:
> > Armv8.3-a Pointer Authentication cause the function return address
> > to
> > be changed. GDB need to use address bit[55] to know which mode is
> > active
> > and mask/unmask the link register in order to get backtrace.
> > 
> > If address is in kernel mode, we mask the address. If address is in
> > user mode,
> > we need to unmask the address.
> > ---
> >   gdb/aarch64-tdep.c | 5 ++++-
> >   1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> > index 4b5af4616af..d4bb4305cea 100644
> > --- a/gdb/aarch64-tdep.c
> > +++ b/gdb/aarch64-tdep.c
> > @@ -257,7 +257,10 @@ aarch64_frame_unmask_lr (struct gdbarch_tdep
> > *tdep,
> >       {
> >         int cmask_num = AARCH64_PAUTH_CMASK_REGNUM (tdep-
> > >pauth_reg_base);
> >         CORE_ADDR cmask = frame_unwind_register_unsigned
> > (this_frame, cmask_num);
> > -      addr = addr & ~cmask;
> > +      if (addr & 0x0080000000000000ULL)
> 
> I think we should define this constant in aarch64-tdep.h to make it
> more 
> obvious:
> 
> #define AARCH64_PAC_VA_RANGE_BIT 55
> #define AARCH64_PAC_VA_RANGE_MASK (1ULL << AARCH64_PAC_VA_RANGE_BIT)
> 
> > +        addr = addr | cmask;
> > +      else
> > +        addr = addr & ~cmask;
> 
> For the unmasking of the address, it would be nice to put this into
> a 
> separate function that unmasks an address given a particular mask
> value. 
> Something like this:
> 
> static CORE_ADDR
> aarch64_unmask_address (CORE_ADDR address, CORE_ADDR mask)
> {
>    /* Unmask kernel mode and user mode addresses appropriately based
> on
>       the VA range bit.  */
>    if (address & AARCH64_PAC_VA_RANGE_MASK)
>      address | mask;
>    else
>      address & ~mask;
> 
>    return address;
> }
> 
> If we ever need to unmask kernel/user addresses somewhere else in
> the 
> code, we can just call this function from now on.

Got it.
> 
> Could you please send a v2 of the patch with the suggested changes?

Sure.
Thanks for the suggestions.

> 
> Thanks for the patch.