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

[P J P]

+-- 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