[V2] AArch64 pauth: Indicate unmasked addresses in backtrace

Message ID C7883044-EBB8-462D-963C-6741AC186444@arm.com
State New, archived
Headers

Commit Message

Alan Hayward Aug. 7, 2019, 12:35 p.m. UTC
  > On 6 Aug 2019, at 17:15, Tom Tromey <tom@tromey.com> wrote:
> 
>>>>>> "Alan" == Alan Hayward <Alan.Hayward@arm.com> writes:
> 
> Alan> Armv8.3-a Pointer Authentication causes the function return address to be
> Alan> obfuscated on entry to some functions. GDB must unmask the link register in
> Alan> order to produce a backtrace.
> 
> Alan> The following patch adds markers of [PAC] to the bracktrace, to indicate
> Alan> which addresses needed unmasking.  This includes the backtrace when using MI.
> 
> Thanks for the patch.  I don't think this was re-reviewed?

It hadn’t been - so thanks!

> 
> Alan> --- a/gdb/frame.c
> Alan> +++ b/gdb/frame.c
> Alan> @@ -124,6 +124,8 @@ struct frame_info
> Alan>    struct {
> Alan>      enum cached_copy_status status;
> Alan>      CORE_ADDR value;
> Alan> +    /* Did VALUE require unmasking when being read.  */
> Alan> +    bool masked;
> Alan>    } prev_pc;
> 
> I think putting the new field between "status" and "value" will pack the
> structure a bit better.

It doesn’t look as nice when switched, but agreed and done.

> 
> Alan> +bool
> Alan> +get_frame_pc_masked (struct frame_info *frame)
> 
> Maybe make the parameter "const"?  I see there isn't much use of const
> in frame.h, but on the other hand, that's probably just historical and
> not required.
> 

Done.

> Otherwise the patch looks fine to me.  It is ok.  I'd prefer those
> changes but if you'd prefer not, a note to that effect is fine.
> 
> thanks,
> Tom

Patch pushed and posted below for reference.


Alan.
  

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 82e0819c13..bacf5d7636 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,18 @@ 
+2019-08-07  Alan Hayward  <alan.hayward@arm.com>
+
+       * NEWS: Expand the Pointer Authentication entry.
+       * aarch64-tdep.c (aarch64_frame_unmask_address): Rename from this.
+       (aarch64_frame_unmask_lr): ... to this.
+       (aarch64_prologue_prev_register, aarch64_dwarf2_prev_register):
+       Call aarch64_frame_unmask_lr.
+       * frame.c (struct frame_info): Add "masked" variable.
+       (frame_set_previous_pc_masked) (frame_get_pc_masked): New functions.
+       (fprint_frame): Check for masked pc.
+       * frame.h (frame_set_previous_pc_masked) (frame_get_pc_masked): New
+       declarations.
+       * python/py-framefilter.c (py_print_frame): Check for masked pc.
+       * stack.c (print_frame): Check for masked pc.
+
 2019-08-06  Tom Tromey  <tom@tromey.com>

        * stabsread.c (patch_block_stabs, read_one_struct_field)
diff --git a/gdb/NEWS b/gdb/NEWS
index b4c59e4410..fa01adf6e8 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -16,7 +16,9 @@ 
   architectures require kernel changes.  TLS is not yet supported for
   amd64 and i386 process core dumps.

-* Support for Pointer Authentication on AArch64 Linux.
+* Support for Pointer Authentication (PAC) on AArch64 Linux.  Return
+  addresses that required unmasking are shown in the backtrace with the
+  postfix [PAC].

 * Two new convenience functions $_cimag and $_creal that extract the
   imaginary and real parts respectively from complex numbers.
diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index e23cc3f956..9b6324f0fc 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -250,13 +250,13 @@  class instruction_reader : public abstract_instruction_reader

 } // namespace

-/* If address signing is enabled, mask off the signature bits from ADDR, using
-   the register values in THIS_FRAME.  */
+/* If address signing is enabled, mask off the signature bits from the link
+   register, which is passed by value in ADDR, using the register values in
+   THIS_FRAME.  */

 static CORE_ADDR
