Message ID | 20191108125534.114474-1-damien.hedde@greensocs.com |
---|---|
State | New |
Headers | show |
Series | gdbstub: Fix buffer overflow in handle_read_all_regs | expand |
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); >
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
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 >
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 >> >
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
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 >
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
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
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);
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(-)