Re: [PATCH v5 10/12] gdb: Implement amd64 linux shadow stack support for inferior calls.

[Andrew Burgess <aburgess@redhat.com>]

Christina Schimpe <christina.schimpe@intel.com> writes:

> This patch enables inferior calls to support Intel's Control-Flow
> Enforcement Technology (CET), which provides the shadow stack feature
> for the x86 architecture.
> Following the restriction of the linux kernel, enable inferior calls
> for amd64 only.
>
> Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
> Reviewed-By: Eli Zaretskii <eliz@gnu.org>
> Reviewed-By: Luis Machado <luis.machado@arm.com>
> ---
>  gdb/amd64-linux-tdep.c                        | 63 +++++++++++++++++++
>  gdb/doc/gdb.texinfo                           | 29 +++++++++
>  .../gdb.arch/amd64-shadow-stack-cmds.exp      | 55 +++++++++++++++-
>  3 files changed, 146 insertions(+), 1 deletion(-)
>
> diff --git a/gdb/amd64-linux-tdep.c b/gdb/amd64-linux-tdep.c
> index f6ae5395870..899fe2df02c 100644
> --- a/gdb/amd64-linux-tdep.c
> +++ b/gdb/amd64-linux-tdep.c
> @@ -1932,6 +1932,67 @@ amd64_linux_shadow_stack_element_size_aligned (gdbarch *gdbarch)
>    return (binfo->bits_per_word / binfo->bits_per_byte);
>  }
>  
> +/* Read the shadow stack pointer register and return its value, if
> +   possible.  */
> +
> +static std::optional<CORE_ADDR>
> +amd64_linux_get_shadow_stack_pointer (gdbarch *gdbarch, regcache *regcache)
> +{
> +  const i386_gdbarch_tdep *tdep = gdbarch_tdep<i386_gdbarch_tdep> (gdbarch);
> +
> +  if (tdep == nullptr || tdep->ssp_regnum < 0)
> +    return {};

We don't check for 'tdep == nullptr' anywhere else (for x86), so I don't
think this check is needed.  If you _really_ want you could gdb_assert,
but I'm pretty sure you'll have crashed long before now if tdep ==
nullptr.

> +
> +  CORE_ADDR ssp;
> +  if (regcache_raw_read_unsigned (regcache, tdep->ssp_regnum, &ssp)
> +      != REG_VALID)
> +    return {};
> +
> +  /* Dependent on the target in case the shadow stack pointer is
> +     unavailable, the ssp register can be invalid or 0x0 when shadow stack
> +     is supported by HW and the linux kernel but not enabled for the
> +     current thread.  */

Just so I understand, is the 0 coming from actual inferior state?  Or is
the 0 creeping in from somewhere in GDB when we should be creating an
unavailable value, but get it wrong?

Using 0 a magic address value is something I really dislike (having
worked on targets where 0 is a valid address), so they always make me
uncomfortable.  If this is an actual h/w thing then there's nothing we
could or should do about it ... but if this is a GDB thing, then maybe
we can fix that?

> +  if (ssp == 0x0)
> +    return {};
> +
> +  return ssp;
> +}
> +
> +/* If shadow stack is enabled, push the address NEW_ADDR to the shadow
> +   stack and increment the shadow stack pointer accordingly.  */
> +
> +static void
> +amd64_linux_shadow_stack_push (gdbarch *gdbarch, CORE_ADDR new_addr,
> +			       regcache *regcache)
> +{
> +  std::optional<CORE_ADDR> ssp
> +    = amd64_linux_get_shadow_stack_pointer (gdbarch, regcache);
> +  if (!ssp.has_value ())
> +    return;
> +
> +  /* The shadow stack grows downwards.  To push addresses to the stack,
> +     we need to decrement SSP.    */
> +  const int element_size
> +    = amd64_linux_shadow_stack_element_size_aligned (gdbarch);
> +  const CORE_ADDR new_ssp = *ssp - element_size;
> +
> +  /* Using /proc/PID/smaps we can only check if NEW_SSP points to shadow
> +     stack memory.  If it doesn't, we assume the stack is full.  */
> +  std::pair<CORE_ADDR, CORE_ADDR> memrange;
> +  if (!linux_address_in_shadow_stack_mem_range (new_ssp, &memrange))
> +    error (_("No space left on the shadow stack."));
> +
> +  /* On x86 there can be a shadow stack token at bit 63.  For x32, the

I don't understand this first sentence and how it relates to either the
rest of the comment, or the following code.

> +     address size is only 32 bit.  Thus, we must use ELEMENT_SIZE (and
> +     not gdbarch_addr_bit) to determine the width of the address to be
> +     written.  */
> +  const bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> +  write_memory_unsigned_integer (new_ssp, element_size, byte_order,
> +				 (ULONGEST) new_addr);
> +
> +  i386_gdbarch_tdep *tdep = gdbarch_tdep<i386_gdbarch_tdep> (gdbarch);
> +  regcache_raw_write_unsigned (regcache, tdep->ssp_regnum, new_ssp);

By this point we know that tdep->ssp_regnum must be a valid regnum.  But
the checks for that are in a separate function, so maybe we should:

  gdb_assert (tdep->ssp_regnum > -1);

> +}
>  
>  /* Implement shadow stack pointer unwinding. For each new shadow stack
>     pointer check if its address is still in the shadow stack memory range.
> @@ -2059,6 +2120,8 @@ amd64_linux_init_abi_common(struct gdbarch_info info, struct gdbarch *gdbarch,
>  
>    set_gdbarch_remove_non_address_bits_watchpoint
>      (gdbarch, amd64_linux_remove_non_address_bits_watchpoint);
> +
> +  set_gdbarch_shadow_stack_push (gdbarch, amd64_linux_shadow_stack_push);
>    dwarf2_frame_set_init_reg (gdbarch, amd64_init_reg);
>  }
>  
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index 0881ac4aee5..b5120b78426 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -27037,6 +27037,35 @@ registers
>  
>  @end itemize
>  
> +@subsubsection Intel Control-Flow Enforcement Technology.
> +@cindex Intel Control-Flow Enforcement Technology.
> +
> +The @dfn{Intel Control-Flow Enforcement Technology} (@acronym{Intel CET})
> +provides two capabilities to defend against ``Return-oriented Programming''
> +and ``call/jmp-oriented programming'' style control-flow attacks:
> +
> +@itemize @bullet
> +@item Shadow Stack:
> +A shadow stack is a second stack for a program.  It holds the return
> +addresses pushed by the call instruction.  The @code{RET} instruction pops the
> +return addresses from both call and shadow stack.  If the return addresses from
> +the two stacks do not match, the processor signals a control protection
> +exception.
> +@item Indirect Branch Tracking (IBT):
> +When IBT is enabled, the CPU implements a state machine that tracks indirect
> +@code{JMP} and @code{CALL} instructions.  The state machine can be either IDLE
> +or WAIT_FOR_ENDBRANCH.  In WAIT_FOR_ENDBRANCH state the next instruction in
> +the program stream must be an @code{ENDBR} instruction, otherwise the
> +processor signals a control protection exception.

If I understand it, the IBT doesn't currently have an impact on GDB,
right?  The inferior function being called likely starts with an `endbr`
instruction, but GDB will just leave the IBT mechanism in the IDLE
state, and the endbr will be interpreted as a nop.

I also found this description a little too light on the details.  Just
having the name was enough to go and find the real docs, but I think the
text could be made cleared with two additional sentences:

  @item Indirect Branch Tracking (IBT):
  When IBT is enabled, the CPU implements a state machine that tracks indirect
  @code{JMP} and @code{CALL} instructions.  The state machine can be either IDLE
  or WAIT_FOR_ENDBRANCH.  When a @code{JMP} or @code{CALL} is executed
  the state machine chages to the WAIT_FOR_ENDBRANCH state.  In
  WAIT_FOR_ENDBRANCH state the next instruction in the program stream
  must be an @code{ENDBR} instruction, otherwise the
  processor signals a control protection exception.  After executing a
  @code{ENDBR} instruction the state machine returns to the IDLE state.

This change isn't a hard requirement, but I do think this makes the
description more useful.

> +@end itemize
> +
> +Impact on Call/Print:
> +Inferior calls in @value{GDBN} reset the current PC to the beginning of the
> +function that is called.  No call instruction is executed, but the @code{RET}
> +instruction actually is.  To avoid a control protection exception due to the
> +missing return address on the shadow stack, @value{GDBN} pushes the new return
> +address to the shadow stack and updates the shadow stack pointer.
> +
>  @node Alpha
>  @subsection Alpha
>  
> diff --git a/gdb/testsuite/gdb.arch/amd64-shadow-stack-cmds.exp b/gdb/testsuite/gdb.arch/amd64-shadow-stack-cmds.exp
> index 17f32ce3964..622612d2f7d 100644
> --- a/gdb/testsuite/gdb.arch/amd64-shadow-stack-cmds.exp
> +++ b/gdb/testsuite/gdb.arch/amd64-shadow-stack-cmds.exp
> @@ -13,12 +13,29 @@
>  # You should have received a copy of the GNU General Public License
>  # along with this program.  If not, see <http://www.gnu.org/licenses/>.
>  
> -# Test shadow stack enabling for frame level update and the return command.
> +# Test shadow stack enabling for frame level update, the return and the
> +# call commands.
> +# As potential CET violations often only occur after resuming normal
> +# execution, test normal program continuation after each return or call
> +# commands.
>  
>  require allow_ssp_tests
>  
>  standard_testfile amd64-shadow-stack.c
>  
> +proc restart_and_run_infcall_call2 {} {

There should be a comment before each proc please.

Thanks,
Andrew