-aarch64_frame_unmask_address (struct gdbarch_tdep *tdep,
-                             struct frame_info *this_frame,
-                             CORE_ADDR addr)
+aarch64_frame_unmask_lr (struct gdbarch_tdep *tdep,
+                        struct frame_info *this_frame, CORE_ADDR addr)
 {
   if (tdep->has_pauth ()
       && frame_unwind_register_unsigned (this_frame,
@@ -265,6 +265,9 @@  aarch64_frame_unmask_address (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;
+
+      /* Record in the frame that the link register required unmasking.  */
+      set_frame_previous_pc_masked (this_frame);
     }

   return addr;
@@ -952,7 +955,7 @@  aarch64_prologue_prev_register (struct frame_info *this_frame,
       if (tdep->has_pauth ()
          && trad_frame_value_p (cache->saved_regs,
                                 tdep->pauth_ra_state_regnum))
-       lr = aarch64_frame_unmask_address (tdep, this_frame, lr);
+       lr = aarch64_frame_unmask_lr (tdep, this_frame, lr);

       return frame_unwind_got_constant (this_frame, prev_regnum, lr);
     }
@@ -1119,7 +1122,7 @@  aarch64_dwarf2_prev_register (struct frame_info *this_frame,
     {
     case AARCH64_PC_REGNUM:
       lr = frame_unwind_register_unsigned (this_frame, AARCH64_LR_REGNUM);
-      lr = aarch64_frame_unmask_address (tdep, this_frame, lr);
+      lr = aarch64_frame_unmask_lr (tdep, this_frame, lr);
       return frame_unwind_got_constant (this_frame, regnum, lr);

     default:
diff --git a/gdb/doc/ChangeLog b/gdb/doc/ChangeLog
index 6faf5f359d..702bb7c7a0 100644
--- a/gdb/doc/ChangeLog
+++ b/gdb/doc/ChangeLog
@@ -1,3 +1,7 @@ 
+2019-08-07  Alan Hayward  <alan.hayward@arm.com>
+
+       * gdb.texinfo (AArch64 Pointer Authentication): New subsection.
+
 2019-08-05  Christian Biesinger  <cbiesinger@google.com>

        * python.texi (Blocks In Python): Document dictionary access on blocks.
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 89b1eda2c1..7f8c0aff1c 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -24390,6 +24390,14 @@  but the lengths of the @code{z} and @code{p} registers will not change.  This
 is a known limitation of @value{GDBN} and does not affect the execution of the
 target process.

+@subsubsection AArch64 Pointer Authentication.
+@cindex AArch64 Pointer Authentication.
+
+When @value{GDBN} is debugging the AArch64 architecture, and the program is
+using the v8.3-A feature Pointer Authentication (PAC), then whenever the link
+register @code{$lr} is pointing to an PAC function it's value will be masked.
+When GDB prints a backtrace, any addresses that required unmasking will be
+postfixed with the marker [PAC].

 @node i386
 @subsection x86 Architecture-specific Issues
diff --git a/gdb/frame.c b/gdb/frame.c
index adac24f68c..b62fd5cd85 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -123,6 +123,8 @@  struct frame_info
   /* Cached copy of the previous frame's resume address.  */
   struct {
     enum cached_copy_status status;
+    /* Did VALUE require unmasking when being read.  */
+    bool masked;
     CORE_ADDR value;
   } prev_pc;

@@ -161,6 +163,25 @@  struct frame_info
   const char *stop_string;
 };

+/* See frame.h.  */
+
+void
+set_frame_previous_pc_masked (struct frame_info *frame)
+{
+  frame->prev_pc.masked = true;
+}
+
+/* See frame.h.  */
+
+bool
+get_frame_pc_masked (const struct frame_info *frame)
+{
+  gdb_assert (frame->next != nullptr);
+  gdb_assert (frame->next->prev_pc.status == CC_VALUE);
+
+  return frame->next->prev_pc.masked;
+}
+
 /* A frame stash used to speed up frame lookups.  Create a hash table
    to stash frames previously accessed from the frame cache for
    quicker subsequent retrieval.  The hash table is emptied whenever
@@ -429,8 +450,11 @@  fprint_frame (struct ui_file *file, struct frame_info *fi)
   if (fi->next == NULL || fi->next->prev_pc.status == CC_UNKNOWN)
     fprintf_unfiltered (file, "<unknown>");
   else if (fi->next->prev_pc.status == CC_VALUE)
-    fprintf_unfiltered (file, "%s",
-                       hex_string (fi->next->prev_pc.value));
+    {
+      fprintf_unfiltered (file, "%s", hex_string (fi->next->prev_pc.value));
+      if (fi->next->prev_pc.masked)
+       fprintf_unfiltered (file, "[PAC]");
+    }
   else if (fi->next->prev_pc.status == CC_NOT_SAVED)
     val_print_not_saved (file);
   else if (fi->next->prev_pc.status == CC_UNAVAILABLE)
diff --git a/gdb/frame.h b/gdb/frame.h
index ccc285005a..fdb401d84f 100644
--- a/gdb/frame.h
+++ b/gdb/frame.h
@@ -949,4 +949,13 @@  extern const gdb::option::option_def set_backtrace_option_defs[2];
 /* The values behind the global "set backtrace ..." settings.  */
 extern set_backtrace_options user_set_backtrace_options;

+/* Mark that the PC value is masked for the previous frame.  */
+
+extern void set_frame_previous_pc_masked (struct frame_info *frame);
+
+/* Get whether the PC value is masked for the given frame.  */
+
+extern bool get_frame_pc_masked (const struct frame_info *frame);
+
+
 #endif /* !defined (FRAME_H)  */
diff --git a/gdb/python/py-framefilter.c b/gdb/python/py-framefilter.c
index a2a96ac0d3..d805ec68f2 100644
--- a/gdb/python/py-framefilter.c
+++ b/gdb/python/py-framefilter.c
@@ -901,6 +901,8 @@  py_print_frame (PyObject *filter, frame_filter_flags flags,
            {
              annotate_frame_address ();
              out->field_core_addr ("addr", gdbarch, address);
+             if (get_frame_pc_masked (frame))
+               out->field_string ("pac", " [PAC]");
              annotate_frame_address_end ();
              out->text (" in ");
            }
diff --git a/gdb/stack.c b/gdb/stack.c
index c68b3876d3..0859815baf 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -1298,7 +1298,11 @@  print_frame (const frame_print_options &fp_opts,
        {
          annotate_frame_address ();
          if (pc_p)
-           uiout->field_core_addr ("addr", gdbarch, pc);
+           {
+             uiout->field_core_addr ("addr", gdbarch, pc);
+             if (get_frame_pc_masked (frame))
+               uiout->field_string ("pac", " [PAC]");
+           }
          else
            uiout->field_string ("addr", "<unavailable>",
                                 ui_out_style_kind::ADDRESS);