* [PATCH] [AArch64] Sanitize the address before working with allocation tags
@ 2021-05-18 20:19 Luis Machado
2021-05-18 20:33 ` Simon Marchi
0 siblings, 1 reply; 6+ messages in thread
From: Luis Machado @ 2021-05-18 20:19 UTC (permalink / raw)
To: 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] [AArch64] Sanitize the address before working with allocation tags
2021-05-18 20:19 [PATCH] [AArch64] Sanitize the address before working with allocation tags Luis Machado
@ 2021-05-18 20:33 ` Simon Marchi
2021-05-18 21:20 ` Luis Machado
0 siblings, 1 reply; 6+ messages in thread
From: Simon Marchi @ 2021-05-18 20:33 UTC (permalink / raw)
To: Luis Machado, 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] [AArch64] Sanitize the address before working with allocation tags
2021-05-18 20:33 ` Simon Marchi
@ 2021-05-18 21:20 ` Luis Machado
2021-05-20 8:38 ` Alan Hayward
0 siblings, 1 reply; 6+ messages in thread
From: Luis Machado @ 2021-05-18 21:20 UTC (permalink / raw)
To: Simon Marchi, 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] [AArch64] Sanitize the address before working with allocation tags
2021-05-18 21:20 ` Luis Machado
@ 2021-05-20 8:38 ` Alan Hayward
2021-05-20 10:59 ` Luis Machado
0 siblings, 1 reply; 6+ messages in thread
From: Alan Hayward @ 2021-05-20 8:38 UTC (permalink / raw)
To: Luis Machado; +Cc: Simon Marchi, gdb-patches\@sourceware.org, nd
> 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] [AArch64] Sanitize the address before working with allocation tags
2021-05-20 8:38 ` Alan Hayward
@ 2021-05-20 10:59 ` Luis Machado
2021-05-20 12:22 ` Alan Hayward
0 siblings, 1 reply; 6+ messages in thread
From: Luis Machado @ 2021-05-20 10:59 UTC (permalink / raw)
To: Alan Hayward; +Cc: Simon Marchi, gdb-patches\@sourceware.org, nd
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.
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] [AArch64] Sanitize the address before working with allocation tags
2021-05-20 10:59 ` Luis Machado
@ 2021-05-20 12:22 ` Alan Hayward
0 siblings, 0 replies; 6+ messages in thread
From: Alan Hayward @ 2021-05-20 12:22 UTC (permalink / raw)
To: Luis Machado; +Cc: Simon Marchi, gdb-patches\@sourceware.org, nd
> 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-05-20 12:23 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-18 20:19 [PATCH] [AArch64] Sanitize the address before working with allocation tags Luis Machado
2021-05-18 20:33 ` Simon Marchi
2021-05-18 21:20 ` Luis Machado
2021-05-20 8:38 ` Alan Hayward
2021-05-20 10:59 ` Luis Machado
2021-05-20 12:22 ` Alan Hayward
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).