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

[Alan Hayward via Gdb-patches <gdb-patches@sourceware.org>]

> 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.