Patchwork [AArch64,6/6] Core file support for "pauth" feature

login
register
mail settings
Submitter Jiong Wang
Date Aug. 9, 2017, 12:23 p.m.
Message ID <e4493fb7-9792-1bda-0011-808ac36418ae@foss.arm.com>
Download mbox | patch
Permalink /patch/22040/
State New
Headers show

Comments

Jiong Wang - Aug. 9, 2017, 12:23 p.m.
This patch add coredump support for the new "pauth" feature.  A few necessary
low level methods and descriptions are added accordingly.

2017-08-09  Jiong Wang  <jiong.wang@arm.com>
bfd/
	* elf-bfd.h (elfcore_write_aarch_pauth): New function declaration.
	* elf.c (elfcore_grok_aarch_pauth): New function.
	(elfcore_grok_note): Handle NT_ARM_PAUTH.
	(elfcore_write_aarch_pauth): New function.
	(elfcore_write_register_note): Handle ".reg-aarch-pauth" section.

gdb/
	* aarch64-linux-tdep.c: #include "auxv.h".
	#include "elf/common.h"
	(aarch64_collect_pauth_regset): New function.
	(aarch64_supply_pauth_regset): New function.
	(aarch64_linux_pauthregset): New regset.
	(aarch64_linux_iterate_over_regset_sections): Support
	".reg-aarch64-pauth" section.
	(aarch64_linux_core_read_description): New function.
	(aarch64_linux_init_abi): Register core_read_description method.
	* aarch64-linux-tdep.h (AARCH64_LINUX_SIZEOF_PAUTH): New define.
	(HWCAP_APIA): New define.
	(aarch64_linux_pauthregset): New variable declaration.
Nick Clifton - Aug. 9, 2017, 4:49 p.m.
Hi Jiong,

> 2017-08-09  Jiong Wang  <jiong.wang@arm.com>
> bfd/
>      * elf-bfd.h (elfcore_write_aarch_pauth): New function declaration.
>      * elf.c (elfcore_grok_aarch_pauth): New function.
>      (elfcore_grok_note): Handle NT_ARM_PAUTH.
>      (elfcore_write_aarch_pauth): New function.
>      (elfcore_write_register_note): Handle ".reg-aarch-pauth" section.

This part of the patch is approved.

Cheers
  Nick
Pedro Alves - Aug. 10, 2017, 11:08 a.m.
On 08/09/2017 01:23 PM, Jiong Wang wrote:

>  /* Implementation of `gdbarch_stap_is_single_operand', as defined in
> diff --git a/gdb/aarch64-linux-tdep.h b/gdb/aarch64-linux-tdep.h
> index d0f9b12..580a8b8 100644
> --- a/gdb/aarch64-linux-tdep.h
> +++ b/gdb/aarch64-linux-tdep.h
> @@ -29,6 +29,14 @@
>     are 4 bytes wide each, and the whole structure is padded to 128 bit
>     alignment.  */
>  #define AARCH64_LINUX_SIZEOF_FPREGSET (33 * V_REGISTER_SIZE)
> +#define AARCH64_LINUX_SIZEOF_PAUTH (2 * X_REGISTER_SIZE)
>  
>  extern const struct regset aarch64_linux_gregset;
>  extern const struct regset aarch64_linux_fpregset;
> +extern const struct regset aarch64_linux_pauthregset;
> +
> +#ifndef HWCAP_APIA
> +/* AArch64 GNU/Linux HWCAP values.  These should be synced with kernel
> +   definitions.  */
> +#define HWCAP_APIA (1 << 16)
> +#endif

Re. the #ifndef, consider that tdep.h files are included in cross
debugger builds.  E.g., an x86-hosted gdb cross debugging aarch64.
Some archs have "namespaced" names like the s390 mips, sparc, etc.
(e.g., HWCAP_S390_VX) which avoids the case of the names being defined
on host/target with a different meanings/values, but not all do.
But even with such names, we always have to provide fallback definitions
for cross debuggers.  And with that all in mind, and since you're defining
fallbacks anyway, how about unconditionally defining/using our
own conflict-resistant versions, like AARCH64_HWCAP_APIA?

Thanks,
Pedro Alves
Yao Qi - Aug. 10, 2017, 9:22 p.m.
On 17-08-10 12:08:36, Pedro Alves wrote:
> > +#ifndef HWCAP_APIA
> > +/* AArch64 GNU/Linux HWCAP values.  These should be synced with kernel
> > +   definitions.  */
> > +#define HWCAP_APIA (1 << 16)
> > +#endif
> 
> Re. the #ifndef, consider that tdep.h files are included in cross
> debugger builds.  E.g., an x86-hosted gdb cross debugging aarch64.
> Some archs have "namespaced" names like the s390 mips, sparc, etc.
> (e.g., HWCAP_S390_VX) which avoids the case of the names being defined
> on host/target with a different meanings/values, but not all do.
> But even with such names, we always have to provide fallback definitions
> for cross debuggers.  And with that all in mind, and since you're defining
> fallbacks anyway, how about unconditionally defining/using our
> own conflict-resistant versions, like AARCH64_HWCAP_APIA?
> 

