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

Message ID 20230413143743.37947-1-luis.machado@arm.com
State New
Headers
Series [v3,gdb/aarch64] pauth: Create new feature string for pauth to prevent crashing older gdb's |

Commit Message

Luis Machado April 13, 2023, 2:37 p.m. UTC
  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(-)
  

Comments

Tom Tromey April 13, 2023, 5:17 p.m. UTC | #1
>>>>> "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
  
Luis Machado April 14, 2023, 12:45 p.m. UTC | #2
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.
  

Patch

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