[PATCH] gdbstub: Fix buffer overflow in handle_read_all_regs

[PATCH] gdbstub: Fix buffer overflow in handle_read_all_regs

[Damien Hedde]

Ensure we don't put too much register data in buffers. This avoids
a buffer overflow (and stack corruption) when a target has lots
of registers.

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---

Hi all,

While working on a target with many registers. I found out the gdbstub
may do buffer overflows when receiving a 'g' query (to read general
registers). This patch prevents that.

Gdb is pretty happy with a partial set of registers and queries
remaining registers one by one when needed.

Regards,
Damien
---
 gdbstub.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index 4cf8af365e..dde0cfe0fe 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1810,8 +1810,17 @@ static void handle_read_all_regs(GdbCmdContext *gdb_ctx, void *user_ctx)
     cpu_synchronize_state(gdb_ctx->s->g_cpu);
     len = 0;
     for (addr = 0; addr < gdb_ctx->s->g_cpu->gdb_num_g_regs; addr++) {
-        len += gdb_read_register(gdb_ctx->s->g_cpu, gdb_ctx->mem_buf + len,
-                                 addr);
+        int size = gdb_read_register(gdb_ctx->s->g_cpu, gdb_ctx->mem_buf + len,
+                                     addr);
+        if (len + size > MAX_PACKET_LENGTH / 2) {
+            /*
+             * Prevent gdb_ctx->str_buf overflow in memtohex() below.
+             * As a consequence, send only the first registers content.
+             * Gdb will query remaining ones if/when needed.
+             */
+            break;
+        }
+        len += size;
     }
 
     memtohex(gdb_ctx->str_buf, gdb_ctx->mem_buf, len);
-- 
2.24.0

Re: [PATCH] gdbstub: Fix buffer overflow in handle_read_all_regs

[Luc Michel]

On 11/8/19 1:55 PM, Damien Hedde wrote:
> Ensure we don't put too much register data in buffers. This avoids
> a buffer overflow (and stack corruption) when a target has lots
> of registers.
> 
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
> 
> Hi all,
> 
> While working on a target with many registers. I found out the gdbstub
> may do buffer overflows when receiving a 'g' query (to read general
> registers). This patch prevents that.
> 
> Gdb is pretty happy with a partial set of registers and queries
> remaining registers one by one when needed.
> 
> Regards,
> Damien
> ---
>  gdbstub.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/gdbstub.c b/gdbstub.c
> index 4cf8af365e..dde0cfe0fe 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -1810,8 +1810,17 @@ static void handle_read_all_regs(GdbCmdContext *gdb_ctx, void *user_ctx)
>      cpu_synchronize_state(gdb_ctx->s->g_cpu);
>      len = 0;
>      for (addr = 0; addr < gdb_ctx->s->g_cpu->gdb_num_g_regs; addr++) {
> -        len += gdb_read_register(gdb_ctx->s->g_cpu, gdb_ctx->mem_buf + len,
> -                                 addr);
> +        int size = gdb_read_register(gdb_ctx->s->g_cpu, gdb_ctx->mem_buf + len,
> +                                     addr);
> +        if (len + size > MAX_PACKET_LENGTH / 2) {
> +            /*
> +             * Prevent gdb_ctx->str_buf overflow in memtohex() below.
> +             * As a consequence, send only the first registers content.
> +             * Gdb will query remaining ones if/when needed.
> +             */
I could not find this behaviour documented in the GDB remote protocol
documentation. However in the GDB source code (in process_g_packet() in
remote.c) :

  /* If this is smaller than we guessed the 'g' packet would be,
     update our records.  A 'g' reply that doesn't include a register's
     value implies either that the register is not available, or that
     the 'p' packet must be used.  */


So:

Reviewed-by: Luc Michel <luc.michel@greensocs.com>

> +            break;
> +        }
> +        len += size;
>      }
>  
>      memtohex(gdb_ctx->str_buf, gdb_ctx->mem_buf, len);
> 

Re: [PATCH] gdbstub: Fix buffer overflow in handle_read_all_regs

[Alex Bennée]

Damien Hedde <damien.hedde@greensocs.com> writes:

> Ensure we don't put too much register data in buffers. This avoids
> a buffer overflow (and stack corruption) when a target has lots
> of registers.
>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
>
> Hi all,
>
> While working on a target with many registers. I found out the gdbstub
> may do buffer overflows when receiving a 'g' query (to read general
> registers). This patch prevents that.
>
> Gdb is pretty happy with a partial set of registers and queries
> remaining registers one by one when needed.

