[PATCH] [gdb/aarch64] pauth: Create new feature string for pauth to prevent crashing older gdb's

[PATCH] [gdb/aarch64] pauth: Create new feature string for pauth to prevent crashing older gdb's

[Luis Machado]

Older gdb's (9, 10, 11 and 12) have a bug that causes them to crash whenever
a target reports the pauth feature string in the target description and also
provide additional register outside of gdb's known and expected feature
strings.

This was fixed in gdb 13 onwards, but that means we're stuck with gdb's out
there that will crash on connection to the above targets.

QEMU has postponed inclusion of the pauth feature string in version 8, and
instead we agreed to use a new feature name to prevent crashing those older
gdb's.

Initially there was a plan to backport a trivial fix all the way to gdb 9, but
given QEMU's choice, this is no longer needed.

This new feature string is org.gnu.gdb.aarch64.pauth_v2, and should be used
by all targets going forward, except native linux gdb and gdbserver, for
backwards compatibility with older gdb's/gdbserver's.

gdb/gdbserver will still emit the old feature string for Linux since it doesn't
report additional system registers and thus doesn't cause a crash of older
gdb's. We can revisit this in the future once the problematic gdb's are likely
no longer in use.

I've added some documentation to explain the situation.
---
 gdb/aarch64-tdep.c  | 15 ++++++++++++++-
 gdb/doc/gdb.texinfo | 16 ++++++++++++++++
 2 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index d8349e4ccdb..ec0e51bdaf7 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -3500,8 +3500,15 @@ aarch64_features_from_target_desc (const struct target_desc *tdesc)
     return features;
 
   features.vq = aarch64_get_tdesc_vq (tdesc);
+
+  /* We need to look for a couple pauth feature name variations.  */
   features.pauth
       = (tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.pauth") != nullptr);
+
+  if (!features.pauth)
+    features.pauth = (tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.pauth_v2")
+		      != nullptr);
+
   features.mte
       = (tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.mte") != nullptr);
 
@@ -3679,7 +3686,6 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   feature_core = tdesc_find_feature (tdesc,"org.gnu.gdb.aarch64.core");
   feature_fpu = tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.fpu");
   feature_sve = tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.sve");
-  feature_pauth = tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.pauth");
   const struct tdesc_feature *feature_mte
     = tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.mte");
   const struct tdesc_feature *feature_tls
@@ -3773,6 +3779,13 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
 	}
     }
 
+  /* We have two versions of the pauth target description due to a past bug
+     where GDB would crash when seeing the first version of the pauth target
+     description.  */
+  feature_pauth = tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.pauth");
+  if (feature_pauth == nullptr)
+    feature_pauth = tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.pauth_v2");
+
   /* Add the pauth registers.  */
   int pauth_masks = 0;
   if (feature_pauth != NULL)
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 45a0580bc29..aefeb63f75c 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -47980,6 +47980,22 @@ has a signed link register value that needs to be unmasked/decoded.
 Extra registers are allowed in this feature, but they will not affect
 @value{GDBN}.
 
+Due to a bug in previous versions of @value{GDBN} (versions 9, 10, 11 and 12),
+a new feature string was created to prevent targets causing a @value{GDBN}
+crash whenever they reported support for Pointer Authentication and also
+reported additional system registers that were not accounted for by
+@value{GDBN}.  This is common when using emulators and on bare-metal debugging
+scenarios.
+
+The new feature string is @samp{org.gnu.gdb.aarch64.pauth_v2}, and it has
+the same contents as feature string @samp{org.gnu.gdb.aarch64.pauth}.
+
+Targets reporting Pointer Authentication support via the remote protocol
+should always use the newer feature string @samp{org.gnu.gdb.aarch64.pauth_v2}.
+
+In the future the @samp{org.gnu.gdb.aarch64.pauth} feature string might be
+deprecated in favor of @samp{org.gnu.gdb.aarch64.pauth_v2}.
+
 @subsubsection AArch64 TLS registers feature
 
 The @samp{org.gnu.gdb.aarch64.tls} optional feature was introduced to expose
