[PATCH 1/1] aarch64: fix a crash during maintenance print cooked-registers

[PATCH 1/1] aarch64: fix a crash during maintenance print cooked-registers

[Klaus Gerlicher]

Hi all, Luis,

I found this accidently while looking at Thiago's GDBserver improvements. Luis
I think added the pseudo register in question, so maybe he can comment
on this issue. I used a aarch64 qemu 9.2.

Thanks
Klaus

On aarch64 with pauth enabled I can see a crash when
using "maintenance print cooked-registers".

This happens because the register dump code tries to read
a pseudo reg that is not handled here because it is supposedly
only used in unwinding.

Fix this by returning a zero value typed as a built-in uint64.
---
 gdb/aarch64-tdep.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 840f9877361..6e712b12b86 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -3290,6 +3290,9 @@ aarch64_pseudo_read_value (gdbarch *gdbarch, const frame_info_ptr &next_frame,
     return aarch64_pseudo_read_value_1 (next_frame, pseudo_reg_num,
 					pseudo_offset - AARCH64_SVE_V0_REGNUM);
 
+  if (tdep->has_pauth () && pseudo_reg_num == tdep->ra_sign_state_regnum)
+    return value::zero (builtin_type (gdbarch)->builtin_uint64, lval_register);
+
   gdb_assert_not_reached ("regnum out of bound");
 }
 
-- 
2.34.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH 1/1] aarch64: fix a crash during maintenance print cooked-registers

[Luis Machado]

Hi,

On 2/3/25 13:52, Klaus Gerlicher wrote:
> Hi all, Luis,
> 
> I found this accidently while looking at Thiago's GDBserver improvements. Luis
> I think added the pseudo register in question, so maybe he can comment
> on this issue. I used a aarch64 qemu 9.2.

Thanks for catching this.

> 
> Thanks
> Klaus
> 
> On aarch64 with pauth enabled I can see a crash when
> using "maintenance print cooked-registers".
> 
> This happens because the register dump code tries to read
> a pseudo reg that is not handled here because it is supposedly
> only used in unwinding.

Indeed. ra_sign_state is only used to track which frames have PAC enabled or not.

