Re: [Qemu-devel] [PATCH] gdbstub: Fix buffer overflows in gdb_handle_packet()

[Kevin Wolf]

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