Heh I was just looking at this code with regards to SVE (which can get
quite big).

>
> Regards,
> Damien
> ---
>  gdbstub.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index 4cf8af365e..dde0cfe0fe 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -1810,8 +1810,17 @@ static void handle_read_all_regs(GdbCmdContext *gdb_ctx, void *user_ctx)
>      cpu_synchronize_state(gdb_ctx->s->g_cpu);
>      len = 0;
>      for (addr = 0; addr < gdb_ctx->s->g_cpu->gdb_num_g_regs; addr++) {
> -        len += gdb_read_register(gdb_ctx->s->g_cpu, gdb_ctx->mem_buf + len,
> -                                 addr);
> +        int size = gdb_read_register(gdb_ctx->s->g_cpu, gdb_ctx->mem_buf + len,
> +                                     addr);
> +        if (len + size > MAX_PACKET_LENGTH / 2) {
> +            /*
> +             * Prevent gdb_ctx->str_buf overflow in memtohex() below.
> +             * As a consequence, send only the first registers content.
> +             * Gdb will query remaining ones if/when needed.
> +             */

Haven't we already potentially overflowed gdb_ctx->mem_buf though? I
suspect the better fix is for str_buf is to make it growable with
g_string and be able to handle arbitrary size conversions (unless the
spec limits us). But we still don't want a hostile gdbstub to be able to
spam memory by asking for registers that might be bigger than
MAX_PACKET_LENGTH bytes.

> +            break;
> +        }
> +        len += size;
>      }
>
>      memtohex(gdb_ctx->str_buf, gdb_ctx->mem_buf, len);


--
Alex Bennée

Re: [PATCH] gdbstub: Fix buffer overflow in handle_read_all_regs

[Damien Hedde]

On 11/8/19 3:09 PM, Alex Bennée wrote:
> 
> Damien Hedde <damien.hedde@greensocs.com> writes:
> 
>> Ensure we don't put too much register data in buffers. This avoids
>> a buffer overflow (and stack corruption) when a target has lots
>> of registers.
>>
>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>> ---
>>
>> Hi all,
>>
>> While working on a target with many registers. I found out the gdbstub
>> may do buffer overflows when receiving a 'g' query (to read general
>> registers). This patch prevents that.
>>
>> Gdb is pretty happy with a partial set of registers and queries
>> remaining registers one by one when needed.
> 
> Heh I was just looking at this code with regards to SVE (which can get
> quite big).

SVE ?

> 
>>
>> Regards,
>> Damien
>> ---
>>  gdbstub.c | 13 +++++++++++--
>>  1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/gdbstub.c b/gdbstub.c
>> index 4cf8af365e..dde0cfe0fe 100644
>> --- a/gdbstub.c
>> +++ b/gdbstub.c
>> @@ -1810,8 +1810,17 @@ static void handle_read_all_regs(GdbCmdContext *gdb_ctx, void *user_ctx)
>>      cpu_synchronize_state(gdb_ctx->s->g_cpu);
>>      len = 0;
>>      for (addr = 0; addr < gdb_ctx->s->g_cpu->gdb_num_g_regs; addr++) {
>> -        len += gdb_read_register(gdb_ctx->s->g_cpu, gdb_ctx->mem_buf + len,
>> -                                 addr);
>> +        int size = gdb_read_register(gdb_ctx->s->g_cpu, gdb_ctx->mem_buf + len,
>> +                                     addr);
>> +        if (len + size > MAX_PACKET_LENGTH / 2) {
>> +            /*
>> +             * Prevent gdb_ctx->str_buf overflow in memtohex() below.
>> +             * As a consequence, send only the first registers content.
>> +             * Gdb will query remaining ones if/when needed.
>> +             */
> 
> Haven't we already potentially overflowed gdb_ctx->mem_buf though? I
> suspect the better fix is for str_buf is to make it growable with
> g_string and be able to handle arbitrary size conversions (unless the
> spec limits us). But we still don't want a hostile gdbstub to be able to
> spam memory by asking for registers that might be bigger than
> MAX_PACKET_LENGTH bytes.

For gdb_ctx->mem_buf  it's ok because it has also a size of
MAX_PACKET_LENGTH. (assuming no single register can be bigger than
MAX_PACKET_LENGTH)
str_buf has a size of MAX_PACKET_LENGTH + 1

I'm not sure I've understood the second part but if we increase the size
of str_buf then we will need also a bigger packet buffer.

The size here only depends on what are the target declared registers, so
it depends only on the cpu target code.

> 
>> +            break;
>> +        }
>> +        len += size;
>>      }
>>
>>      memtohex(gdb_ctx->str_buf, gdb_ctx->mem_buf, len);
> 
> 
> --
> Alex Bennée
> 

