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

[Damien Hedde <damien.hedde@greensocs.com>]

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
>