I am inclined to use the same macro name as kernel uses.  These macros are
only used in $arch-linux-{tdep,nat}.c, so it is clear that the macros
are about architecture $arch.
Pedro Alves - Aug. 10, 2017, 9:32 p.m.
On 08/10/2017 10:22 PM, Yao Qi wrote:
> On 17-08-10 12:08:36, Pedro Alves wrote:
>>> +#ifndef HWCAP_APIA
>>> +/* AArch64 GNU/Linux HWCAP values.  These should be synced with kernel
>>> +   definitions.  */
>>> +#define HWCAP_APIA (1 << 16)
>>> +#endif
>>
>> Re. the #ifndef, consider that tdep.h files are included in cross
>> debugger builds.  E.g., an x86-hosted gdb cross debugging aarch64.
>> Some archs have "namespaced" names like the s390 mips, sparc, etc.
>> (e.g., HWCAP_S390_VX) which avoids the case of the names being defined
>> on host/target with a different meanings/values, but not all do.
>> But even with such names, we always have to provide fallback definitions
>> for cross debuggers.  And with that all in mind, and since you're defining
>> fallbacks anyway, how about unconditionally defining/using our
>> own conflict-resistant versions, like AARCH64_HWCAP_APIA?
>>
> 
> I am inclined to use the same macro name as kernel uses.  These macros are
> only used in $arch-linux-{tdep,nat}.c, so it is clear that the macros
> are about architecture $arch.

I think there's a misunderstanding.  It's not about clarity -- if HWCAP_APIA
is defined on a !Aarch64 host as some value other than "(1 << 16)", then
this:

> +++ b/gdb/aarch64-linux-tdep.c
>  
> -  return tdesc_aarch64;
> +  return aarch64_hwcap & HWCAP_APIA ? tdesc_aarch64_pauth : tdesc_aarch64;
>  }

will silently compile to use wrong value.

Might never happen in practice, but why write a potential problem,
_particularly since you already have to write the fallback
macro anyway_?  What's the advantage of not doing what I suggested?

It'd be different if the macro was _only_ used in a -nat.c
file, but then I'd object to defining it in the -tdep.h file.

Thanks,
Pedro Alves
Yao Qi - Aug. 10, 2017, 10:04 p.m.
On 17-08-10 22:32:40, Pedro Alves wrote:
> 
> I think there's a misunderstanding.  It's not about clarity -- if HWCAP_APIA
> is defined on a !Aarch64 host as some value other than "(1 << 16)", then
> this:
> 
> > +++ b/gdb/aarch64-linux-tdep.c
> >  
> > -  return tdesc_aarch64;
> > +  return aarch64_hwcap & HWCAP_APIA ? tdesc_aarch64_pauth : tdesc_aarch64;
> >  }
> 
> will silently compile to use wrong value.
> 
> Might never happen in practice, but why write a potential problem,
> _particularly since you already have to write the fallback
> macro anyway_?  What's the advantage of not doing what I suggested?

I want the macro name sync'ed with kernel.  However I understand your
point.  Let me see if I can ask kernel people change it to HWCAP_ARM64_APIA,
or something else.  Kernel patches are not merged yet, as far I as
know.
Yao Qi - Aug. 11, 2017, 3:38 p.m.
On 17-08-10 22:04:02, Yao Qi wrote:
> 
> I want the macro name sync'ed with kernel.  However I understand your
> point.  Let me see if I can ask kernel people change it to HWCAP_ARM64_APIA,
> or something else.  Kernel patches are not merged yet, as far I as
> know.

Kernel people don't want to change the naming scheme for new hwcaps.
http://lists.infradead.org/pipermail/linux-arm-kernel/2017-August/525204.html
How about we unconditionally define HWCAP_ARM64_APIA in aarch64-linux-tdep.c?
WDYT?

#define HWCAP_ARM64_APIA (1 << 16)
Pedro Alves - Aug. 11, 2017, 3:55 p.m.
On 08/11/2017 04:38 PM, Yao Qi wrote:
> On 17-08-10 22:04:02, Yao Qi wrote:

> How about we unconditionally define HWCAP_ARM64_APIA in aarch64-linux-tdep.c?
> WDYT?
> 
> #define HWCAP_ARM64_APIA (1 << 16)
> 

