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