-- 
2.25.1

Re: [PATCH] [gdb/aarch64] pauth: Create new feature string for pauth to prevent crashing older gdb's

[Eli Zaretskii]

> CC: <alex.bennee@linaro.org>, <richard.henderson@linaro.org>,
>  <peter.maydell@linaro.org>
> Date: Tue, 11 Apr 2023 23:43:27 +0100
> From: Luis Machado via Gdb-patches <gdb-patches@sourceware.org>
> 
> I've added some documentation to explain the situation.
> ---
>  gdb/aarch64-tdep.c  | 15 ++++++++++++++-
>  gdb/doc/gdb.texinfo | 16 ++++++++++++++++
>  2 files changed, 30 insertions(+), 1 deletion(-)

Thanks.

> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index 45a0580bc29..aefeb63f75c 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -47980,6 +47980,22 @@ has a signed link register value that needs to be unmasked/decoded.
>  Extra registers are allowed in this feature, but they will not affect
>  @value{GDBN}.
>  
> +Due to a bug in previous versions of @value{GDBN} (versions 9, 10, 11 and 12),
> +a new feature string was created to prevent targets causing a @value{GDBN}
> +crash whenever they reported support for Pointer Authentication and also
> +reported additional system registers that were not accounted for by
> +@value{GDBN}.  This is common when using emulators and on bare-metal debugging
> +scenarios.
> +
> +The new feature string is @samp{org.gnu.gdb.aarch64.pauth_v2}, and it has
> +the same contents as feature string @samp{org.gnu.gdb.aarch64.pauth}.
> +
> +Targets reporting Pointer Authentication support via the remote protocol
> +should always use the newer feature string @samp{org.gnu.gdb.aarch64.pauth_v2}.
> +
> +In the future the @samp{org.gnu.gdb.aarch64.pauth} feature string might be
> +deprecated in favor of @samp{org.gnu.gdb.aarch64.pauth_v2}.
> +

Since this is the manual for GDB users, it should describe the feature
first, and explain its use and rationale after that.  So this
description is backwards: it starts with the reason for introduction
of this feature, something that doesn't necessarily make sense to the
reader of the manual.

Instead, we should first say that 'org.gnu.gdb.aarch64.pauth' will be
deprecated in favor of 'org.gnu.gdb.aarch64.pauth_v2', then introduce
'org.gnu.gdb.aarch64.pauth_v2' saying that it has the same contents as
the former, and finally explain that targets should use
'org.gnu.gdb.aarch64.pauth_v2' to avoid the danger of crashing older
GDBs.

OK?

Reviewed-By: Eli Zaretskii <eliz@gnu.org>

Re: [PATCH] [gdb/aarch64] pauth: Create new feature string for pauth to prevent crashing older gdb's

[Luis Machado]

Hi Eli,