Agreed, because that's essentially what I was suggesting.  :-)

The difference was that I was suggesting 'AARCH64_ + $org_name', to
follow the scheme of all other GDB-invented Aarch64-related macro symbols.
AARCH64_HWCAP_APIA seems more clearly to me a gdb-replacement name,
than HWCAP_ARM64_APIA.  I could totally see someone confusing the latter
for a real kernel-defined name, since it follows the scheme of
other ports.  But that's obviously a minor thing.

Thanks,
Pedro Alves

Patch

diff --git a/bfd/elf-bfd.h b/bfd/elf-bfd.h
index 92a8e02..d9b61ba 100644
--- a/bfd/elf-bfd.h
+++ b/bfd/elf-bfd.h
@@ -2539,6 +2539,8 @@  extern char *elfcore_write_s390_gs_bc
   (bfd *, char *, int *, const void *, int);
 extern char *elfcore_write_arm_vfp
   (bfd *, char *, int *, const void *, int);
+extern char *elfcore_write_aarch_pauth
+  (bfd *, char *, int *, const void *, int);
 extern char *elfcore_write_aarch_tls
   (bfd *, char *, int *, const void *, int);
 extern char *elfcore_write_aarch_hw_break
diff --git a/bfd/elf.c b/bfd/elf.c
index b99e297..30772cf 100644
--- a/bfd/elf.c
+++ b/bfd/elf.c
@@ -9207,6 +9207,12 @@  elfcore_grok_arm_vfp (bfd *abfd, Elf_Internal_Note *note)
 }
 
 static bfd_boolean
