gdbstub: Fix buffer overflows in gdb_handle_packet()

Some places in gdb_handle_packet() can get an arbitrary length (most
times directly from the client) and either didn't check it at all or
checked against the wrong value, potentially causing buffer overflows.

Cc: qemu-stable@nongnu.org
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 gdbstub.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

+-- On Tue, 13 Oct 2015, Kevin Wolf wrote --+
| diff --git a/gdbstub.c b/gdbstub.c
| index d2c95b5..9c29aa0 100644
| --- a/gdbstub.c
| +++ b/gdbstub.c
| @@ -956,6 +956,13 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
|          if (*p == ',')
|              p++;
|          len = strtoull(p, NULL, 16);
| +
| +        /* memtohex() doubles the required space */
| +        if (len > MAX_PACKET_LENGTH / 2) {
| +            put_packet (s, "E22");
| +            break;
| +        }
| +
|          if (target_memory_rw_debug(s->g_cpu, addr, mem_buf, len, false) != 0) {
|              put_packet (s, "E14");
|          } else {
| @@ -970,6 +977,12 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
|          len = strtoull(p, (char **)&p, 16);
|          if (*p == ':')
|              p++;
| +
| +        /* hextomem() reads 2*len bytes */
| +        if (len > strlen(p) / 2) {
| +            put_packet (s, "E22");
| +            break;
| +        }
|          hextomem(mem_buf, p, len);
|          if (target_memory_rw_debug(s->g_cpu, addr, mem_buf, len,
|                                     true) != 0) {
| @@ -1107,7 +1120,8 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
|              cpu = find_cpu(thread);
|              if (cpu != NULL) {
|                  cpu_synchronize_state(cpu);
| -                len = snprintf((char *)mem_buf, sizeof(mem_buf),
| +                /* memtohex() doubles the required space */
| +                len = snprintf((char *)mem_buf, sizeof(buf) / 2,
|                                 "CPU#%d [%s]", cpu->cpu_index,
|                                 cpu->halted ? "halted " : "running");
|                  memtohex(buf, mem_buf, len);
| @@ -1136,8 +1150,8 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
|                  put_packet(s, "E01");
|                  break;
|              }
| -            hextomem(mem_buf, p + 5, len);
|              len = len / 2;
| +            hextomem(mem_buf, p + 5, len);
|              mem_buf[len++] = 0;
|              qemu_chr_be_write(s->mon_chr, mem_buf, len);
|              put_packet(s, "OK");
| 

Yes, patch looks good to fix the said buffer overflows.

Nevertheless, I wonder how hextomem() & memtohex() routines help? Because IIUC 
they seem to be changing command semantics. Ie host gdb(1) user would need to 
supply len/2 value to read/write 'len' bytes.

--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Am 14.10.2015 um 08:53 hat P J P geschrieben:
> +-- On Tue, 13 Oct 2015, Kevin Wolf wrote --+
> | diff --git a/gdbstub.c b/gdbstub.c
> | index d2c95b5..9c29aa0 100644
> | --- a/gdbstub.c
> | +++ b/gdbstub.c
> | @@ -956,6 +956,13 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
> |          if (*p == ',')
> |              p++;
> |          len = strtoull(p, NULL, 16);
> | +
> | +        /* memtohex() doubles the required space */
> | +        if (len > MAX_PACKET_LENGTH / 2) {
> | +            put_packet (s, "E22");
> | +            break;
> | +        }
> | +
> |          if (target_memory_rw_debug(s->g_cpu, addr, mem_buf, len, false) != 0) {
> |              put_packet (s, "E14");
> |          } else {
> | @@ -970,6 +977,12 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
> |          len = strtoull(p, (char **)&p, 16);
> |          if (*p == ':')
> |              p++;
> | +
> | +        /* hextomem() reads 2*len bytes */
> | +        if (len > strlen(p) / 2) {
> | +            put_packet (s, "E22");
> | +            break;
> | +        }
> |          hextomem(mem_buf, p, len);
> |          if (target_memory_rw_debug(s->g_cpu, addr, mem_buf, len,
> |                                     true) != 0) {
> | @@ -1107,7 +1120,8 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
> |              cpu = find_cpu(thread);
> |              if (cpu != NULL) {
> |                  cpu_synchronize_state(cpu);
> | -                len = snprintf((char *)mem_buf, sizeof(mem_buf),
> | +                /* memtohex() doubles the required space */
> | +                len = snprintf((char *)mem_buf, sizeof(buf) / 2,
> |                                 "CPU#%d [%s]", cpu->cpu_index,
> |                                 cpu->halted ? "halted " : "running");
> |                  memtohex(buf, mem_buf, len);
> | @@ -1136,8 +1150,8 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
> |                  put_packet(s, "E01");
> |                  break;
> |              }
> | -            hextomem(mem_buf, p + 5, len);
> |              len = len / 2;
> | +            hextomem(mem_buf, p + 5, len);
> |              mem_buf[len++] = 0;
> |              qemu_chr_be_write(s->mon_chr, mem_buf, len);
> |              put_packet(s, "OK");
> | 
> 
> Yes, patch looks good to fix the said buffer overflows.
> 
> Nevertheless, I wonder how hextomem() & memtohex() routines help? Because IIUC 
> they seem to be changing command semantics. Ie host gdb(1) user would need to 
> supply len/2 value to read/write 'len' bytes.

That's just how the gdb protocol works. Binary data is transferred as a
string of hex digits, with every byte being represented by two digits.
The requested length is in bytes of binary data, so the length of the
actually transferred data on the wire is indeed double that length.

Kevin

+-- On Wed, 14 Oct 2015, Kevin Wolf wrote --+
| >  Ie host gdb(1) user would need to 
| > supply len/2 value to read/write 'len' bytes.
| 
| That's just how the gdb protocol works. Binary data is transferred as a
| string of hex digits, with every byte being represented by two digits.
| The requested length is in bytes of binary data, so the length of the
| actually transferred data on the wire is indeed double that length.

  I see, got it.

Thank you.
--
 - P J P
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

13.10.2015 10:38, Kevin Wolf wrote:
> Some places in gdb_handle_packet() can get an arbitrary length (most
> times directly from the client) and either didn't check it at all or
> checked against the wrong value, potentially causing buffer overflows.

Applied to -trivial, thank you!

/mjt