On 4/12/23 07:04, Eli Zaretskii wrote:
>> CC: <alex.bennee@linaro.org>, <richard.henderson@linaro.org>,
>>   <peter.maydell@linaro.org>
>> Date: Tue, 11 Apr 2023 23:43:27 +0100
>> From: Luis Machado via Gdb-patches <gdb-patches@sourceware.org>
>>
>> I've added some documentation to explain the situation.
>> ---
>>   gdb/aarch64-tdep.c  | 15 ++++++++++++++-
>>   gdb/doc/gdb.texinfo | 16 ++++++++++++++++
>>   2 files changed, 30 insertions(+), 1 deletion(-)
> 
> Thanks.
> 
>> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
>> index 45a0580bc29..aefeb63f75c 100644
>> --- a/gdb/doc/gdb.texinfo
>> +++ b/gdb/doc/gdb.texinfo
>> @@ -47980,6 +47980,22 @@ has a signed link register value that needs to be unmasked/decoded.
>>   Extra registers are allowed in this feature, but they will not affect
>>   @value{GDBN}.
>>   
>> +Due to a bug in previous versions of @value{GDBN} (versions 9, 10, 11 and 12),
>> +a new feature string was created to prevent targets causing a @value{GDBN}
>> +crash whenever they reported support for Pointer Authentication and also
>> +reported additional system registers that were not accounted for by
>> +@value{GDBN}.  This is common when using emulators and on bare-metal debugging
>> +scenarios.
>> +
>> +The new feature string is @samp{org.gnu.gdb.aarch64.pauth_v2}, and it has
>> +the same contents as feature string @samp{org.gnu.gdb.aarch64.pauth}.
>> +
>> +Targets reporting Pointer Authentication support via the remote protocol
>> +should always use the newer feature string @samp{org.gnu.gdb.aarch64.pauth_v2}.
>> +
>> +In the future the @samp{org.gnu.gdb.aarch64.pauth} feature string might be
>> +deprecated in favor of @samp{org.gnu.gdb.aarch64.pauth_v2}.
>> +
> 
> Since this is the manual for GDB users, it should describe the feature
> first, and explain its use and rationale after that.  So this
> description is backwards: it starts with the reason for introduction
> of this feature, something that doesn't necessarily make sense to the
> reader of the manual.
> 
> Instead, we should first say that 'org.gnu.gdb.aarch64.pauth' will be
> deprecated in favor of 'org.gnu.gdb.aarch64.pauth_v2', then introduce
> 'org.gnu.gdb.aarch64.pauth_v2' saying that it has the same contents as
> the former, and finally explain that targets should use
> 'org.gnu.gdb.aarch64.pauth_v2' to avoid the danger of crashing older
> GDBs.
> 
> OK?

It sounds OK.

How about the following flow?

In the future the @samp{org.gnu.gdb.aarch64.pauth} feature string might be
deprecated in favor of feature string @samp{org.gnu.gdb.aarch64.pauth_v2}.

The @samp{org.gnu.gdb.aarch64.pauth_v2} feature has the exact same contents
as feature @samp{org.gnu.gdb.aarch64.pauth}.

Targets reporting Pointer Authentication support via the remote protocol
should always use the newer feature string
@samp{org.gnu.gdb.aarch64.pauth_v2}.

The reason for having feature @samp{org.gnu.gdb.aarch64.pauth_v2} is a bug in
previous versions of @value{GDBN} (versions 9, 10, 11 and 12).  This bug
caused @value{GDBN} to crash whenever the target reported support for Pointer
Authentication (using feature string @samp{org.gnu.gdb.aarch64.pauth}) and also
reported additional system registers that were not accounted for by
@value{GDBN}.  This is common when using emulators and on bare-metal debugging
scenarios.

> 
> Reviewed-By: Eli Zaretskii <eliz@gnu.org>

Re: [PATCH] [gdb/aarch64] pauth: Create new feature string for pauth to prevent crashing older gdb's

[Eli Zaretskii]

> Date: Wed, 12 Apr 2023 09:46:31 +0100
> Cc: gdb-patches@sourceware.org, alex.bennee@linaro.org,
>  richard.henderson@linaro.org, peter.maydell@linaro.org
> From: Luis Machado <luis.machado@arm.com>
> 
> How about the following flow?
> 
> In the future the @samp{org.gnu.gdb.aarch64.pauth} feature string might be
> deprecated in favor of feature string @samp{org.gnu.gdb.aarch64.pauth_v2}.
> 
> The @samp{org.gnu.gdb.aarch64.pauth_v2} feature has the exact same contents
> as feature @samp{org.gnu.gdb.aarch64.pauth}.
> 
> Targets reporting Pointer Authentication support via the remote protocol
> should always use the newer feature string
> @samp{org.gnu.gdb.aarch64.pauth_v2}.
> 
> The reason for having feature @samp{org.gnu.gdb.aarch64.pauth_v2} is a bug in
> previous versions of @value{GDBN} (versions 9, 10, 11 and 12).  This bug
> caused @value{GDBN} to crash whenever the target reported support for Pointer
> Authentication (using feature string @samp{org.gnu.gdb.aarch64.pauth}) and also
> reported additional system registers that were not accounted for by
> @value{GDBN}.  This is common when using emulators and on bare-metal debugging
> scenarios.

