RE: [PATCH 07/12] gdb, bfd: amd64 linux coredump support with shadow stack.

["Schimpe, Christina" <christina.schimpe@intel.com>]

> -----Original Message-----
> From: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
> Sent: Thursday, February 6, 2025 4:15 AM
> To: Schimpe, Christina <christina.schimpe@intel.com>
> Cc: gdb-patches@sourceware.org
> Subject: Re: [PATCH 07/12] gdb, bfd: amd64 linux coredump support with shadow
> stack.
> 
> 
> "Schimpe, Christina" <christina.schimpe@intel.com> writes:
> 
> > From: Felix Willgerodt <felix.willgerodt@intel.com>
> >
> > Intel's Control-Flow Enforcement Technology (CET) provides the shadow
> > stack feature for the x86 architecture.
> >
> > This commit adds support to write and read the shadow-stack node in
> > corefiles.  This helps debugging return address violations post-mortem.
> > The format is synced with the linux kernel commit "x86: Add PTRACE
> > interface for shadow stack".  As the linux kernel restricts shadow
> > stack support to 64-bit, apply the fix for amd64 only.
> >
> > Co-Authored-By: Christina Schimpe <christina.schimpe@intel.com>
> > ---
> >  bfd/elf.c                                     | 24 +++++++++
> 
> I don't know what is current practice: should BFD changes be sent separately to
> the binutils mailing list, or in the same patch or patch series as the GDB changes
> that use them?
> 
> If the latter, then the binutils mailing list needs to at least be on Cc:

Right, I think I should post it in the binutils mailing list. Thanks for pointing that out.
I will fix your comments and post it then there.

Christina


> >  gdb/amd64-linux-tdep.c                        | 52 +++++++++++++++++--
> >  .../gdb.arch/amd64-shadow-stack-corefile.exp  | 50 ++++++++++++++++++
> >  3 files changed, 122 insertions(+), 4 deletions(-)  create mode
> > 100644 gdb/testsuite/gdb.arch/amd64-shadow-stack-corefile.exp
> 
> Just some minor comments.
> 
> Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
> 
> > @@ -1630,6 +1642,30 @@ static const struct regset amd64_linux_xstateregset
> =
> >      amd64_linux_collect_xstateregset
> >    };
> >
> > +static void
> > +amd64_linux_supply_ssp (const struct regset *regset,
> > +			struct regcache *regcache, int regnum,
> > +			const void *ssp, size_t len)
> > +{
> > +  x86_supply_ssp (regcache, *static_cast<const uint64_t *> (ssp)); }
> > +
> > +static void
> > +amd64_linux_collect_ssp (const struct regset *regset,
> > +			 const struct regcache *regcache, int regnum,
> > +			 void *ssp, size_t len)
> > +{
> > +  x86_collect_ssp (regcache, *static_cast<uint64_t *> (ssp)); }
> 
> These functions should have documentation comments.
> 
> > +
> > +/* Shadow stack pointer register.  */
> > +
> > +static const struct regset amd64_linux_ssp_register
> > +  {
> > +    NULL, amd64_linux_supply_ssp, amd64_linux_collect_ssp
> > +  };
> > +
> > +
> 
> Just one line of separation between functions is enough.
> 
> >  /* Iterate over core file register note sections.  */
> >
> >  static void
> 
> --
> Thiago
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