I was wondering why I had not caught this before, and I just noticed my VM had pauth disabled. :-(

Enabling it, now I see the crash. In any case...

> 
> Fix this by returning a zero value typed as a built-in uint64.
> ---
>  gdb/aarch64-tdep.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> index 840f9877361..6e712b12b86 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -3290,6 +3290,9 @@ aarch64_pseudo_read_value (gdbarch *gdbarch, const frame_info_ptr &next_frame,
>      return aarch64_pseudo_read_value_1 (next_frame, pseudo_reg_num,
>  					pseudo_offset - AARCH64_SVE_V0_REGNUM);
>  
> +  if (tdep->has_pauth () && pseudo_reg_num == tdep->ra_sign_state_regnum)
> +    return value::zero (builtin_type (gdbarch)->builtin_uint64, lval_register);
> +
>    gdb_assert_not_reached ("regnum out of bound");
>  }
>  

... the fix above looks OK to me. Though it makes me wonder why we starting hitting this.

Internally we don't assign a name to this register so it technically doesn't show up in any
listing. Maybe something changed and we now list even pseudo registers without a name?

Approved-By: Luis Machado <luis.machado@arm.com>

RE: [PATCH 1/1] aarch64: fix a crash during maintenance print cooked-registers

[Gerlicher, Klaus]

Hi Luis,

"info all-registers" ignores registers w/o name but "maint print cooked-registers" does not and I think that hasn't changed. I think maintenance commands aren't supposed to hide anything since they are used more in GDB development than by a GDB user.

Anyway, thanks for the quick feedback, I'll push this some time soon.

Thanks
Klaus

> -----Original Message-----
> From: Luis Machado <luis.machado@arm.com>
> Sent: Monday, February 3, 2025 3:28 PM
> To: Gerlicher, Klaus <klaus.gerlicher@intel.com>; gdb-
> patches@sourceware.org
> Subject: Re: [PATCH 1/1] aarch64: fix a crash during maintenance print
> cooked-registers
> 
> Hi,
> 
> On 2/3/25 13:52, Klaus Gerlicher wrote:
> > Hi all, Luis,
> >
> > I found this accidently while looking at Thiago's GDBserver improvements.
> Luis
> > I think added the pseudo register in question, so maybe he can comment
> > on this issue. I used a aarch64 qemu 9.2.
> 
> Thanks for catching this.
> 
> >
> > Thanks
> > Klaus
> >
> > On aarch64 with pauth enabled I can see a crash when
> > using "maintenance print cooked-registers".
> >
> > This happens because the register dump code tries to read
> > a pseudo reg that is not handled here because it is supposedly
> > only used in unwinding.
> 
> Indeed. ra_sign_state is only used to track which frames have PAC enabled or
> not.
> 
> I was wondering why I had not caught this before, and I just noticed my VM
> had pauth disabled. :-(
> 
> Enabling it, now I see the crash. In any case...
> 
> >
> > Fix this by returning a zero value typed as a built-in uint64.
> > ---
> >  gdb/aarch64-tdep.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> > index 840f9877361..6e712b12b86 100644
> > --- a/gdb/aarch64-tdep.c
> > +++ b/gdb/aarch64-tdep.c
> > @@ -3290,6 +3290,9 @@ aarch64_pseudo_read_value (gdbarch *gdbarch,
> const frame_info_ptr &next_frame,
> >      return aarch64_pseudo_read_value_1 (next_frame, pseudo_reg_num,
> >  					pseudo_offset -
> AARCH64_SVE_V0_REGNUM);
> >
> > +  if (tdep->has_pauth () && pseudo_reg_num == tdep-
> >ra_sign_state_regnum)
> > +    return value::zero (builtin_type (gdbarch)->builtin_uint64, lval_register);
> > +
> >    gdb_assert_not_reached ("regnum out of bound");
> >  }
> >
> 
> ... the fix above looks OK to me. Though it makes me wonder why we starting
> hitting this.
> 
> Internally we don't assign a name to this register so it technically doesn't show
> up in any
> listing. Maybe something changed and we now list even pseudo registers
> without a name?
> 
> Approved-By: Luis Machado <luis.machado@arm.com>
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH 1/1] aarch64: fix a crash during maintenance print cooked-registers

[Luis Machado]

On 2/3/25 14:38, Gerlicher, Klaus wrote:
> Hi Luis,
> 
> "info all-registers" ignores registers w/o name but "maint print cooked-registers" does not and I think that hasn't changed. I think maintenance commands aren't supposed to hide anything since they are used more in GDB development than by a GDB user.

That make sense. Probably an oversight on our end when we first enabled this.

> 
> Anyway, thanks for the quick feedback, I'll push this some time soon.

Sounds good. Thanks.

> 
> Thanks
> Klaus
> 
>> -----Original Message-----
>> From: Luis Machado <luis.machado@arm.com>
>> Sent: Monday, February 3, 2025 3:28 PM
>> To: Gerlicher, Klaus <klaus.gerlicher@intel.com>; gdb-
>> patches@sourceware.org
>> Subject: Re: [PATCH 1/1] aarch64: fix a crash during maintenance print
>> cooked-registers
>>
>> Hi,
>>
>> On 2/3/25 13:52, Klaus Gerlicher wrote:
>>> Hi all, Luis,
>>>
>>> I found this accidently while looking at Thiago's GDBserver improvements.
>> Luis
>>> I think added the pseudo register in question, so maybe he can comment
>>> on this issue. I used a aarch64 qemu 9.2.
>>
>> Thanks for catching this.
>>
>>>
>>> Thanks
>>> Klaus
>>>
>>> On aarch64 with pauth enabled I can see a crash when
>>> using "maintenance print cooked-registers".
>>>
>>> This happens because the register dump code tries to read
>>> a pseudo reg that is not handled here because it is supposedly
>>> only used in unwinding.
>>
>> Indeed. ra_sign_state is only used to track which frames have PAC enabled or
>> not.
>>
>> I was wondering why I had not caught this before, and I just noticed my VM
>> had pauth disabled. :-(
>>
>> Enabling it, now I see the crash. In any case...
>>
>>>
>>> Fix this by returning a zero value typed as a built-in uint64.
>>> ---
>>>  gdb/aarch64-tdep.c | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
>>> index 840f9877361..6e712b12b86 100644
>>> --- a/gdb/aarch64-tdep.c
>>> +++ b/gdb/aarch64-tdep.c
>>> @@ -3290,6 +3290,9 @@ aarch64_pseudo_read_value (gdbarch *gdbarch,
>> const frame_info_ptr &next_frame,
>>>      return aarch64_pseudo_read_value_1 (next_frame, pseudo_reg_num,
>>>  					pseudo_offset -
>> AARCH64_SVE_V0_REGNUM);
>>>
>>> +  if (tdep->has_pauth () && pseudo_reg_num == tdep-
>>> ra_sign_state_regnum)
>>> +    return value::zero (builtin_type (gdbarch)->builtin_uint64, lval_register);
>>> +
>>>    gdb_assert_not_reached ("regnum out of bound");
>>>  }
>>>
>>
>> ... the fix above looks OK to me. Though it makes me wonder why we starting
>> hitting this.
>>
>> Internally we don't assign a name to this register so it technically doesn't show
>> up in any
>> listing. Maybe something changed and we now list even pseudo registers
>> without a name?
>>
>> Approved-By: Luis Machado <luis.machado@arm.com>
> Intel Deutschland GmbH
> Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
> Tel: +49 89 99 8853-0, www.intel.de
> Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
> Chairperson of the Supervisory Board: Nicole Lau
> Registered Office: Munich
> Commercial Register: Amtsgericht Muenchen HRB 186928

[PUSHED][PATCH 1/1] aarch64: fix a crash during maintenance print cooked-registers

[Gerlicher, Klaus]

> -----Original Message-----
> From: Luis Machado <luis.machado@arm.com>
> Sent: Monday, February 3, 2025 3:41 PM
> To: Gerlicher, Klaus <klaus.gerlicher@intel.com>; gdb-
> patches@sourceware.org
> Subject: Re: [PATCH 1/1] aarch64: fix a crash during maintenance print
> cooked-registers
> 
> On 2/3/25 14:38, Gerlicher, Klaus wrote:
> > Hi Luis,
> >
> > "info all-registers" ignores registers w/o name but "maint print cooked-
> registers" does not and I think that hasn't changed. I think maintenance
> commands aren't supposed to hide anything since they are used more in GDB
> development than by a GDB user.
> 
> That make sense. Probably an oversight on our end when we first enabled
> this.
> 
> >
> > Anyway, thanks for the quick feedback, I'll push this some time soon.
> 
> Sounds good. Thanks.
> 
> >
> > Thanks
> > Klaus
> >
> >> -----Original Message-----
> >> From: Luis Machado <luis.machado@arm.com>
> >> Sent: Monday, February 3, 2025 3:28 PM
> >> To: Gerlicher, Klaus <klaus.gerlicher@intel.com>; gdb-
> >> patches@sourceware.org
> >> Subject: Re: [PATCH 1/1] aarch64: fix a crash during maintenance print
> >> cooked-registers
> >>
> >> Hi,
> >>
> >> On 2/3/25 13:52, Klaus Gerlicher wrote:
> >>> Hi all, Luis,
> >>>
> >>> I found this accidently while looking at Thiago's GDBserver improvements.
> >> Luis
> >>> I think added the pseudo register in question, so maybe he can comment
> >>> on this issue. I used a aarch64 qemu 9.2.
> >>
> >> Thanks for catching this.
> >>
> >>>
> >>> Thanks
> >>> Klaus
> >>>
> >>> On aarch64 with pauth enabled I can see a crash when
> >>> using "maintenance print cooked-registers".
> >>>
> >>> This happens because the register dump code tries to read
> >>> a pseudo reg that is not handled here because it is supposedly
> >>> only used in unwinding.
> >>
> >> Indeed. ra_sign_state is only used to track which frames have PAC enabled
> or
> >> not.
> >>
> >> I was wondering why I had not caught this before, and I just noticed my
> VM
> >> had pauth disabled. :-(
> >>
> >> Enabling it, now I see the crash. In any case...
> >>
> >>>
> >>> Fix this by returning a zero value typed as a built-in uint64.
> >>> ---
> >>>  gdb/aarch64-tdep.c | 3 +++
> >>>  1 file changed, 3 insertions(+)
> >>>
> >>> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> >>> index 840f9877361..6e712b12b86 100644
> >>> --- a/gdb/aarch64-tdep.c
> >>> +++ b/gdb/aarch64-tdep.c
> >>> @@ -3290,6 +3290,9 @@ aarch64_pseudo_read_value (gdbarch
> *gdbarch,
> >> const frame_info_ptr &next_frame,
> >>>      return aarch64_pseudo_read_value_1 (next_frame, pseudo_reg_num,
> >>>  					pseudo_offset -
> >> AARCH64_SVE_V0_REGNUM);
> >>>
> >>> +  if (tdep->has_pauth () && pseudo_reg_num == tdep-
> >>> ra_sign_state_regnum)
> >>> +    return value::zero (builtin_type (gdbarch)->builtin_uint64,
> lval_register);
> >>> +
> >>>    gdb_assert_not_reached ("regnum out of bound");
> >>>  }
> >>>
> >>
> >> ... the fix above looks OK to me. Though it makes me wonder why we
> starting
> >> hitting this.
> >>
> >> Internally we don't assign a name to this register so it technically doesn't
> show
> >> up in any
> >> listing. Maybe something changed and we now list even pseudo registers
> >> without a name?
> >>
> >> Approved-By: Luis Machado <luis.machado@arm.com>
> > Intel Deutschland GmbH
> > Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
> > Tel: +49 89 99 8853-0, www.intel.de
> > Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
> > Chairperson of the Supervisory Board: Nicole Lau
> > Registered Office: Munich
> > Commercial Register: Amtsgericht Muenchen HRB 186928

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928