[PATCH] [AArch64] Sanitize the address before working with allocation tags

[PATCH] [AArch64] Sanitize the address before working with allocation tags

[Luis Machado via Gdb-patches]

Remove the logical tag/top byte from the address whenever we have to work with
allocation tags.

gdb/ChangeLog:

YYYY-MM-DD  Luis Machado  <luis.machado@linaro.org>

	* aarch64-linux-tdep.c (aarch64_linux_memtag_matches_p): Remove the top
	byte.
	(aarch64_linux_set_memtags): Likewise.
	(aarch64_linux_get_memtag): Likewise.
	(aarch64_linux_report_signal_info): Likewise.
---
 gdb/aarch64-linux-tdep.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
index 9602fc4b29a..e9761ed2189 100644
--- a/gdb/aarch64-linux-tdep.c
+++ b/gdb/aarch64-linux-tdep.c
@@ -1587,7 +1587,8 @@ aarch64_linux_memtag_matches_p (struct gdbarch *gdbarch,
   CORE_ADDR addr = value_as_address (address);
 
   /* Fetch the allocation tag for ADDRESS.  */
-  gdb::optional<CORE_ADDR> atag = aarch64_mte_get_atag (addr);
+  gdb::optional<CORE_ADDR> atag
+    = aarch64_mte_get_atag (address_significant (gdbarch, addr));
 
   if (!atag.has_value ())
     return true;
@@ -1625,6 +1626,9 @@ aarch64_linux_set_memtags (struct gdbarch *gdbarch, struct value *address,
     }
   else
     {
+      /* Remove the top byte.  */
+      addr = address_significant (gdbarch, addr);
+
       /* Make sure we are dealing with a tagged address to begin with.  */
       if (!aarch64_linux_tagged_address_p (gdbarch, address))
 	return false;
@@ -1679,6 +1683,8 @@ aarch64_linux_get_memtag (struct gdbarch *gdbarch, struct value *address,
       if (!aarch64_linux_tagged_address_p (gdbarch, address))
 	return nullptr;
 
+      /* Remove the top byte.  */
+      addr = address_significant (gdbarch, addr);
       gdb::optional<CORE_ADDR> atag = aarch64_mte_get_atag (addr);
 
       if (!atag.has_value ())
@@ -1751,7 +1757,8 @@ aarch64_linux_report_signal_info (struct gdbarch *gdbarch,
       uiout->field_core_addr ("fault-addr", gdbarch, fault_addr);
       uiout->text ("\n");
 
-      gdb::optional<CORE_ADDR> atag = aarch64_mte_get_atag (fault_addr);
+      gdb::optional<CORE_ADDR> atag
+	= aarch64_mte_get_atag (address_significant (gdbarch, fault_addr));
       gdb_byte ltag = aarch64_mte_get_ltag (fault_addr);
 
       if (!atag.has_value ())
-- 
2.25.1

Re: [PATCH] [AArch64] Sanitize the address before working with allocation tags

[Simon Marchi via Gdb-patches]

On 2021-05-18 4:19 p.m., Luis Machado via Gdb-patches wrote:
> Remove the logical tag/top byte from the address whenever we have to work with
> allocation tags.

Can you explain a bit more why this is needed?  What down the line
doesn't like to receive an address with a logical tag?

Simon

Re: [PATCH] [AArch64] Sanitize the address before working with allocation tags

[Luis Machado via Gdb-patches]

On 5/18/21 5:33 PM, Simon Marchi wrote:
> On 2021-05-18 4:19 p.m., Luis Machado via Gdb-patches wrote:
>> Remove the logical tag/top byte from the address whenever we have to work with
>> allocation tags.
> 
> Can you explain a bit more why this is needed?  What down the line
> doesn't like to receive an address with a logical tag?

We shouldn't be passing an address with a non-zero top byte (or tag) to 
a ptrace request, for example. It may work (in fact, it works) but we 
are not supposed to rely on it. So we sanitize the pointer before it 
gets to fetch_memtags/store_memtags.

This is clarified in the AArch64 Tagged Address ABI document 
(https://www.kernel.org/doc/html/latest/arm64/tagged-address-abi.html).

In an upcoming patch to support memory tags in core files 
(https://sourceware.org/pipermail/gdb-patches/2021-May/178973.html), 
this address also gets passed down to the core target's fetch_memtags 
implementation. It needs to compare addresses, so it doesn't make sense 
to let through an address with a non-zero top byte, or else we risk not 
having a match due to differences in the upper byte.

Re: [PATCH] [AArch64] Sanitize the address before working with allocation tags

[Alan Hayward via Gdb-patches]

> On 18 May 2021, at 22:20, Luis Machado via Gdb-patches <gdb-patches@sourceware.org> wrote:
> 
> On 5/18/21 5:33 PM, Simon Marchi wrote:
>> On 2021-05-18 4:19 p.m., Luis Machado via Gdb-patches wrote:
>>> Remove the logical tag/top byte from the address whenever we have to work with
>>> allocation tags.
>> Can you explain a bit more why this is needed?  What down the line
>> doesn't like to receive an address with a logical tag?
> 
> We shouldn't be passing an address with a non-zero top byte (or tag) to a ptrace request, for example. It may work (in fact, it works) but we are not supposed to rely on it. So we sanitize the pointer before it gets to fetch_memtags/store_memtags.
> 
> This is clarified in the AArch64 Tagged Address ABI document (https://www.kernel.org/doc/html/latest/arm64/tagged-address-abi.html).
> 
> In an upcoming patch to support memory tags in core files (https://sourceware.org/pipermail/gdb-patches/2021-May/178973.html), this address also gets passed down to the core target's fetch_memtags implementation. It needs to compare addresses, so it doesn't make sense to let through an address with a non-zero top byte, or else we risk not having a match due to differences in the upper byte.
> 


Would it make sense to put the address_significant() at the beginning of aarch64_mte_get_atag()?
That’d ensure any future code that calls aarch64_mte_get_atag() is safe too. And it would mean the higher functions are dealing with a single address throughout.

Alternatively, it could even move down into target_fetch_memtags() instead (same with target_store_memtags), but I’m less keen on that.


Alan.

Re: [PATCH] [AArch64] Sanitize the address before working with allocation tags

[Luis Machado via Gdb-patches]

On 5/20/21 5:38 AM, Alan Hayward wrote:
> 
> 
>> On 18 May 2021, at 22:20, Luis Machado via Gdb-patches <gdb-patches@sourceware.org> wrote:
>>
>> On 5/18/21 5:33 PM, Simon Marchi wrote:
>>> On 2021-05-18 4:19 p.m., Luis Machado via Gdb-patches wrote:
>>>> Remove the logical tag/top byte from the address whenever we have to work with
>>>> allocation tags.
>>> Can you explain a bit more why this is needed?  What down the line
>>> doesn't like to receive an address with a logical tag?
>>
>> We shouldn't be passing an address with a non-zero top byte (or tag) to a ptrace request, for example. It may work (in fact, it works) but we are not supposed to rely on it. So we sanitize the pointer before it gets to fetch_memtags/store_memtags.
>>
>> This is clarified in the AArch64 Tagged Address ABI document (https://www.kernel.org/doc/html/latest/arm64/tagged-address-abi.html).
>>
>> In an upcoming patch to support memory tags in core files (https://sourceware.org/pipermail/gdb-patches/2021-May/178973.html), this address also gets passed down to the core target's fetch_memtags implementation. It needs to compare addresses, so it doesn't make sense to let through an address with a non-zero top byte, or else we risk not having a match due to differences in the upper byte.
>>
> 
> 
> Would it make sense to put the address_significant() at the beginning of aarch64_mte_get_atag()?
> That’d ensure any future code that calls aarch64_mte_get_atag() is safe too. And it would mean the higher functions are dealing with a single address throughout.

Yes and no. I didn't want to pollute aarch64_mte_get_atag by passing 
gdbarch just to be able to call address_significant. Alternatively, we 
could just remove the top byte by hand without using 
address_significantm, since we're within the AArch64 target anyway.

> 
> Alternatively, it could even move down into target_fetch_memtags() instead (same with target_store_memtags), but I’m less keen on that.

Yes. I considered that, but all the implementations would have to 
sanitize the address. We don't know what kinds of targets will want to 
implement those functions, so we can't safely assume they want to strip 
the top bytes. And, again, we'd need to pass/fetch the gdbarch from 
within the native layers just so we can call address_significant.

> 
> 
> Alan.
> 

Re: [PATCH] [AArch64] Sanitize the address before working with allocation tags

[Alan Hayward via Gdb-patches]

> On 20 May 2021, at 11:59, Luis Machado <luis.machado@linaro.org> wrote:
> 
> On 5/20/21 5:38 AM, Alan Hayward wrote:
>>> On 18 May 2021, at 22:20, Luis Machado via Gdb-patches <gdb-patches@sourceware.org> wrote:
>>> 
>>> On 5/18/21 5:33 PM, Simon Marchi wrote:
>>>> On 2021-05-18 4:19 p.m., Luis Machado via Gdb-patches wrote:
>>>>> Remove the logical tag/top byte from the address whenever we have to work with
>>>>> allocation tags.
>>>> Can you explain a bit more why this is needed?  What down the line
>>>> doesn't like to receive an address with a logical tag?
>>> 
>>> We shouldn't be passing an address with a non-zero top byte (or tag) to a ptrace request, for example. It may work (in fact, it works) but we are not supposed to rely on it. So we sanitize the pointer before it gets to fetch_memtags/store_memtags.
>>> 
>>> This is clarified in the AArch64 Tagged Address ABI document (https://www.kernel.org/doc/html/latest/arm64/tagged-address-abi.html).
>>> 
>>> In an upcoming patch to support memory tags in core files (https://sourceware.org/pipermail/gdb-patches/2021-May/178973.html), this address also gets passed down to the core target's fetch_memtags implementation. It needs to compare addresses, so it doesn't make sense to let through an address with a non-zero top byte, or else we risk not having a match due to differences in the upper byte.
>>> 
>> Would it make sense to put the address_significant() at the beginning of aarch64_mte_get_atag()?
>> That’d ensure any future code that calls aarch64_mte_get_atag() is safe too. And it would mean the higher functions are dealing with a single address throughout.
> 
> Yes and no. I didn't want to pollute aarch64_mte_get_atag by passing gdbarch just to be able to call address_significant. Alternatively, we could just remove the top byte by hand without using address_significantm, since we're within the AArch64 target anyway.

aarch64_mte_get_atag is a static function in the same file, so it’s not that big a leap to pass in gdbarch. But I’m not that fixed on changing it either.

> 
>> Alternatively, it could even move down into target_fetch_memtags() instead (same with target_store_memtags), but I’m less keen on that.
> 
> Yes. I considered that, but all the implementations would have to sanitize the address. We don't know what kinds of targets will want to implement those functions, so we can't safely assume they want to strip the top bytes. And, again, we'd need to pass/fetch the gdbarch from within the native layers just so we can call address_significant.

Ok, let’s not do that one then.


I’m ok with this patch as is.


Alan.