Re: [PATCH, v2] aarch64: Check for valid inferior thread/regcache before reading pauth registers

[Luis Machado via Gdb-patches <gdb-patches@sourceware.org>]

Hi,


On 3/23/23 10:56, Luis Machado via Gdb-patches wrote:
> Changes in v2:
> 
> - Dropped helper functions and used inferior_ptid/current_inferior ()
> instead.  Changes are now aarch64-specific.
> 
> There were reports of gdb throwing internal errors when calling
> inferior_thread ()/get_current_regcache () on a system with
> Pointer Authentication enabled.
> 
> In such cases, gdb produces the following backtrace:
> 
> ../../../repos/binutils-gdb/gdb/thread.c:86: internal-error: inferior_thread: Assertion `current_thread_ != nullptr' failed.
> A problem internal to GDB has been detected,
> further debugging may prove unreliable.
> ----- Backtrace -----
> 0xaaaae04a571f gdb_internal_backtrace_1
> 	../../../repos/binutils-gdb/gdb/bt-utils.c:122
> 0xaaaae04a57f3 _Z22gdb_internal_backtracev
> 	../../../repos/binutils-gdb/gdb/bt-utils.c:168
> 0xaaaae0b52ccf internal_vproblem
> 	../../../repos/binutils-gdb/gdb/utils.c:401
> 0xaaaae0b5310b _Z15internal_verrorPKciS0_St9__va_list
> 	../../../repos/binutils-gdb/gdb/utils.c:481
> 0xaaaae0e24b8f _Z18internal_error_locPKciS0_z
> 	../../../repos/binutils-gdb/gdbsupport/errors.cc:58
> 0xaaaae0a88983 _Z15inferior_threadv
> 	../../../repos/binutils-gdb/gdb/thread.c:86
> 0xaaaae0956c87 _Z20get_current_regcachev
> 	../../../repos/binutils-gdb/gdb/regcache.c:428
> 0xaaaae035223f aarch64_remove_non_address_bits
> 	../../../repos/binutils-gdb/gdb/aarch64-tdep.c:3572
> 0xaaaae03e8abb _Z31gdbarch_remove_non_address_bitsP7gdbarchm
> 	../../../repos/binutils-gdb/gdb/gdbarch.c:3109
> 0xaaaae0a692d7 memory_xfer_partial
> 	../../../repos/binutils-gdb/gdb/target.c:1620
> 0xaaaae0a695e3 _Z19target_xfer_partialP10target_ops13target_objectPKcPhPKhmmPm
> 	../../../repos/binutils-gdb/gdb/target.c:1684
> 0xaaaae0a69e9f target_read_partial
> 	../../../repos/binutils-gdb/gdb/target.c:1937
> 0xaaaae0a69fdf _Z11target_readP10target_ops13target_objectPKcPhml
> 	../../../repos/binutils-gdb/gdb/target.c:1977
> 0xaaaae0a69937 _Z18target_read_memorymPhl
> 	../../../repos/binutils-gdb/gdb/target.c:1773
> 0xaaaae08be523 ps_xfer_memory
> 	../../../repos/binutils-gdb/gdb/proc-service.c:90
> 0xaaaae08be6db ps_pdread
> 	../../../repos/binutils-gdb/gdb/proc-service.c:124
> 0x40001ed7c3b3 _td_fetch_value
> 	/build/glibc-RIFKjK/glibc-2.31/nptl_db/fetch-value.c:115
> 0x40001ed791ef td_ta_map_lwp2thr
> 	/build/glibc-RIFKjK/glibc-2.31/nptl_db/td_ta_map_lwp2thr.c:194
> 0xaaaae07f4473 thread_from_lwp
> 	../../../repos/binutils-gdb/gdb/linux-thread-db.c:413
> 0xaaaae07f6d6f _ZN16thread_db_target4waitE6ptid_tP17target_waitstatus10enum_flagsI16target_wait_flagE
> 	../../../repos/binutils-gdb/gdb/linux-thread-db.c:1420
> 0xaaaae0a6b33b _Z11target_wait6ptid_tP17target_waitstatus10enum_flagsI16target_wait_flagE
> 	../../../repos/binutils-gdb/gdb/target.c:2586
> 0xaaaae0789cf7 do_target_wait_1
> 	../../../repos/binutils-gdb/gdb/infrun.c:3825
> 0xaaaae0789e6f operator()
> 	../../../repos/binutils-gdb/gdb/infrun.c:3884
> 0xaaaae078a167 do_target_wait
> 	../../../repos/binutils-gdb/gdb/infrun.c:3903
> 0xaaaae078b0af _Z20fetch_inferior_eventv
> 	../../../repos/binutils-gdb/gdb/infrun.c:4314
> 0xaaaae076652f _Z22inferior_event_handler19inferior_event_type
> 	../../../repos/binutils-gdb/gdb/inf-loop.c:41
> 0xaaaae07dc68b handle_target_event
> 	../../../repos/binutils-gdb/gdb/linux-nat.c:4206
> 0xaaaae0e25fbb handle_file_event
> 	../../../repos/binutils-gdb/gdbsupport/event-loop.cc:573
> 0xaaaae0e264f3 gdb_wait_for_event
> 	../../../repos/binutils-gdb/gdbsupport/event-loop.cc:694
> 0xaaaae0e24f9b _Z16gdb_do_one_eventi
> 	../../../repos/binutils-gdb/gdbsupport/event-loop.cc:217
> 0xaaaae080f033 start_event_loop
> 	../../../repos/binutils-gdb/gdb/main.c:411
> 0xaaaae080f1b7 captured_command_loop
> 	../../../repos/binutils-gdb/gdb/main.c:475
> 0xaaaae0810b97 captured_main
> 	../../../repos/binutils-gdb/gdb/main.c:1318
> 0xaaaae0810c1b _Z8gdb_mainP18captured_main_args
> 	../../../repos/binutils-gdb/gdb/main.c:1337
> 0xaaaae0338453 main
> 	../../../repos/binutils-gdb/gdb/gdb.c:32
> ---------------------
> ../../../repos/binutils-gdb/gdb/thread.c:86: internal-error: inferior_thread: Assertion `current_thread_ != nullptr' failed.
> A problem internal to GDB has been detected,
> further debugging may prove unreliable.
> Quit this debugging session? (y or n)
> 
> We also see failures across the testsuite if the tests get executed on a target
> that has native support for the pointer authentication feature. But
> gdb.base/break.exp and gdb.base/access-mem-running.exp are two examples of
> tests that run into errors and internal errors.
> 
> This issue started after commit d88cb738e6a7a7179dfaff8af78d69250c852af1, which
> enabled more broad use of pointer authentication masks to remove non-address
> bits of pointers, but wasn't immediately detected because systems with native
> support for pointer authentication are not that common yet.
> 
> The above crash happens because gdb is in the middle of handling an event,
> and do_target_wait_1 calls switch_to_inferior_no_thread, nullifying the
> current thread.  This means a call to inferior_thread () will assert, and
> attempting to call get_current_regcache () will also call inferior_thread (),
> resulting in an assertion as well.
> 
> target_has_registers was one function that seemed useful for detecting these
> types of situation where we don't have a register cache. The problem with that
> is the inconsistent state of inferior_ptid, which is used by
> target_has_registers.
> 
> Despite the call to switch_to_no_thread in switch_to_inferior_no_thread from
> do_target_wait_1 in the backtrace above clearing inferior_ptid, the call to
> ps_xfer_memory sets inferior_ptid momentarily before reading memory:
> 
> static ps_err_e
> ps_xfer_memory (const struct ps_prochandle *ph, psaddr_t addr,
>                  gdb_byte *buf, size_t len, int write)
> {
>    scoped_restore_current_inferior restore_inferior;
>    set_current_inferior (ph->thread->inf);
> 
>    scoped_restore_current_program_space restore_current_progspace;
>    set_current_program_space (ph->thread->inf->pspace);
> 
>    scoped_restore save_inferior_ptid = make_scoped_restore (&inferior_ptid);
>    inferior_ptid = ph->thread->ptid;
> 
>    CORE_ADDR core_addr = ps_addr_to_core_addr (addr);
> 
>    int ret;
>    if (write)
>      ret = target_write_memory (core_addr, buf, len);
>    else
>      ret = target_read_memory (core_addr, buf, len);
>    return (ret == 0 ? PS_OK : PS_ERR);
> }
> 
> Maybe this shouldn't happen, or maybe it is just an unfortunate state to be
> in. But this prevents the use of target_has_registers to guard against the
> lack of registers, since, although current_thread_ is still nullptr,
> inferior_ptid is valid and is not null_ptid.
> 
> There is another crash scenario after we kill a previously active inferior, in
> which case the gdbarch will still say we support pointer authentication but we
> will also have no current thread (inferior_thread () will assert etc).
> 
> If the target has support for pointer authentication, gdb needs to use
> a couple (or 4, for bare-metal) mask registers to mask off some bits of
> pointers, and for that it needs to access the registers.
> 
> At some points, like the one from the backtrace above, there is no active
> thread/current regcache because gdb is in the middle of doing event handling
> and switching between threads.
> 
> Simon suggested the use of inferior_ptid to fetch the register cache, as
> opposed to relying on the current register cache.  Though we need to make sure
> inferior_ptid is valid (not null_ptid), I think this works nicely.
> 
> With inferior_ptid, we can do safety checks along the way, making sure we have
> a thread to fetch a register cache from and checking if the thread is actually
> stopped or running.
> 
> The following patch implements this idea with safety checks to make sure we
> don't run into assertions or errors.  If any of the checks fail, we fallback to
> using a default mask to remove non-address bits of a pointer.
> 
> I discussed with Pedro the possibility of caching the mask register values
> (which are per-process and can change mid-execution), but there isn't a good
> spot to cache those values. Besides, the mask registers can change constantly
> for bare-metal debugging when switching between exception levels.
> 
> In some cases, it is just not possible to get access to these mask registers,
> like the case where threads are running. In those cases, using a default mask
> to remove the non-address bits should be enough.
> 
> This can happen when we let threads run in the background and then we attempt
> to access a memory address (now that gdb is capable of reading memory even
> with threads running).  Thus gdb will attempt to remove non-address bits
> of that memory access, will attempt to access registers, running into errors.
> 
> Regression-tested on aarch64-linux Ubuntu 20.04.
> ---
>   gdb/aarch64-tdep.c | 88 ++++++++++++++++++++++++++++++++--------------
>   1 file changed, 62 insertions(+), 26 deletions(-)
> 
> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> index 5b1b9921f87..d11d8320799 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -55,6 +55,9 @@
>   #include <algorithm>
>   #include <unordered_map>
>   
> +/* For inferior_ptid and current_inferior ().  */
> +#include "inferior.h"
> +
>   /* A Homogeneous Floating-Point or Short-Vector Aggregate may have at most
>      four members.  */
>   #define HA_MAX_NUM_FLDS		4
> @@ -3556,40 +3559,73 @@ aarch64_stack_frame_destroyed_p (struct gdbarch *gdbarch, CORE_ADDR pc)
>   static CORE_ADDR
>   aarch64_remove_non_address_bits (struct gdbarch *gdbarch, CORE_ADDR pointer)
>   {
> -  aarch64_gdbarch_tdep *tdep = gdbarch_tdep<aarch64_gdbarch_tdep> (gdbarch);
> -
>     /* By default, we assume TBI and discard the top 8 bits plus the VA range
> -     select bit (55).  */
> +     select bit (55).  Below we try to fetch information about pointer
> +     authentication masks in order to make non-address removal more
> +     precise.  */
>     CORE_ADDR mask = AARCH64_TOP_BITS_MASK;
>   
> -  if (tdep->has_pauth ())
> +  /* Check if we have an inferior first.  If not, just use the default
> +     mask.
> +
> +     We use the inferior_ptid here because the pointer authentication masks
> +     should be the same across threads of a process.  Since we may not have
> +     access to the current thread (gdb may have switched to no inferiors
> +     momentarily), we use the inferior ptid.  */
> +  if (inferior_ptid != null_ptid)
>       {
> -      /* Fetch the PAC masks.  These masks are per-process, so we can just
> -	 fetch data from whatever thread we have at the moment.
> -
> -	 Also, we have both a code mask and a data mask.  For now they are the
> -	 same, but this may change in the future.  */
> -      struct regcache *regs = get_current_regcache ();
> -      CORE_ADDR cmask, dmask;
> -      int dmask_regnum = AARCH64_PAUTH_DMASK_REGNUM (tdep->pauth_reg_base);
> -      int cmask_regnum = AARCH64_PAUTH_CMASK_REGNUM (tdep->pauth_reg_base);
> -
> -      /* If we have a kernel address and we have kernel-mode address mask
> -	 registers, use those instead.  */
> -      if (tdep->pauth_reg_count > 2
> -	  && pointer & VA_RANGE_SELECT_BIT_MASK)
> +      /* If we do have an inferior, attempt to fetch its thread's thread_info
> +	 struct.  */
> +      thread_info *thread
> +	= find_thread_ptid (current_inferior ()->process_target (),
> +			    inferior_ptid);
> +
> +      /* If the thread is running, we will not be able to fetch the mask
> +	 registers.  */
> +      if (thread != nullptr && thread->state != THREAD_RUNNING)
>   	{
> -	  dmask_regnum = AARCH64_PAUTH_DMASK_HIGH_REGNUM (tdep->pauth_reg_base);
> -	  cmask_regnum = AARCH64_PAUTH_CMASK_HIGH_REGNUM (tdep->pauth_reg_base);
> -	}
> +	  /* Otherwise, fetch the register cache and the masks.  */
> +	  struct regcache *regs
> +	    = get_thread_regcache (current_inferior ()->process_target (),
> +				   inferior_ptid);
> +
> +	  /* Use the gdbarch from the register cache to check for pointer
> +	     authentication support, as it matches the features found in
> +	     that particular thread.  */
> +	  aarch64_gdbarch_tdep *tdep
> +	    = gdbarch_tdep<aarch64_gdbarch_tdep> (regs->arch ());
>   
> -      if (regs->cooked_read (dmask_regnum, &dmask) != REG_VALID)
> -	dmask = mask;
> +	  /* Is there pointer authentication support?  */
> +	  if (tdep->has_pauth ())
> +	    {
> +	      CORE_ADDR cmask, dmask;
> +	      int dmask_regnum
> +		= AARCH64_PAUTH_DMASK_REGNUM (tdep->pauth_reg_base);
> +	      int cmask_regnum
> +		= AARCH64_PAUTH_CMASK_REGNUM (tdep->pauth_reg_base);
> +
> +	      /* If we have a kernel address and we have kernel-mode address
> +		 mask registers, use those instead.  */
> +	      if (tdep->pauth_reg_count > 2
> +		  && pointer & VA_RANGE_SELECT_BIT_MASK)
> +		{
> +		  dmask_regnum
> +		    = AARCH64_PAUTH_DMASK_HIGH_REGNUM (tdep->pauth_reg_base);
> +		  cmask_regnum
> +		    = AARCH64_PAUTH_CMASK_HIGH_REGNUM (tdep->pauth_reg_base);
> +		}
>   
> -      if (regs->cooked_read (cmask_regnum, &cmask) != REG_VALID)
> -	cmask = mask;
> +	      /* We have both a code mask and a data mask.  For now they are
> +		 the same, but this may change in the future.  */
> +	      if (regs->cooked_read (dmask_regnum, &dmask) != REG_VALID)
> +		dmask = mask;
>   
> -      mask |= aarch64_mask_from_pac_registers (cmask, dmask);
> +	      if (regs->cooked_read (cmask_regnum, &cmask) != REG_VALID)
> +		cmask = mask;
> +
> +	      mask |= aarch64_mask_from_pac_registers (cmask, dmask);
> +	    }
> +	}
>       }
>   
>     return aarch64_remove_top_bits (pointer, mask);


Unless there are objections to the strategy used in the patch, I'm planning on pushing this later today and
also pushing a backport to gdb 13.1 (where this code is linux-specific).