diff mbox

Aarch64 SVE: Fix stack smashing when calling functions

Message ID 20180917144540.78906-1-alan.hayward@arm.com
State New
Headers show

Commit Message

Alan Hayward Sept. 17, 2018, 2:45 p.m. UTC
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

Joel Brobecker Sept. 17, 2018, 6:42 p.m. UTC | #1
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!
Alan Hayward Sept. 18, 2018, 9:08 a.m. UTC | #2
> 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.
Joel Brobecker Sept. 18, 2018, 2:39 p.m. UTC | #3
> >> 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,
diff mbox

Patch

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));
 
       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)
 	    {