Re: [PATCH,v3] [aarch64] Fix removal of non-address bits for PAuth

[Lancelot SIX <lsix@lancelotsix.com>]

Hi Luis, 

I went through the patch and have a couple of questions above.

> diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
> index 15773c75da8..279c8d98f5d 100644
> --- a/gdb/aarch64-linux-tdep.c
> +++ b/gdb/aarch64-linux-tdep.c
> @@ -1787,7 +1787,8 @@ aarch64_linux_report_signal_info (struct gdbarch *gdbarch,
>        uiout->text ("\n");
>  
>        gdb::optional<CORE_ADDR> atag
> -	= aarch64_mte_get_atag (address_significant (gdbarch, fault_addr));
> +	= aarch64_mte_get_atag (gdbarch_remove_non_address_bits (gdbarch,
> +								 fault_addr));
>        gdb_byte ltag = aarch64_mte_get_ltag (fault_addr);
>  
>        if (!atag.has_value ())
> @@ -1961,6 +1962,47 @@ aarch64_linux_decode_memtag_section (struct gdbarch *gdbarch,
>    return tags;
>  }
>  
> +/* AArch64 implementation of the remove_non_address_bits gdbarch hook.  Remove
> +   non address bits from a pointer value.  */
> +
> +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).  */
> +  CORE_ADDR mask = AARCH64_TOP_BITS_MASK;
> +
> +  if (tdep->has_pauth ())
> +    {
> +      /* 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;
> +
> +      if (regs->cooked_read (tdep->pauth_reg_base, &dmask) != REG_VALID)
> +	dmask = mask;
> +
> +      if (regs->cooked_read (tdep->pauth_reg_base + 1, &cmask) != REG_VALID)
> +	cmask = mask;
> +
> +      if (dmask != cmask)
> +	{
> +	  /* Warn if the masks are different.  */
> +	  aarch64_pauth_mask_warning ();
> +	  mask |= dmask > cmask? dmask : cmask;
> +	}
> +      else
> +	mask |= cmask;

Here, I am wondering what happens if either cooked_read does not return
ROG_VALID.  Wouldn't cmask/dmask have un-initialized values, making the
end of the method hazardous?

I guess initializing both to 0 would solve this.

> +    }
> +
> +  return aarch64_remove_top_bits (pointer, mask);
> +}
> +
>  static void
>  aarch64_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
>  {
> index 0f73286f145..d9c4b994850 100644
> --- a/gdb/arch/aarch64.c
> +++ b/gdb/arch/aarch64.c
> @@ -58,3 +58,30 @@ aarch64_create_target_description (const aarch64_features &features)
>  
>    return tdesc.release ();
>  }
> +
> +/* See arch/aarch64.h.  */
> +
> +CORE_ADDR
> +aarch64_remove_top_bits (CORE_ADDR pointer, CORE_ADDR mask)
> +{
> +  /* The VA range select bit is 55.  This bit tells us if we have a
> +     kernel-space address or a user-space address.  */
> +  bool kernel_address = (pointer & VA_RANGE_SELECT_BIT_MASK) != 0;
> +

I am wondering: is this Linux specific or is this valid accross all
configurations?  If this is linux specific, is aarch64.c the right place
to implement this?

Best,
Lancelot.

> +  /* Remove the top non-address bits.  */
> +  pointer &= ~mask;
> +
> +  /* Sign-extend if we have a kernel-space address.  */
> +  if (kernel_address)
> +    pointer |= mask;
> +
> +  return pointer;
> +}
> +
> +/* See arch/aarch64.h.  */
> +
> +void
> +aarch64_pauth_mask_warning ()
> +{
> +  warning (_("Pointer authentication masks for code (C) and data (D) differ"));
> +}