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 <address@hidden> writes:
> 
>> On 11/8/19 5:50 PM, Alex Bennée wrote:
>>>
>>> Damien Hedde <address@hidden> writes:
>>>
>>>> On 11/8/19 3:09 PM, Alex Bennée wrote:
>>>>>
>>>>> Damien Hedde <address@hidden> 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 <address@hidden>
>>>>>> ---
>>>>>>
>>>>>> 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