The flow is OK, but the last paragraph is too long.  The main reason
of this text is to tell people to use the v2 feature, not to explain
why it exists.  So the explanation of the reason should be shorter
like

  We recommend always using the @samp{org.gnu.gdb.aarch64.pauth_v2}
  feature because using @samp{org.gnu.gdb.aarch64.pauth} could crash
  older versions of @value{GDBN} due to a known bug in those versions.

Is it really important to say that the bug happens only when
additional system registers are reported?

Re: [PATCH] [gdb/aarch64] pauth: Create new feature string for pauth to prevent crashing older gdb's

[Luis Machado]

On 4/12/23 10:20, Eli Zaretskii wrote:
>> Date: Wed, 12 Apr 2023 09:46:31 +0100
>> Cc: gdb-patches@sourceware.org, alex.bennee@linaro.org,
>>   richard.henderson@linaro.org, peter.maydell@linaro.org
>> From: Luis Machado <luis.machado@arm.com>
>>
>> How about the following flow?
>>
>> In the future the @samp{org.gnu.gdb.aarch64.pauth} feature string might be
>> deprecated in favor of feature string @samp{org.gnu.gdb.aarch64.pauth_v2}.
>>
>> The @samp{org.gnu.gdb.aarch64.pauth_v2} feature has the exact same contents
>> as feature @samp{org.gnu.gdb.aarch64.pauth}.
>>
>> Targets reporting Pointer Authentication support via the remote protocol
>> should always use the newer feature string
>> @samp{org.gnu.gdb.aarch64.pauth_v2}.
>>
>> The reason for having feature @samp{org.gnu.gdb.aarch64.pauth_v2} is a bug in
>> previous versions of @value{GDBN} (versions 9, 10, 11 and 12).  This bug
>> caused @value{GDBN} to crash whenever the target reported support for Pointer
>> Authentication (using feature string @samp{org.gnu.gdb.aarch64.pauth}) and also
>> reported additional system registers that were not accounted for by
>> @value{GDBN}.  This is common when using emulators and on bare-metal debugging
>> scenarios.
> 
> The flow is OK, but the last paragraph is too long.  The main reason
> of this text is to tell people to use the v2 feature, not to explain
> why it exists.  So the explanation of the reason should be shorter
> like

