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

[Luis Machado <luis.machado@arm.com>]

Hi,

On 9/22/22 13:59, Lancelot SIX wrote:
> 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.
> 

If either one of the register reads fail, then we assign the default MASK. Otherwise we
continue with using the mask that's been read from the register.

Does that make sense?

>> +    }
>> +
>> +  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?

Although this is only used on Linux systems at the moment, it is not technically Linux-specific.

When we say kernel-space, it means the other half of the VA space (non-user).

I have an upcoming patch (relying on this one) to use this function for bare metal pointer authentication
support (with user QEMU). So I think it makes sense to have this function in arch-specific code
and not in Linux-specific code.

> 
> 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"));
>> +}