Re: [PATCH] gdbstub: Fix buffer overflow in handle_read_all_regs

[Damien Hedde]

On 11/8/19 3:43 PM, Damien Hedde wrote:
> 
> 
> On 11/8/19 3:09 PM, Alex Bennée wrote:
>>
>> Damien Hedde <damien.hedde@greensocs.com> writes:
>>
>>> Ensure we don't put too much register data in buffers. This avoids
>>> a buffer overflow (and stack corruption) when a target has lots
>>> of registers.
>>>
>>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>>> ---
>>>
>>> Hi all,
>>>
>>> While working on a target with many registers. I found out the gdbstub
>>> may do buffer overflows when receiving a 'g' query (to read general
>>> registers). This patch prevents that.
>>>
>>> Gdb is pretty happy with a partial set of registers and queries
>>> remaining registers one by one when needed.
>>
>> Heh I was just looking at this code with regards to SVE (which can get
>> quite big).
> 
> SVE ?
> 
>>
>>>
>>> Regards,
>>> Damien
>>> ---
>>>  gdbstub.c | 13 +++++++++++--
>>>  1 file changed, 11 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/gdbstub.c b/gdbstub.c
>>> index 4cf8af365e..dde0cfe0fe 100644
>>> --- a/gdbstub.c
>>> +++ b/gdbstub.c
>>> @@ -1810,8 +1810,17 @@ static void handle_read_all_regs(GdbCmdContext *gdb_ctx, void *user_ctx)
>>>      cpu_synchronize_state(gdb_ctx->s->g_cpu);
>>>      len = 0;
>>>      for (addr = 0; addr < gdb_ctx->s->g_cpu->gdb_num_g_regs; addr++) {
>>> -        len += gdb_read_register(gdb_ctx->s->g_cpu, gdb_ctx->mem_buf + len,
>>> -                                 addr);
>>> +        int size = gdb_read_register(gdb_ctx->s->g_cpu, gdb_ctx->mem_buf + len,
>>> +                                     addr);
>>> +        if (len + size > MAX_PACKET_LENGTH / 2) {
>>> +            /*
>>> +             * Prevent gdb_ctx->str_buf overflow in memtohex() below.
>>> +             * As a consequence, send only the first registers content.
>>> +             * Gdb will query remaining ones if/when needed.
>>> +             */
>>
>> Haven't we already potentially overflowed gdb_ctx->mem_buf though? I
>> suspect the better fix is for str_buf is to make it growable with
>> g_string and be able to handle arbitrary size conversions (unless the
>> spec limits us). But we still don't want a hostile gdbstub to be able to
>> spam memory by asking for registers that might be bigger than
>> MAX_PACKET_LENGTH bytes.
> 
> For gdb_ctx->mem_buf  it's ok because it has also a size of
> MAX_PACKET_LENGTH. (assuming no single register can be bigger than
> MAX_PACKET_LENGTH)
sorry, it is MAX_PACKET_LENGTH / 2 for the max single register allowed size.

> str_buf has a size of MAX_PACKET_LENGTH + 1
> 
> I'm not sure I've understood the second part but if we increase the size
> of str_buf then we will need also a bigger packet buffer.
> 
> The size here only depends on what are the target declared registers, so
> it depends only on the cpu target code.
> 
>>
>>> +            break;
>>> +        }
>>> +        len += size;
>>>      }
>>>
>>>      memtohex(gdb_ctx->str_buf, gdb_ctx->mem_buf, len);
>>
>>
>> --
>> Alex Bennée
>>
> 

Re: [PATCH] gdbstub: Fix buffer overflow in handle_read_all_regs

[Alex Bennée]

Damien Hedde <damien.hedde@greensocs.com> writes:

> On 11/8/19 3:09 PM, Alex Bennée wrote:
>>
>> Damien Hedde <damien.hedde@greensocs.com> writes:
>>
>>> Ensure we don't put too much register data in buffers. This avoids
>>> a buffer overflow (and stack corruption) when a target has lots
>>> of registers.
>>>
>>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>>> ---
>>>
>>> Hi all,
>>>
>>> While working on a target with many registers. I found out the gdbstub
>>> may do buffer overflows when receiving a 'g' query (to read general
>>> registers). This patch prevents that.
>>>
>>> Gdb is pretty happy with a partial set of registers and queries
>>> remaining registers one by one when needed.
>>
>> Heh I was just looking at this code with regards to SVE (which can get
>> quite big).
>
> SVE ?

ARM's Scalable Vector Registers which currently can get upto 16 vector
quads (256 bytes) but are likely to get bigger.

>
>>
>>>
>>> Regards,
>>> Damien
>>> ---
>>>  gdbstub.c | 13 +++++++++++--
>>>  1 file changed, 11 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/gdbstub.c b/gdbstub.c
>>> index 4cf8af365e..dde0cfe0fe 100644
>>> --- a/gdbstub.c
>>> +++ b/gdbstub.c
>>> @@ -1810,8 +1810,17 @@ static void handle_read_all_regs(GdbCmdContext *gdb_ctx, void *user_ctx)
>>>      cpu_synchronize_state(gdb_ctx->s->g_cpu);
>>>      len = 0;
>>>      for (addr = 0; addr < gdb_ctx->s->g_cpu->gdb_num_g_regs; addr++) {
>>> -        len += gdb_read_register(gdb_ctx->s->g_cpu, gdb_ctx->mem_buf + len,
>>> -                                 addr);
>>> +        int size = gdb_read_register(gdb_ctx->s->g_cpu, gdb_ctx->mem_buf + len,
>>> +                                     addr);
>>> +        if (len + size > MAX_PACKET_LENGTH / 2) {
>>> +            /*
>>> +             * Prevent gdb_ctx->str_buf overflow in memtohex() below.
>>> +             * As a consequence, send only the first registers content.
>>> +             * Gdb will query remaining ones if/when needed.
>>> +             */
>>
>> Haven't we already potentially overflowed gdb_ctx->mem_buf though? I
>> suspect the better fix is for str_buf is to make it growable with
>> g_string and be able to handle arbitrary size conversions (unless the
>> spec limits us). But we still don't want a hostile gdbstub to be able to
>> spam memory by asking for registers that might be bigger than
>> MAX_PACKET_LENGTH bytes.
>
> For gdb_ctx->mem_buf  it's ok because it has also a size of
> MAX_PACKET_LENGTH. (assuming no single register can be bigger than
> MAX_PACKET_LENGTH)
> str_buf has a size of MAX_PACKET_LENGTH + 1

Are these limits of the protocol rather than our own internal limits?

> I'm not sure I've understood the second part but if we increase the size
> of str_buf then we will need also a bigger packet buffer.

Glib provides some nice functions for managing arbitrary sized strings
in a nice flexible way which grow on demand. There is also a nice
growable GByteArray type which we can use for the packet buffer. I think
I'd started down this road of re-factoring but never got around to
posting the patches.

> The size here only depends on what are the target declared registers, so
> it depends only on the cpu target code.

Sure - but guest registers are growing all the time!

--
Alex Bennée

Re: [PATCH] gdbstub: Fix buffer overflow in handle_read_all_regs

[Damien Hedde]

On 11/8/19 5:50 PM, Alex Bennée wrote:
> 
> Damien Hedde <damien.hedde@greensocs.com> writes:
> 
>> On 11/8/19 3:09 PM, Alex Bennée wrote:
>>>
>>> Damien Hedde <damien.hedde@greensocs.com> writes:
>>>
>>>> Ensure we don't put too much register data in buffers. This avoids
>>>> a buffer overflow (and stack corruption) when a target has lots
>>>> of registers.
>>>>
>>>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>>>> ---
>>>>
>>>> Hi all,
>>>>
>>>> While working on a target with many registers. I found out the gdbstub
>>>> may do buffer overflows when receiving a 'g' query (to read general
>>>> registers). This patch prevents that.
>>>>
>>>> Gdb is pretty happy with a partial set of registers and queries
>>>> remaining registers one by one when needed.
>>>
>>> Heh I was just looking at this code with regards to SVE (which can get
>>> quite big).
>>
>> SVE ?
> 
> ARM's Scalable Vector Registers which currently can get upto 16 vector
> quads (256 bytes) but are likely to get bigger.
> 
>>
>>>
>>>>
>>>> Regards,
>>>> Damien
>>>> ---
>>>>  gdbstub.c | 13 +++++++++++--
>>>>  1 file changed, 11 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/gdbstub.c b/gdbstub.c
>>>> index 4cf8af365e..dde0cfe0fe 100644
>>>> --- a/gdbstub.c
>>>> +++ b/gdbstub.c
>>>> @@ -1810,8 +1810,17 @@ static void handle_read_all_regs(GdbCmdContext *gdb_ctx, void *user_ctx)
>>>>      cpu_synchronize_state(gdb_ctx->s->g_cpu);
>>>>      len = 0;
>>>>      for (addr = 0; addr < gdb_ctx->s->g_cpu->gdb_num_g_regs; addr++) {
>>>> -        len += gdb_read_register(gdb_ctx->s->g_cpu, gdb_ctx->mem_buf + len,
>>>> -                                 addr);
>>>> +        int size = gdb_read_register(gdb_ctx->s->g_cpu, gdb_ctx->mem_buf + len,
>>>> +                                     addr);
>>>> +        if (len + size > MAX_PACKET_LENGTH / 2) {
>>>> +            /*
>>>> +             * Prevent gdb_ctx->str_buf overflow in memtohex() below.
>>>> +             * As a consequence, send only the first registers content.
>>>> +             * Gdb will query remaining ones if/when needed.
>>>> +             */
>>>
>>> Haven't we already potentially overflowed gdb_ctx->mem_buf though? I
>>> suspect the better fix is for str_buf is to make it growable with
>>> g_string and be able to handle arbitrary size conversions (unless the
>>> spec limits us). But we still don't want a hostile gdbstub to be able to
>>> spam memory by asking for registers that might be bigger than
>>> MAX_PACKET_LENGTH bytes.
>>
>> For gdb_ctx->mem_buf  it's ok because it has also a size of
>> MAX_PACKET_LENGTH. (assuming no single register can be bigger than
>> MAX_PACKET_LENGTH)
>> str_buf has a size of MAX_PACKET_LENGTH + 1
> 
> Are these limits of the protocol rather than our own internal limits?

gdb has a dynamic sized packet buffer. Remote protocol doc says:

‘qSupported [:gdbfeature [;gdbfeature]… ]’
    [...] Any GDB which sends a ‘qSupported’ packet supports receiving
packets of unlimited length (earlier versions of GDB may reject overly
long responses).


> 
>> I'm not sure I've understood the second part but if we increase the size
>> of str_buf then we will need also a bigger packet buffer.
> 
> Glib provides some nice functions for managing arbitrary sized strings
> in a nice flexible way which grow on demand. There is also a nice
> growable GByteArray type which we can use for the packet buffer. I think
> I'd started down this road of re-factoring but never got around to
> posting the patches.
> 
>> The size here only depends on what are the target declared registers, so
>> it depends only on the cpu target code.
> 
> Sure - but guest registers are growing all the time!
> 
> --
> Alex Bennée
> 

Re: [PATCH] gdbstub: Fix buffer overflow in handle_read_all_regs

[Alex Bennée]

Damien Hedde <damien.hedde@greensocs.com> writes:

> On 11/8/19 5:50 PM, Alex Bennée wrote:
>>
>> Damien Hedde <damien.hedde@greensocs.com> writes:
>>
>>> On 11/8/19 3:09 PM, Alex Bennée wrote:
>>>>
>>>> Damien Hedde <damien.hedde@greensocs.com> writes:
>>>>
>>>>> Ensure we don't put too much register data in buffers. This avoids
>>>>> a buffer overflow (and stack corruption) when a target has lots
>>>>> of registers.
>>>>>
>>>>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>>>>> ---
>>>>>
>>>>> Hi all,
>>>>>
>>>>> While working on a target with many registers. I found out the gdbstub
>>>>> may do buffer overflows when receiving a 'g' query (to read general
>>>>> registers). This patch prevents that.
>>>>>
>>>>> Gdb is pretty happy with a partial set of registers and queries
>>>>> remaining registers one by one when needed.
>>>>
>>>> Heh I was just looking at this code with regards to SVE (which can get
>>>> quite big).
>>>
>>> SVE ?
>>
>> ARM's Scalable Vector Registers which currently can get upto 16 vector
>> quads (256 bytes) but are likely to get bigger.
>>
>>>
>>>>
>>>>>
>>>>> Regards,
>>>>> Damien
>>>>> ---
>>>>>  gdbstub.c | 13 +++++++++++--
>>>>>  1 file changed, 11 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/gdbstub.c b/gdbstub.c
>>>>> index 4cf8af365e..dde0cfe0fe 100644
>>>>> --- a/gdbstub.c
>>>>> +++ b/gdbstub.c
>>>>> @@ -1810,8 +1810,17 @@ static void handle_read_all_regs(GdbCmdContext *gdb_ctx, void *user_ctx)
>>>>>      cpu_synchronize_state(gdb_ctx->s->g_cpu);
>>>>>      len = 0;
>>>>>      for (addr = 0; addr < gdb_ctx->s->g_cpu->gdb_num_g_regs; addr++) {
>>>>> -        len += gdb_read_register(gdb_ctx->s->g_cpu, gdb_ctx->mem_buf + len,
>>>>> -                                 addr);
>>>>> +        int size = gdb_read_register(gdb_ctx->s->g_cpu, gdb_ctx->mem_buf + len,
>>>>> +                                     addr);
>>>>> +        if (len + size > MAX_PACKET_LENGTH / 2) {
>>>>> +            /*
>>>>> +             * Prevent gdb_ctx->str_buf overflow in memtohex() below.
>>>>> +             * As a consequence, send only the first registers content.
>>>>> +             * Gdb will query remaining ones if/when needed.
>>>>> +             */
>>>>
>>>> Haven't we already potentially overflowed gdb_ctx->mem_buf though? I
>>>> suspect the better fix is for str_buf is to make it growable with
>>>> g_string and be able to handle arbitrary size conversions (unless the
>>>> spec limits us). But we still don't want a hostile gdbstub to be able to
>>>> spam memory by asking for registers that might be bigger than
>>>> MAX_PACKET_LENGTH bytes.
>>>
>>> For gdb_ctx->mem_buf  it's ok because it has also a size of
>>> MAX_PACKET_LENGTH. (assuming no single register can be bigger than
>>> MAX_PACKET_LENGTH)
>>> str_buf has a size of MAX_PACKET_LENGTH + 1
>>
>> Are these limits of the protocol rather than our own internal limits?
>
> gdb has a dynamic sized packet buffer. Remote protocol doc says:
>
> ‘qSupported [:gdbfeature [;gdbfeature]… ]’
>     [...] Any GDB which sends a ‘qSupported’ packet supports receiving
> packets of unlimited length (earlier versions of GDB may reject overly
> long responses).

OK so it seems worth cleaning this up. I'm currently putting together a
patch set to support these large SVE registers and I'm cleaning up the
core gdbstub code while I go. If you are interested the current WIP
branch is:

  https://github.com/stsquad/qemu/commits/gdbstub/sve-registers

but I can include you on the review CC when I post (hopefully this
week)?

>
>
>>
>>> I'm not sure I've understood the second part but if we increase the size
>>> of str_buf then we will need also a bigger packet buffer.
>>
>> Glib provides some nice functions for managing arbitrary sized strings
>> in a nice flexible way which grow on demand. There is also a nice
>> growable GByteArray type which we can use for the packet buffer. I think
>> I'd started down this road of re-factoring but never got around to
>> posting the patches.
>>
>>> The size here only depends on what are the target declared registers, so
>>> it depends only on the cpu target code.
>>
>> Sure - but guest registers are growing all the time!
>>
>> --
>> Alex Bennée
>>


--
Alex Bennée

Re: [PATCH] gdbstub: Fix buffer overflow in handle_read_all_regs

[Damien Hedde]

On 11/14/19 2:47 PM, Alex Bennée wrote:
> 
> Damien Hedde <damien.hedde@greensocs.com> writes:
> 
>> On 11/8/19 5:50 PM, Alex Bennée wrote:
>>>
>>> Damien Hedde <damien.hedde@greensocs.com> writes:
>>>
>>>> On 11/8/19 3:09 PM, Alex Bennée wrote:
>>>>>
>>>>> Damien Hedde <damien.hedde@greensocs.com> writes:
>>>>>
>>>>>> Ensure we don't put too much register data in buffers. This avoids
>>>>>> a buffer overflow (and stack corruption) when a target has lots
>>>>>> of registers.
>>>>>>
>>>>>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>>>>>> ---
>>>>>>
>>>>>> Hi all,
>>>>>>
>>>>>> While working on a target with many registers. I found out the gdbstub
>>>>>> may do buffer overflows when receiving a 'g' query (to read general
>>>>>> registers). This patch prevents that.
>>>>>>
>>>>>> Gdb is pretty happy with a partial set of registers and queries
>>>>>> remaining registers one by one when needed.
>>>>>
>>>>> Heh I was just looking at this code with regards to SVE (which can get
>>>>> quite big).
>>>>
>>>> SVE ?
>>>
>>> ARM's Scalable Vector Registers which currently can get upto 16 vector
>>> quads (256 bytes) but are likely to get bigger.
>>>
>>>>
>>>>>
>>>>>>
>>>>>> Regards,
>>>>>> Damien
>>>>>> ---
>>>>>>  gdbstub.c | 13 +++++++++++--
>>>>>>  1 file changed, 11 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/gdbstub.c b/gdbstub.c
>>>>>> index 4cf8af365e..dde0cfe0fe 100644
>>>>>> --- a/gdbstub.c
>>>>>> +++ b/gdbstub.c
>>>>>> @@ -1810,8 +1810,17 @@ static void handle_read_all_regs(GdbCmdContext *gdb_ctx, void *user_ctx)
>>>>>>      cpu_synchronize_state(gdb_ctx->s->g_cpu);
>>>>>>      len = 0;
>>>>>>      for (addr = 0; addr < gdb_ctx->s->g_cpu->gdb_num_g_regs; addr++) {
>>>>>> -        len += gdb_read_register(gdb_ctx->s->g_cpu, gdb_ctx->mem_buf + len,
>>>>>> -                                 addr);
>>>>>> +        int size = gdb_read_register(gdb_ctx->s->g_cpu, gdb_ctx->mem_buf + len,
>>>>>> +                                     addr);
>>>>>> +        if (len + size > MAX_PACKET_LENGTH / 2) {
>>>>>> +            /*
>>>>>> +             * Prevent gdb_ctx->str_buf overflow in memtohex() below.
>>>>>> +             * As a consequence, send only the first registers content.
>>>>>> +             * Gdb will query remaining ones if/when needed.
>>>>>> +             */
>>>>>
>>>>> Haven't we already potentially overflowed gdb_ctx->mem_buf though? I
>>>>> suspect the better fix is for str_buf is to make it growable with
>>>>> g_string and be able to handle arbitrary size conversions (unless the
>>>>> spec limits us). But we still don't want a hostile gdbstub to be able to
>>>>> spam memory by asking for registers that might be bigger than
>>>>> MAX_PACKET_LENGTH bytes.
>>>>
>>>> For gdb_ctx->mem_buf  it's ok because it has also a size of
>>>> MAX_PACKET_LENGTH. (assuming no single register can be bigger than
>>>> MAX_PACKET_LENGTH)
>>>> str_buf has a size of MAX_PACKET_LENGTH + 1
>>>
>>> Are these limits of the protocol rather than our own internal limits?
>>
>> gdb has a dynamic sized packet buffer. Remote protocol doc says:
>>
>> ‘qSupported [:gdbfeature [;gdbfeature]… ]’
>>     [...] Any GDB which sends a ‘qSupported’ packet supports receiving
>> packets of unlimited length (earlier versions of GDB may reject overly
>> long responses).
> 
> OK so it seems worth cleaning this up. I'm currently putting together a
> patch set to support these large SVE registers and I'm cleaning up the
> core gdbstub code while I go. If you are interested the current WIP
> branch is:
> 
>   https://github.com/stsquad/qemu/commits/gdbstub/sve-registers
> 
> but I can include you on the review CC when I post (hopefully this
> week)?

Sure. I will review what I can.
> 
>>
>>
>>>
>>>> I'm not sure I've understood the second part but if we increase the size
>>>> of str_buf then we will need also a bigger packet buffer.
>>>
>>> Glib provides some nice functions for managing arbitrary sized strings
>>> in a nice flexible way which grow on demand. There is also a nice
>>> growable GByteArray type which we can use for the packet buffer. I think
>>> I'd started down this road of re-factoring but never got around to
>>> posting the patches.
>>>
>>>> The size here only depends on what are the target declared registers, so
>>>> it depends only on the cpu target code.
>>>
>>> Sure - but guest registers are growing all the time!
>>>
>>> --
>>> Alex Bennée
>>>
> 
> 
> --
> Alex Bennée
> 

--
Damien