+elfcore_grok_aarch_pauth (bfd *abfd, Elf_Internal_Note *note)
+{
+  return elfcore_make_note_pseudosection (abfd, ".reg-aarch-pauth", note);
+}
+
+static bfd_boolean
 elfcore_grok_aarch_tls (bfd *abfd, Elf_Internal_Note *note)
 {
   return elfcore_make_note_pseudosection (abfd, ".reg-aarch-tls", note);
@@ -9699,6 +9705,13 @@  elfcore_grok_note (bfd *abfd, Elf_Internal_Note *note)
       else
 	return TRUE;
 
+    case NT_ARM_PAC_MASK:
+      if (note->namesz == 6
+	  && strcmp (note->namedata, "LINUX") == 0)
+	return elfcore_grok_aarch_pauth (abfd, note);
+      else
+	return TRUE;
+
     case NT_ARM_TLS:
       if (note->namesz == 6
 	  && strcmp (note->namedata, "LINUX") == 0)
@@ -10794,6 +10807,18 @@  elfcore_write_arm_vfp (bfd *abfd,
 }
 
 char *
+elfcore_write_aarch_pauth (bfd *abfd,
+			   char *buf,
+			   int *bufsiz,
+			   const void *aarch_pauth,
+			   int size)
+{
+  char *note_name = "LINUX";
+  return elfcore_write_note (abfd, buf, bufsiz,
+			     note_name, NT_ARM_PAC_MASK, aarch_pauth, size);
+}
+
+char *
 elfcore_write_aarch_tls (bfd *abfd,
 		       char *buf,
 		       int *bufsiz,
@@ -10875,6 +10900,8 @@  elfcore_write_register_note (bfd *abfd,
     return elfcore_write_s390_gs_bc (abfd, buf, bufsiz, data, size);
   if (strcmp (section, ".reg-arm-vfp") == 0)
     return elfcore_write_arm_vfp (abfd, buf, bufsiz, data, size);
+  if (strcmp (section, ".reg-aarch-pauth") == 0)
+    return elfcore_write_aarch_pauth (abfd, buf, bufsiz, data, size);
   if (strcmp (section, ".reg-aarch-tls") == 0)
     return elfcore_write_aarch_tls (abfd, buf, bufsiz, data, size);
   if (strcmp (section, ".reg-aarch-hw-break") == 0)
diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
index ec6125a..56ca1ce 100644
--- a/gdb/aarch64-linux-tdep.c
+++ b/gdb/aarch64-linux-tdep.c
@@ -45,6 +45,8 @@ 
 
 #include "record-full.h"
 #include "linux-record.h"
+#include "auxv.h"
+#include "elf/common.h"
 
 /* Signal frame handling.
 
@@ -217,6 +219,65 @@  const struct regset aarch64_linux_fpregset =
     regcache_supply_regset, regcache_collect_regset
   };
 
+/* Collect register REGNUM from REGCACHE and store its contents in the buffer
+   PAUTH_REGS which is of SIZE.  REGSET is not used as for PAUTH regset the
+   register number inside GDB is dynamicly allocated so we are not using
+   register map in REGSET.  */
+
+static void
+aarch64_collect_pauth_regset (const struct regset *regset,
+			      const struct regcache *regcache,
+			      int regnum, void *pauth_regs, size_t size)
+{
+  struct gdbarch *gdbarch = get_regcache_arch (regcache);
+  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
+
+  gdb_assert (size == AARCH64_LINUX_SIZEOF_PAUTH);
+  gdb_assert (regnum == -1);
+  gdb_assert (aarch64_tdep_has_pauth_p (tdep));
+
+  int dmask = tdep->regnum.pauth_reg_base;
+  int cmask = tdep->regnum.pauth_reg_base + 1;
+
+  memset (pauth_regs, 0, AARCH64_LINUX_SIZEOF_PAUTH);
+  regcache_raw_collect (regcache, dmask, pauth_regs);
+  regcache_raw_collect (regcache, cmask,
+			(gdb_byte *) pauth_regs + X_REGISTER_SIZE);
+
+  return;
+}
+
+/* Supply register REGNUM from the buffer PAUTH_REGS which is of SIZE to
+   REGCACHE.  REGSET is not used as for PAUTH regset the register number inside
+   GDB is dynamicly allocated so we are not using register map in REGSET.  */
+
+static void
+aarch64_supply_pauth_regset (const struct regset *regset,
+			     struct regcache *regcache,
+			     int regnum, const void *pauth_regs, size_t size)
+{
+  struct gdbarch *gdbarch = get_regcache_arch (regcache);
+  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
+
+  gdb_assert (size == AARCH64_LINUX_SIZEOF_PAUTH);
+  gdb_assert (regnum == -1);
+  gdb_assert (aarch64_tdep_has_pauth_p (tdep));
+
+  int dmask = tdep->regnum.pauth_reg_base;
+  int cmask = tdep->regnum.pauth_reg_base + 1;
+
+  regcache_raw_supply (regcache, dmask, pauth_regs);
+  regcache_raw_supply (regcache, cmask,
+		       (gdb_byte *) pauth_regs + X_REGISTER_SIZE);
+
+  return;
+}
+
+const struct regset aarch64_linux_pauthregset =
+  {
+    NULL, aarch64_supply_pauth_regset, aarch64_collect_pauth_regset
+  };
+
 /* Implement the "regset_from_core_section" gdbarch method.  */
 
 static void
@@ -225,10 +286,16 @@  aarch64_linux_iterate_over_regset_sections (struct gdbarch *gdbarch,
 					    void *cb_data,
 					    const struct regcache *regcache)
 {
+  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
+
   cb (".reg", AARCH64_LINUX_SIZEOF_GREGSET, &aarch64_linux_gregset,
       NULL, cb_data);
   cb (".reg2", AARCH64_LINUX_SIZEOF_FPREGSET, &aarch64_linux_fpregset,
       NULL, cb_data);
+
+  if (aarch64_tdep_has_pauth_p (tdep))
+    cb (".reg-aarch-pauth", AARCH64_LINUX_SIZEOF_PAUTH,
+	&aarch64_linux_pauthregset, NULL, cb_data);
 }
 
 /* Determine target description from core file.  */
@@ -242,7 +309,7 @@  aarch64_linux_core_read_description (struct gdbarch *gdbarch,
   if (target_auxv_search (target, AT_HWCAP, &aarch64_hwcap) != 1)
     return NULL;
 
-  return tdesc_aarch64;
+  return aarch64_hwcap & HWCAP_APIA ? tdesc_aarch64_pauth : tdesc_aarch64;
 }
 
 /* Implementation of `gdbarch_stap_is_single_operand', as defined in
diff --git a/gdb/aarch64-linux-tdep.h b/gdb/aarch64-linux-tdep.h
index d0f9b12..580a8b8 100644
--- a/gdb/aarch64-linux-tdep.h
+++ b/gdb/aarch64-linux-tdep.h
@@ -29,6 +29,14 @@ 
    are 4 bytes wide each, and the whole structure is padded to 128 bit
    alignment.  */
 #define AARCH64_LINUX_SIZEOF_FPREGSET (33 * V_REGISTER_SIZE)
+#define AARCH64_LINUX_SIZEOF_PAUTH (2 * X_REGISTER_SIZE)
 
 extern const struct regset aarch64_linux_gregset;
 extern const struct regset aarch64_linux_fpregset;
+extern const struct regset aarch64_linux_pauthregset;
+
+#ifndef HWCAP_APIA
+/* AArch64 GNU/Linux HWCAP values.  These should be synced with kernel
+   definitions.  */
+#define HWCAP_APIA (1 << 16)
+#endif