Aarch64 SVE: Fix stack smashing when calling functions
Commit Message
Using "call" on a function that passes arguments via float registers can cause
gdb to overflow buffers.
Ensure enough memory is reserved to hold a full FP register.
2018-09-17 Alan Hayward <alan.hayward@arm.com>
* aarch64-tdep.c (pass_in_v): Use register size.
(aarch64_extract_return_value): Likewise.
(aarch64_store_return_value): Likewise.
---
gdb/aarch64-tdep.c | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
Comments
Hi Alan,
> Using "call" on a function that passes arguments via float registers can cause
> gdb to overflow buffers.
> Ensure enough memory is reserved to hold a full FP register.
>
> 2018-09-17 Alan Hayward <alan.hayward@arm.com>
>
> * aarch64-tdep.c (pass_in_v): Use register size.
> (aarch64_extract_return_value): Likewise.
> (aarch64_store_return_value): Likewise.
Do we have a testcase already that demonstrates the problem?
Otherwise, it would be nice to add one.
> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> index 6993e9061e..516eb138dc 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -1358,7 +1358,10 @@ pass_in_v (struct gdbarch *gdbarch,
> if (info->nsrn < 8)
> {
> int regnum = AARCH64_V0_REGNUM + info->nsrn;
> - gdb_byte reg[V_REGISTER_SIZE];
> + /* Enough space for a full vector register. */
> + gdb_byte reg[register_size (gdbarch, regnum)];
> + gdb_static_assert (AARCH64_V0_REGNUM == AARCH64_SVE_Z0_REGNUM);
> + gdb_assert (len <= sizeof (reg));
Could you explain the relationship between making the buffer large
enough, which is the purpose of this patch, and the assertion that
AARCH64_V0_REGNUM == AARCH64_SVE_Z0_REGNUM?
I don't see a problem with that assertion, but for archeology
purposes, it is better to decorelate changes that are independent.
It helps better document why we introduced changes.
>
> info->argnum++;
> info->nsrn++;
> @@ -1929,7 +1932,10 @@ aarch64_extract_return_value (struct type *type, struct regcache *regs,
> for (int i = 0; i < elements; i++)
> {
> int regno = AARCH64_V0_REGNUM + i;
> - bfd_byte buf[V_REGISTER_SIZE];
> + /* Enough space for a full vector register. */
> + gdb_byte buf[register_size (gdbarch, regno)];
> + gdb_static_assert (AARCH64_V0_REGNUM == AARCH64_SVE_Z0_REGNUM);
> + gdb_assert (len <= sizeof (buf));
>
> if (aarch64_debug)
> {
> @@ -2039,7 +2045,10 @@ aarch64_store_return_value (struct type *type, struct regcache *regs,
> for (int i = 0; i < elements; i++)
> {
> int regno = AARCH64_V0_REGNUM + i;
> - bfd_byte tmpbuf[V_REGISTER_SIZE];
> + /* Enough space for a full vector register. */
> + gdb_byte tmpbuf[register_size (gdbarch, regno)];
> + gdb_static_assert (AARCH64_V0_REGNUM == AARCH64_SVE_Z0_REGNUM);
> + gdb_assert (len <= sizeof (tmpbuf));
>
> if (aarch64_debug)
> {
> --
> 2.15.2 (Apple Git-101.1)
Thank you!
> On 17 Sep 2018, at 19:42, Joel Brobecker <brobecker@adacore.com> wrote:
>
> Hi Alan,
>
>> Using "call" on a function that passes arguments via float registers can cause
>> gdb to overflow buffers.
>
>> Ensure enough memory is reserved to hold a full FP register.
>>
>> 2018-09-17 Alan Hayward <alan.hayward@arm.com>
>>
>> * aarch64-tdep.c (pass_in_v): Use register size.
>> (aarch64_extract_return_value): Likewise.
>> (aarch64_store_return_value): Likewise.
>
> Do we have a testcase already that demonstrates the problem?
> Otherwise, it would be nice to add one.
I should have mentioned that in the description. I can add:
“This fixes gdb.base/callfuncs.exp for Aarch64 SVE."
>
>> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
>> index 6993e9061e..516eb138dc 100644
>> --- a/gdb/aarch64-tdep.c
>> +++ b/gdb/aarch64-tdep.c
>> @@ -1358,7 +1358,10 @@ pass_in_v (struct gdbarch *gdbarch,
>> if (info->nsrn < 8)
>> {
>> int regnum = AARCH64_V0_REGNUM + info->nsrn;
>> - gdb_byte reg[V_REGISTER_SIZE];
>> + /* Enough space for a full vector register. */
>> + gdb_byte reg[register_size (gdbarch, regnum)];
>> + gdb_static_assert (AARCH64_V0_REGNUM == AARCH64_SVE_Z0_REGNUM);
>> + gdb_assert (len <= sizeof (reg));
>
> Could you explain the relationship between making the buffer large
> enough, which is the purpose of this patch, and the assertion that
> AARCH64_V0_REGNUM == AARCH64_SVE_Z0_REGNUM?
>
> I don't see a problem with that assertion, but for archeology
> purposes, it is better to decorelate changes that are independent.
> It helps better document why we introduced changes.
Thinking about it, that assert should be removed.
I was reusing three lines from aarch64_pseudo_read_value_1, which
passed AARCH64_V0_REGNUM into register_size. There the assert made
sense. When I switched to use regnum I didn’t rethink the assert.
I’ll remove the three new instances of that assert from this patch.
Are you happy with those those changes?
Thanks,
Alan.
> >> 2018-09-17 Alan Hayward <alan.hayward@arm.com>
> >>
> >> * aarch64-tdep.c (pass_in_v): Use register size.
> >> (aarch64_extract_return_value): Likewise.
> >> (aarch64_store_return_value): Likewise.
> >
> > Do we have a testcase already that demonstrates the problem?
> > Otherwise, it would be nice to add one.
>
> I should have mentioned that in the description. I can add:
> “This fixes gdb.base/callfuncs.exp for Aarch64 SVE."
I was hoping for something like that. Nice :).
> >> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> >> index 6993e9061e..516eb138dc 100644
> >> --- a/gdb/aarch64-tdep.c
> >> +++ b/gdb/aarch64-tdep.c
> >> @@ -1358,7 +1358,10 @@ pass_in_v (struct gdbarch *gdbarch,
> >> if (info->nsrn < 8)
> >> {
> >> int regnum = AARCH64_V0_REGNUM + info->nsrn;
> >> - gdb_byte reg[V_REGISTER_SIZE];
> >> + /* Enough space for a full vector register. */
> >> + gdb_byte reg[register_size (gdbarch, regnum)];
> >> + gdb_static_assert (AARCH64_V0_REGNUM == AARCH64_SVE_Z0_REGNUM);
> >> + gdb_assert (len <= sizeof (reg));
> >
> > Could you explain the relationship between making the buffer large
> > enough, which is the purpose of this patch, and the assertion that
> > AARCH64_V0_REGNUM == AARCH64_SVE_Z0_REGNUM?
> >
> > I don't see a problem with that assertion, but for archeology
> > purposes, it is better to decorelate changes that are independent.
> > It helps better document why we introduced changes.
>
> Thinking about it, that assert should be removed.
> I was reusing three lines from aarch64_pseudo_read_value_1, which
> passed AARCH64_V0_REGNUM into register_size. There the assert made
> sense. When I switched to use regnum I didn’t rethink the assert.
>
> I’ll remove the three new instances of that assert from this patch.
>
> Are you happy with those those changes?
Yes; pre-approved with the addition of gdb.base/callfuncs.exp in
the revision log and the 3 asserts removed.
Thank you,
@@ -1358,7 +1358,10 @@ pass_in_v (struct gdbarch *gdbarch,
if (info->nsrn < 8)
{
int regnum = AARCH64_V0_REGNUM + info->nsrn;
- gdb_byte reg[V_REGISTER_SIZE];
+ /* Enough space for a full vector register. */
+ gdb_byte reg[register_size (gdbarch, regnum)];
+ gdb_static_assert (AARCH64_V0_REGNUM == AARCH64_SVE_Z0_REGNUM);
+ gdb_assert (len <= sizeof (reg));
info->argnum++;
info->nsrn++;
@@ -1929,7 +1932,10 @@ aarch64_extract_return_value (struct type *type, struct regcache *regs,
for (int i = 0; i < elements; i++)
{
int regno = AARCH64_V0_REGNUM + i;
- bfd_byte buf[V_REGISTER_SIZE];
+ /* Enough space for a full vector register. */
+ gdb_byte buf[register_size (gdbarch, regno)];
+ gdb_static_assert (AARCH64_V0_REGNUM == AARCH64_SVE_Z0_REGNUM);
+ gdb_assert (len <= sizeof (buf));
if (aarch64_debug)
{
@@ -2039,7 +2045,10 @@ aarch64_store_return_value (struct type *type, struct regcache *regs,
for (int i = 0; i < elements; i++)
{
int regno = AARCH64_V0_REGNUM + i;
- bfd_byte tmpbuf[V_REGISTER_SIZE];
+ /* Enough space for a full vector register. */
+ gdb_byte tmpbuf[register_size (gdbarch, regno)];
+ gdb_static_assert (AARCH64_V0_REGNUM == AARCH64_SVE_Z0_REGNUM);
+ gdb_assert (len <= sizeof (tmpbuf));
if (aarch64_debug)
{