Though it is documentation for users, it is also one of the only documents providing guidance for developers of debugging stubs (like QEMU's, openOCD etc).

So I feel like it should include a bit more information rather than the bare minimum.

I don't think this particular section (XML target descriptions) is geared towards the common debugger users.

> 
>    We recommend always using the @samp{org.gnu.gdb.aarch64.pauth_v2}
>    feature because using @samp{org.gnu.gdb.aarch64.pauth} could crash
>    older versions of @value{GDBN} due to a known bug in those versions.

I think the above is sane. It just has less detail, which, as explained above, might not be too useful for
developers of debugging stubs.

> 
> Is it really important to say that the bug happens only when
> additional system registers are reported?

We could make it shorter by only mentioning this is only an issue when using emulators.

Re: [PATCH] [gdb/aarch64] pauth: Create new feature string for pauth to prevent crashing older gdb's

[Eli Zaretskii]

> Date: Wed, 12 Apr 2023 10:25:56 +0100
> Cc: gdb-patches@sourceware.org, alex.bennee@linaro.org,
>  richard.henderson@linaro.org, peter.maydell@linaro.org
> From: Luis Machado <luis.machado@arm.com>
> 
> > The flow is OK, but the last paragraph is too long.  The main reason
> > of this text is to tell people to use the v2 feature, not to explain
> > why it exists.  So the explanation of the reason should be shorter
> > like
> 
> Though it is documentation for users, it is also one of the only documents providing guidance for developers of debugging stubs (like QEMU's, openOCD etc).
> 
> So I feel like it should include a bit more information rather than the bare minimum.
> 
> I don't think this particular section (XML target descriptions) is geared towards the common debugger users.
> 
> > 
> >    We recommend always using the @samp{org.gnu.gdb.aarch64.pauth_v2}
> >    feature because using @samp{org.gnu.gdb.aarch64.pauth} could crash
> >    older versions of @value{GDBN} due to a known bug in those versions.
> 
> I think the above is sane. It just has less detail, which, as explained above, might not be too useful for
> developers of debugging stubs.
> 
> > 
> > Is it really important to say that the bug happens only when
> > additional system registers are reported?
> 
> We could make it shorter by only mentioning this is only an issue when using emulators.

Fine by me, thanks.

Re: [PATCH] [gdb/aarch64] pauth: Create new feature string for pauth to prevent crashing older gdb's

[Peter Maydell]

On Wed, 12 Apr 2023 at 10:26, Luis Machado <luis.machado@arm.com> wrote:
> On 4/12/23 10:20, Eli Zaretskii wrote:
> > Is it really important to say that the bug happens only when
> > additional system registers are reported?
>
> We could make it shorter by only mentioning this is only an issue when using emulators.

It's not emulator-specific, though, is it, if I understand the bug right?
It will happen with any gdb-protocol-speaking stub that reports registers
in XML features that gdb doesn't care about, I think you said? It's just
that the case we know about happens to be with an emulator where the
extra registers reported are system registers.

thanks
-- PMM

Re: [PATCH] [gdb/aarch64] pauth: Create new feature string for pauth to prevent crashing older gdb's

[Luis Machado]

On 4/12/23 12:48, Peter Maydell wrote:
> On Wed, 12 Apr 2023 at 10:26, Luis Machado <luis.machado@arm.com> wrote:
>> On 4/12/23 10:20, Eli Zaretskii wrote:
>>> Is it really important to say that the bug happens only when
>>> additional system registers are reported?
>>
>> We could make it shorter by only mentioning this is only an issue when using emulators.
> 
> It's not emulator-specific, though, is it, if I understand the bug right?
> It will happen with any gdb-protocol-speaking stub that reports registers
> in XML features that gdb doesn't care about, I think you said? It's just
> that the case we know about happens to be with an emulator where the
> extra registers reported are system registers.

Yes, that's true. That was oversimplifying it. Emulators would be the most common case at the moment, like QEMU users
attempting to use Pointer Authentication support.

It could be potentially anything, as I described before. Even a newer gdbserver talking to an old gdb, if the new gdbserver
sends additional registers the old gdb doesn't care about.

Maybe I should expand on that explanation as well.

> 
> thanks
> -- PMM

[PATCH,v2] [gdb/aarch64] pauth: Create new feature string for pauth to prevent crashing older gdb's

[Luis Machado]

Updates since v1:

- Adjusted the documentation flow and details.

Older gdb's (9, 10, 11 and 12) have a bug that causes them to crash whenever
a target reports the pauth feature string in the target description and also
provide additional register outside of gdb's known and expected feature
strings.

This was fixed in gdb 13 onwards, but that means we're stuck with gdb's out
there that will crash on connection to the above targets.

QEMU has postponed inclusion of the pauth feature string in version 8, and
instead we agreed to use a new feature name to prevent crashing those older
gdb's.

Initially there was a plan to backport a trivial fix all the way to gdb 9, but
given QEMU's choice, this is no longer needed.

This new feature string is org.gnu.gdb.aarch64.pauth_v2, and should be used
by all targets going forward, except native linux gdb and gdbserver, for
backwards compatibility with older gdb's/gdbserver's.

gdb/gdbserver will still emit the old feature string for Linux since it doesn't
report additional system registers and thus doesn't cause a crash of older
gdb's. We can revisit this in the future once the problematic gdb's are likely
no longer in use.

I've added some documentation to explain the situation.
---
 gdb/aarch64-tdep.c  | 15 ++++++++++++++-
 gdb/doc/gdb.texinfo | 24 ++++++++++++++++++++++++
 2 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index d8349e4ccdb..ec0e51bdaf7 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -3500,8 +3500,15 @@ aarch64_features_from_target_desc (const struct target_desc *tdesc)
     return features;
 
   features.vq = aarch64_get_tdesc_vq (tdesc);
+
+  /* We need to look for a couple pauth feature name variations.  */
   features.pauth
       = (tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.pauth") != nullptr);
+
+  if (!features.pauth)
+    features.pauth = (tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.pauth_v2")
+		      != nullptr);
+
   features.mte
       = (tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.mte") != nullptr);
 
@@ -3679,7 +3686,6 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   feature_core = tdesc_find_feature (tdesc,"org.gnu.gdb.aarch64.core");
   feature_fpu = tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.fpu");
   feature_sve = tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.sve");
-  feature_pauth = tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.pauth");
   const struct tdesc_feature *feature_mte
     = tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.mte");
   const struct tdesc_feature *feature_tls
@@ -3773,6 +3779,13 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
 	}
     }
 
+  /* We have two versions of the pauth target description due to a past bug
+     where GDB would crash when seeing the first version of the pauth target
+     description.  */
+  feature_pauth = tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.pauth");
+  if (feature_pauth == nullptr)
+    feature_pauth = tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.pauth_v2");
+
   /* Add the pauth registers.  */
   int pauth_masks = 0;
   if (feature_pauth != NULL)
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 45a0580bc29..83cb392416a 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -47980,6 +47980,30 @@ has a signed link register value that needs to be unmasked/decoded.
 Extra registers are allowed in this feature, but they will not affect
 @value{GDBN}.
 
+In the future the @samp{org.gnu.gdb.aarch64.pauth} feature string might be
+deprecated in favor of feature string @samp{org.gnu.gdb.aarch64.pauth_v2}.
+
+The @samp{org.gnu.gdb.aarch64.pauth_v2} feature has the exact same contents
+as feature @samp{org.gnu.gdb.aarch64.pauth}.
+
+Targets reporting Pointer Authentication support via the remote protocol
+should always use the newer feature string
+@samp{org.gnu.gdb.aarch64.pauth_v2}.
+
+The reason for having feature @samp{org.gnu.gdb.aarch64.pauth_v2} is a bug in
+previous versions of @value{GDBN} (versions 9, 10, 11 and 12).  This bug
+caused @value{GDBN} to crash whenever the target reported support for Pointer
+Authentication (using feature string @samp{org.gnu.gdb.aarch64.pauth}) and also
+reported additional system registers that were not accounted for by
+@value{GDBN}.  This is more common when using emulators and on bare-metal
+debugging scenarios.
+
+It can also happen if a newer gdbserver is used with an old @value{GDBN} that
+has the bug.  In such a case, the newer gdbserver might report Pointer
+Authentication support via the @samp{org.gnu.gdb.aarch64.pauth} feature string
+and also report additional registers the older @value{GDBN} does not know
+about, potentially leading to a crash.
+
 @subsubsection AArch64 TLS registers feature
 
 The @samp{org.gnu.gdb.aarch64.tls} optional feature was introduced to expose
-- 
2.25.1

Re: [PATCH,v2] [gdb/aarch64] pauth: Create new feature string for pauth to prevent crashing older gdb's

[Eli Zaretskii]

> From: Luis Machado <luis.machado@arm.com>
> CC: <alex.bennee@linaro.org>, <richard.henderson@linaro.org>,
> 	<peter.maydell@linaro.org>, <eliz@gnu.org>
> Date: Wed, 12 Apr 2023 13:27:22 +0100
> 
>  gdb/aarch64-tdep.c  | 15 ++++++++++++++-
>  gdb/doc/gdb.texinfo | 24 ++++++++++++++++++++++++
>  2 files changed, 38 insertions(+), 1 deletion(-)

Thanks, the documentation part is okay.

Reviewed-By: Eli Zaretskii <eliz@gnu.org>

Re: [PATCH] [gdb/aarch64] pauth: Create new feature string for pauth to prevent crashing older gdb's

[Tom Tromey]

>>>>> "Luis" == Luis Machado via Gdb-patches <gdb-patches@sourceware.org> writes:

Luis> Though it is documentation for users, it is also one of the only
Luis> documents providing guidance for developers of debugging stubs
Luis> (like QEMU's, openOCD etc).

Luis> So I feel like it should include a bit more information rather
Luis> than the bare minimum.

FWIW, I concur -- in the past I've wished that the documentation about
target XML features had been more complete.

Tom

Re: [PATCH, v2] [gdb/aarch64] pauth: Create new feature string for pauth to prevent crashing older gdb's

[Tom Tromey]

>>>>> "Luis" == Luis Machado via Gdb-patches <gdb-patches@sourceware.org> writes:

Luis> +In the future the @samp{org.gnu.gdb.aarch64.pauth} feature string might be
Luis> +deprecated in favor of feature string @samp{org.gnu.gdb.aarch64.pauth_v2}.

Is there a particular reason to say "might be", rather than just
deprecate it directly?  Like would gdb ever go back to the non _v2
feature name?

Tom

Re: [PATCH, v2] [gdb/aarch64] pauth: Create new feature string for pauth to prevent crashing older gdb's

[Luis Machado]

On 4/13/23 14:56, Tom Tromey wrote:
>>>>>> "Luis" == Luis Machado via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Luis> +In the future the @samp{org.gnu.gdb.aarch64.pauth} feature string might be
> Luis> +deprecated in favor of feature string @samp{org.gnu.gdb.aarch64.pauth_v2}.
> 
> Is there a particular reason to say "might be", rather than just
> deprecate it directly?  Like would gdb ever go back to the non _v2
> feature name?
Not really. I was going for flexibility. But in this case, we won't go back to using the
old feature string. We just need to use the old one for backwards compatibility while we
wait for older broken gdb's to vanish.

I'll update it.

[PATCH, v3] [gdb/aarch64] pauth: Create new feature string for pauth to prevent crashing older gdb's

[Luis Machado]

Updates since v2:

- Added NEWS entry.
- Rewrote deprecation paragraph.

Updates since v1:

- Adjusted the documentation flow and details.

Older gdb's (9, 10, 11 and 12) have a bug that causes them to crash whenever
a target reports the pauth feature string in the target description and also
provide additional register outside of gdb's known and expected feature
strings.

This was fixed in gdb 13 onwards, but that means we're stuck with gdb's out
there that will crash on connection to the above targets.

QEMU has postponed inclusion of the pauth feature string in version 8, and
instead we agreed to use a new feature name to prevent crashing those older
gdb's.

Initially there was a plan to backport a trivial fix all the way to gdb 9, but
given QEMU's choice, this is no longer needed.

This new feature string is org.gnu.gdb.aarch64.pauth_v2, and should be used
by all targets going forward, except native linux gdb and gdbserver, for
backwards compatibility with older gdb's/gdbserver's.

gdb/gdbserver will still emit the old feature string for Linux since it doesn't
report additional system registers and thus doesn't cause a crash of older
gdb's. We can revisit this in the future once the problematic gdb's are likely
no longer in use.

I've added some documentation to explain the situation.
---
 gdb/NEWS            |  4 ++++
 gdb/aarch64-tdep.c  | 15 ++++++++++++++-
 gdb/doc/gdb.texinfo | 23 +++++++++++++++++++++++
 3 files changed, 41 insertions(+), 1 deletion(-)

diff --git a/gdb/NEWS b/gdb/NEWS
index 10a1a70fa52..54b5da21245 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -3,6 +3,10 @@
 
 *** Changes since GDB 13
 
+* The AArch64 'org.gnu.gdb.aarch64.pauth' Pointer Authentication feature string
+  has been deprecated in favor of the 'org.gnu.gdb.aarch64.pauth_v2' feature
+  string.
+
 * GDB now has some support for integer types larger than 64 bits.
 
 * Removed targets and native configurations
diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index d8349e4ccdb..ec0e51bdaf7 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -3500,8 +3500,15 @@ aarch64_features_from_target_desc (const struct target_desc *tdesc)
     return features;
 
   features.vq = aarch64_get_tdesc_vq (tdesc);
+
+  /* We need to look for a couple pauth feature name variations.  */
   features.pauth
       = (tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.pauth") != nullptr);
+
+  if (!features.pauth)
+    features.pauth = (tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.pauth_v2")
+		      != nullptr);
+
   features.mte
       = (tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.mte") != nullptr);
 
@@ -3679,7 +3686,6 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   feature_core = tdesc_find_feature (tdesc,"org.gnu.gdb.aarch64.core");
   feature_fpu = tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.fpu");
   feature_sve = tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.sve");
-  feature_pauth = tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.pauth");
   const struct tdesc_feature *feature_mte
     = tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.mte");
   const struct tdesc_feature *feature_tls
@@ -3773,6 +3779,13 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
 	}
     }
 
+  /* We have two versions of the pauth target description due to a past bug
+     where GDB would crash when seeing the first version of the pauth target
+     description.  */
+  feature_pauth = tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.pauth");
+  if (feature_pauth == nullptr)
+    feature_pauth = tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.pauth_v2");
+
   /* Add the pauth registers.  */
   int pauth_masks = 0;
   if (feature_pauth != NULL)
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index c11457952d2..940315b2713 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -47991,6 +47991,29 @@ has a signed link register value that needs to be unmasked/decoded.
 Extra registers are allowed in this feature, but they will not affect
 @value{GDBN}.
 
+Please note the @samp{org.gnu.gdb.aarch64.pauth} feature string is deprecated
+and must only be used for backwards compatibility with older releases of
+@value{GDBN} and @command{gdbserver}.  Targets that support Pointer
+Authentication must advertise such capability by using the
+@samp{org.gnu.gdb.aarch64.pauth_v2} feature string instead.
+
+The @samp{org.gnu.gdb.aarch64.pauth_v2} feature has the exact same contents
+as feature @samp{org.gnu.gdb.aarch64.pauth}.
+
+The reason for having feature @samp{org.gnu.gdb.aarch64.pauth_v2} is a bug in
+previous versions of @value{GDBN} (versions 9, 10, 11 and 12).  This bug
+caused @value{GDBN} to crash whenever the target reported support for Pointer
+Authentication (using feature string @samp{org.gnu.gdb.aarch64.pauth}) and also
+reported additional system registers that were not accounted for by
+@value{GDBN}.  This is more common when using emulators and on bare-metal
+debugging scenarios.
+
+It can also happen if a newer gdbserver is used with an old @value{GDBN} that
+has the bug.  In such a case, the newer gdbserver might report Pointer
+Authentication support via the @samp{org.gnu.gdb.aarch64.pauth} feature string
+and also report additional registers the older @value{GDBN} does not know
+about, potentially leading to a crash.
+
 @subsubsection AArch64 TLS registers feature
 
 The @samp{org.gnu.gdb.aarch64.tls} optional feature was introduced to expose
-- 
2.25.1

Re: [PATCH, v3] [gdb/aarch64] pauth: Create new feature string for pauth to prevent crashing older gdb's

[Tom Tromey]

>>>>> "Luis" == Luis Machado <luis.machado@arm.com> writes:

Luis> Updates since v2:
Luis> - Added NEWS entry.
Luis> - Rewrote deprecation paragraph.

Thank you, this addresses my comment.

Tom

Re: [PATCH, v3] [gdb/aarch64] pauth: Create new feature string for pauth to prevent crashing older gdb's

[Luis Machado]

On 4/13/23 18:17, Tom Tromey wrote:
>>>>>> "Luis" == Luis Machado <luis.machado@arm.com> writes:
> 
> Luis> Updates since v2:
> Luis> - Added NEWS entry.
> Luis> - Rewrote deprecation paragraph.
> 
> Thank you, this addresses my comment.
> 
> Tom

Thanks. I've pushed this version now.