* [PATCH] gdbstub: Fix buffer overflow in handle_read_all_regs
@ 2019-11-08 12:55 Damien Hedde
2019-11-08 13:40 ` Luc Michel
2019-11-08 14:09 ` Alex Bennée
0 siblings, 2 replies; 9+ messages in thread
From: Damien Hedde @ 2019-11-08 12:55 UTC (permalink / raw)
To: qemu-devel; +Cc: Damien Hedde, philmd, alex.bennee
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] gdbstub: Fix buffer overflow in handle_read_all_regs
2019-11-08 12:55 [PATCH] gdbstub: Fix buffer overflow in handle_read_all_regs Damien Hedde
@ 2019-11-08 13:40 ` Luc Michel
2019-11-08 14:09 ` Alex Bennée
1 sibling, 0 replies; 9+ messages in thread
From: Luc Michel @ 2019-11-08 13:40 UTC (permalink / raw)
To: Damien Hedde, qemu-devel; +Cc: alex.bennee, philmd
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);
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] gdbstub: Fix buffer overflow in handle_read_all_regs
2019-11-08 12:55 [PATCH] gdbstub: Fix buffer overflow in handle_read_all_regs Damien Hedde
2019-11-08 13:40 ` Luc Michel
@ 2019-11-08 14:09 ` Alex Bennée
2019-11-08 14:43 ` Damien Hedde
1 sibling, 1 reply; 9+ messages in thread
From: Alex Bennée @ 2019-11-08 14:09 UTC (permalink / raw)
To: Damien Hedde; +Cc: philmd, qemu-devel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] gdbstub: Fix buffer overflow in handle_read_all_regs
2019-11-08 14:09 ` Alex Bennée
@ 2019-11-08 14:43 ` Damien Hedde
2019-11-08 14:52 ` Damien Hedde
2019-11-08 16:50 ` Alex Bennée
0 siblings, 2 replies; 9+ messages in thread
From: Damien Hedde @ 2019-11-08 14:43 UTC (permalink / raw)
To: Alex Bennée; +Cc: philmd, qemu-devel
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
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] gdbstub: Fix buffer overflow in handle_read_all_regs
2019-11-08 14:43 ` Damien Hedde
@ 2019-11-08 14:52 ` Damien Hedde
2019-11-08 16:50 ` Alex Bennée
1 sibling, 0 replies; 9+ messages in thread
From: Damien Hedde @ 2019-11-08 14:52 UTC (permalink / raw)
To: Alex Bennée; +Cc: philmd, qemu-devel
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
>>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] gdbstub: Fix buffer overflow in handle_read_all_regs
2019-11-08 14:43 ` Damien Hedde
2019-11-08 14:52 ` Damien Hedde
@ 2019-11-08 16:50 ` Alex Bennée
2019-11-14 10:19 ` Damien Hedde
1 sibling, 1 reply; 9+ messages in thread
From: Alex Bennée @ 2019-11-08 16:50 UTC (permalink / raw)
To: Damien Hedde; +Cc: philmd, qemu-devel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] gdbstub: Fix buffer overflow in handle_read_all_regs
2019-11-08 16:50 ` Alex Bennée
@ 2019-11-14 10:19 ` Damien Hedde
2019-11-14 13:47 ` Alex Bennée
0 siblings, 1 reply; 9+ messages in thread
From: Damien Hedde @ 2019-11-14 10:19 UTC (permalink / raw)
To: Alex Bennée; +Cc: philmd, qemu-devel, Luc Michel
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
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] gdbstub: Fix buffer overflow in handle_read_all_regs
2019-11-14 10:19 ` Damien Hedde
@ 2019-11-14 13:47 ` Alex Bennée
2019-11-14 14:43 ` Damien Hedde
0 siblings, 1 reply; 9+ messages in thread
From: Alex Bennée @ 2019-11-14 13:47 UTC (permalink / raw)
To: Damien Hedde; +Cc: philmd, qemu-devel, Luc Michel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] gdbstub: Fix buffer overflow in handle_read_all_regs
2019-11-14 13:47 ` Alex Bennée
@ 2019-11-14 14:43 ` Damien Hedde
0 siblings, 0 replies; 9+ messages in thread
From: Damien Hedde @ 2019-11-14 14:43 UTC (permalink / raw)
To: Alex Bennée; +Cc: philmd, qemu-devel, Luc Michel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-11-14 14:47 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-08 12:55 [PATCH] gdbstub: Fix buffer overflow in handle_read_all_regs Damien Hedde
2019-11-08 13:40 ` Luc Michel
2019-11-08 14:09 ` Alex Bennée
2019-11-08 14:43 ` Damien Hedde
2019-11-08 14:52 ` Damien Hedde
2019-11-08 16:50 ` Alex Bennée
2019-11-14 10:19 ` Damien Hedde
2019-11-14 13:47 ` Alex Bennée
2019-11-14 14:43 ` Damien Hedde
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).