[1/2] gdb: return when exceeding buffer size in regcache::transfer_regset
Checks
| Context |
Check |
Description |
| linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_gdb_build--master-arm |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_gdb_check--master-arm |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 |
success
|
Testing passed
|
Commit Message
regcache::transfer_regset iterates over an array of regcache_map_entry,
transferring the registers (between regcache and buffer) described by
those entries. It stops either when it reaches the end of the
regcache_map_entry array (marked by a null entry) or (it seems like the
intent is) when it reaches the end of the buffer (in which case not all
described registers are transferred).
I said "seems like the intent is", because there appears to be a small
bug. transfer_regset is made of two loops:
foreach regcache_map_entry:
foreach register described by the regcache_map_entry:
if the register doesn't fit in the remainder of the buffer:
break
transfer register
When stopping because we have reached the end of the buffer, the break
only breaks out of the inner loop.
This problem causes some failures when I run tests such as
gdb.arch/aarch64-sme-core-3.exp (on AArch64 Linux, in qemu). This is
partly due to aarch64_linux_iterate_over_regset_sections failing to add
a null terminator in its regcache_map_entry array, but I think there is
still a problem in transfer_regset.
The sequence to the crash is:
- The `regcache_map_entry za_regmap` object built in
aarch64_linux_iterate_over_regset_sections does not have a null
terminator.
- When the target does not have a ZA register,
aarch64_linux_collect_za_regset calls `regcache->collect_regset` with
a size of 0 (it's actually pointless, but still it should work).
- transfer_regset gets called with a buffer size of 0.
- transfer_regset detects that the register to transfer wouldn't fit in
0 bytes, so it breaks out of the inner loop.
- The outer loop tries to go read the next regcache_map_entry, but
there isn't one, and we start reading garbage.
Obviously, this would get fixed by making
aarch64_linux_iterate_over_regset_sections use a null terminator (which
is what the following patch does). But I think that when detecting that
there is not enough buffer left for the current register,
transfer_regset should return, not only break out of the inner loop.
This is a kind of contrived scenario, but imagine we have these two
regcache_map_entry objects:
- 2 registers of 8 bytes
- 2 registers of 4 bytes
For some reason, the caller passes a buffer of 12 bytes.
transfer_regset will detect that the second 8 byte register does not
fit, and break out of the inner loop. However, it will then go try the
next regcache_map_entry. It will see that it can fit one 4 byte
register in the remaining buffer space, and transfer it from/to there.
This is very likely not an expected behavior, we wouldn't expect to
read/write this sequence of registers from/to the buffer.
In this example, whether passing a 12 bytes buffer makes sense or
whether it is a size computation bug in the caller, we don't know, but I
think that exiting as soon as a register doesn't fit is the sane thing
to do.
Change-Id: Ia349627d2e5d281822ade92a8e7a4dea4f839e07
---
gdb/regcache.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Comments
On 11/30/23 1:20 PM, Simon Marchi wrote:
> regcache::transfer_regset iterates over an array of regcache_map_entry,
> transferring the registers (between regcache and buffer) described by
> those entries. It stops either when it reaches the end of the
> regcache_map_entry array (marked by a null entry) or (it seems like the
> intent is) when it reaches the end of the buffer (in which case not all
> described registers are transferred).
>
> I said "seems like the intent is", because there appears to be a small
> bug. transfer_regset is made of two loops:
>
> foreach regcache_map_entry:
> foreach register described by the regcache_map_entry:
> if the register doesn't fit in the remainder of the buffer:
> break
>
> transfer register
>
> When stopping because we have reached the end of the buffer, the break
> only breaks out of the inner loop.
>
> This problem causes some failures when I run tests such as
> gdb.arch/aarch64-sme-core-3.exp (on AArch64 Linux, in qemu). This is
> partly due to aarch64_linux_iterate_over_regset_sections failing to add
> a null terminator in its regcache_map_entry array, but I think there is
> still a problem in transfer_regset.
>
> The sequence to the crash is:
>
> - The `regcache_map_entry za_regmap` object built in
> aarch64_linux_iterate_over_regset_sections does not have a null
> terminator.
> - When the target does not have a ZA register,
> aarch64_linux_collect_za_regset calls `regcache->collect_regset` with
> a size of 0 (it's actually pointless, but still it should work).
> - transfer_regset gets called with a buffer size of 0.
> - transfer_regset detects that the register to transfer wouldn't fit in
> 0 bytes, so it breaks out of the inner loop.
> - The outer loop tries to go read the next regcache_map_entry, but
> there isn't one, and we start reading garbage.
>
> Obviously, this would get fixed by making
> aarch64_linux_iterate_over_regset_sections use a null terminator (which
> is what the following patch does). But I think that when detecting that
> there is not enough buffer left for the current register,
> transfer_regset should return, not only break out of the inner loop.
>
> This is a kind of contrived scenario, but imagine we have these two
> regcache_map_entry objects:
>
> - 2 registers of 8 bytes
> - 2 registers of 4 bytes
>
> For some reason, the caller passes a buffer of 12 bytes.
> transfer_regset will detect that the second 8 byte register does not
> fit, and break out of the inner loop. However, it will then go try the
> next regcache_map_entry. It will see that it can fit one 4 byte
> register in the remaining buffer space, and transfer it from/to there.
> This is very likely not an expected behavior, we wouldn't expect to
> read/write this sequence of registers from/to the buffer.
>
> In this example, whether passing a 12 bytes buffer makes sense or
> whether it is a size computation bug in the caller, we don't know, but I
> think that exiting as soon as a register doesn't fit is the sane thing
> to do.
>
> Change-Id: Ia349627d2e5d281822ade92a8e7a4dea4f839e07
> ---
> gdb/regcache.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/gdb/regcache.c b/gdb/regcache.c
> index 9dc354ec2b3a..e46a0b58f505 100644
> --- a/gdb/regcache.c
> +++ b/gdb/regcache.c
> @@ -1208,7 +1208,7 @@ regcache::transfer_regset (const struct regset *regset, int regbase,
> for (; count--; regno++, offs += slot_size)
> {
> if (offs + slot_size > size)
> - break;
> + return;
>
> transfer_regset_register (out_regcache, regno, in_buf, out_buf,
> slot_size, offs);
This makes sense to me. In particular, note that a few lines below we do a
return for the same condition when an individual register doesn't fit.
Reviewed-By: John Baldwin <jhb@FreeBSD.org>
On 2023-11-30 18:42, John Baldwin wrote:
> On 11/30/23 1:20 PM, Simon Marchi wrote:
>> regcache::transfer_regset iterates over an array of regcache_map_entry,
>> transferring the registers (between regcache and buffer) described by
>> those entries. It stops either when it reaches the end of the
>> regcache_map_entry array (marked by a null entry) or (it seems like the
>> intent is) when it reaches the end of the buffer (in which case not all
>> described registers are transferred).
>>
>> I said "seems like the intent is", because there appears to be a small
>> bug. transfer_regset is made of two loops:
>>
>> foreach regcache_map_entry:
>> foreach register described by the regcache_map_entry:
>> if the register doesn't fit in the remainder of the buffer:
>> break
>>
>> transfer register
>>
>> When stopping because we have reached the end of the buffer, the break
>> only breaks out of the inner loop.
>>
>> This problem causes some failures when I run tests such as
>> gdb.arch/aarch64-sme-core-3.exp (on AArch64 Linux, in qemu). This is
>> partly due to aarch64_linux_iterate_over_regset_sections failing to add
>> a null terminator in its regcache_map_entry array, but I think there is
>> still a problem in transfer_regset.
>>
>> The sequence to the crash is:
>>
>> - The `regcache_map_entry za_regmap` object built in
>> aarch64_linux_iterate_over_regset_sections does not have a null
>> terminator.
>> - When the target does not have a ZA register,
>> aarch64_linux_collect_za_regset calls `regcache->collect_regset` with
>> a size of 0 (it's actually pointless, but still it should work).
>> - transfer_regset gets called with a buffer size of 0.
>> - transfer_regset detects that the register to transfer wouldn't fit in
>> 0 bytes, so it breaks out of the inner loop.
>> - The outer loop tries to go read the next regcache_map_entry, but
>> there isn't one, and we start reading garbage.
>>
>> Obviously, this would get fixed by making
>> aarch64_linux_iterate_over_regset_sections use a null terminator (which
>> is what the following patch does). But I think that when detecting that
>> there is not enough buffer left for the current register,
>> transfer_regset should return, not only break out of the inner loop.
>>
>> This is a kind of contrived scenario, but imagine we have these two
>> regcache_map_entry objects:
>>
>> - 2 registers of 8 bytes
>> - 2 registers of 4 bytes
>>
>> For some reason, the caller passes a buffer of 12 bytes.
>> transfer_regset will detect that the second 8 byte register does not
>> fit, and break out of the inner loop. However, it will then go try the
>> next regcache_map_entry. It will see that it can fit one 4 byte
>> register in the remaining buffer space, and transfer it from/to there.
>> This is very likely not an expected behavior, we wouldn't expect to
>> read/write this sequence of registers from/to the buffer.
>>
>> In this example, whether passing a 12 bytes buffer makes sense or
>> whether it is a size computation bug in the caller, we don't know, but I
>> think that exiting as soon as a register doesn't fit is the sane thing
>> to do.
>>
>> Change-Id: Ia349627d2e5d281822ade92a8e7a4dea4f839e07
>> ---
>> gdb/regcache.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/gdb/regcache.c b/gdb/regcache.c
>> index 9dc354ec2b3a..e46a0b58f505 100644
>> --- a/gdb/regcache.c
>> +++ b/gdb/regcache.c
>> @@ -1208,7 +1208,7 @@ regcache::transfer_regset (const struct regset *regset, int regbase,
>> for (; count--; regno++, offs += slot_size)
>> {
>> if (offs + slot_size > size)
>> - break;
>> + return;
>> transfer_regset_register (out_regcache, regno, in_buf, out_buf,
>> slot_size, offs);
>
> This makes sense to me. In particular, note that a few lines below we do a
> return for the same condition when an individual register doesn't fit.
Yeah, although it could be argued that this case is different: we're
targetting a single register, we found the regcache_map_entry that
describes that register, so regardless of the outcome, we know we don't
want to keep searching the regmap array.
> Reviewed-By: John Baldwin <jhb@FreeBSD.org>
Thanks, will add the trailer.
Simon
On 11/30/23 21:20, Simon Marchi wrote:
> regcache::transfer_regset iterates over an array of regcache_map_entry,
> transferring the registers (between regcache and buffer) described by
> those entries. It stops either when it reaches the end of the
> regcache_map_entry array (marked by a null entry) or (it seems like the
> intent is) when it reaches the end of the buffer (in which case not all
> described registers are transferred).
>
> I said "seems like the intent is", because there appears to be a small
> bug. transfer_regset is made of two loops:
>
> foreach regcache_map_entry:
> foreach register described by the regcache_map_entry:
> if the register doesn't fit in the remainder of the buffer:
> break
>
> transfer register
>
> When stopping because we have reached the end of the buffer, the break
> only breaks out of the inner loop.
>
> This problem causes some failures when I run tests such as
> gdb.arch/aarch64-sme-core-3.exp (on AArch64 Linux, in qemu). This is
> partly due to aarch64_linux_iterate_over_regset_sections failing to add
> a null terminator in its regcache_map_entry array, but I think there is
> still a problem in transfer_regset.
>
> The sequence to the crash is:
>
> - The `regcache_map_entry za_regmap` object built in
> aarch64_linux_iterate_over_regset_sections does not have a null
> terminator.
> - When the target does not have a ZA register,
> aarch64_linux_collect_za_regset calls `regcache->collect_regset` with
> a size of 0 (it's actually pointless, but still it should work).
> - transfer_regset gets called with a buffer size of 0.
> - transfer_regset detects that the register to transfer wouldn't fit in
> 0 bytes, so it breaks out of the inner loop.
> - The outer loop tries to go read the next regcache_map_entry, but
> there isn't one, and we start reading garbage.
>
> Obviously, this would get fixed by making
> aarch64_linux_iterate_over_regset_sections use a null terminator (which
> is what the following patch does). But I think that when detecting that
> there is not enough buffer left for the current register,
> transfer_regset should return, not only break out of the inner loop.
>
> This is a kind of contrived scenario, but imagine we have these two
> regcache_map_entry objects:
>
> - 2 registers of 8 bytes
> - 2 registers of 4 bytes
>
> For some reason, the caller passes a buffer of 12 bytes.
> transfer_regset will detect that the second 8 byte register does not
> fit, and break out of the inner loop. However, it will then go try the
> next regcache_map_entry. It will see that it can fit one 4 byte
> register in the remaining buffer space, and transfer it from/to there.
> This is very likely not an expected behavior, we wouldn't expect to
> read/write this sequence of registers from/to the buffer.
>
> In this example, whether passing a 12 bytes buffer makes sense or
> whether it is a size computation bug in the caller, we don't know, but I
> think that exiting as soon as a register doesn't fit is the sane thing
> to do.
>
> Change-Id: Ia349627d2e5d281822ade92a8e7a4dea4f839e07
> ---
> gdb/regcache.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/gdb/regcache.c b/gdb/regcache.c
> index 9dc354ec2b3a..e46a0b58f505 100644
> --- a/gdb/regcache.c
> +++ b/gdb/regcache.c
> @@ -1208,7 +1208,7 @@ regcache::transfer_regset (const struct regset *regset, int regbase,
> for (; count--; regno++, offs += slot_size)
> {
> if (offs + slot_size > size)
> - break;
> + return;
>
> transfer_regset_register (out_regcache, regno, in_buf, out_buf,
> slot_size, offs);
LGTM.
Reviewed-By: Luis Machado <luis.machado@arm.com>
@@ -1208,7 +1208,7 @@ regcache::transfer_regset (const struct regset *regset, int regbase,
for (; count--; regno++, offs += slot_size)
{
if (offs + slot_size > size)
- break;
+ return;
transfer_regset_register (out_regcache, regno, in_buf, out_buf,
slot_size, offs);