The "Hg" GDB packet is used to select the current thread, and can fail.
GDB doesn't not check for failure and emits further packets that can
access and dereference s->c_cpu or s->g_cpu.
Add a check that returns "E22" (EINVAL) when those pointers are not set.
Peter Maydell reported:
GDB doesn't check the "Hg" packet failures and emit a
"qXfer:features:read" packet which accesses th
Looking at the protocol trace from the gdb end:
(gdb) set debug remote 1
(gdb) target remote :1234
Remote debugging using :1234
Sending packet:
$qSupported:multiprocess+;swbreak+;hwbreak+;qRelocInsn+;fork-events+;vfork-events+;exec-events+;vContSupported+;QThreadEvents+;no-resumed+;xmlRegisters=i386#6a...Ack
Packet received: PacketSize=1000;qXfer:features:read+;multiprocess+
Packet qSupported (supported-packets) is supported
Sending packet: $vMustReplyEmpty#3a...Ack
Packet received:
Sending packet: $Hgp0.0#ad...Ack
Packet received: E22
Sending packet: $qXfer:features:read:target.xml:0,ffb#79...Ack
[here is where QEMU crashes]
What seems to happen is that GDB's attempt to set the
current thread with the "Hg" command fails because
gdb_get_cpu() fails, and so we return an E22 error status.
GDB (incorrectly) ignores this and issues a general command
anyway, and then QEMU crashes because we don't handle s->g_cpu
being NULL in this command's handling code.
Fixes: c145eeae1cc
Reported-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
RFC because these are many if..break but I can't think of a cleaner way
today.
The protocol isn't specific about the error, can it be "E03" for ESRCH
(No such process)?
---
gdbstub.c | 40 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 40 insertions(+)
diff --git a/gdbstub.c b/gdbstub.c
index bfc7afb509..480005b094 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1306,6 +1306,10 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
put_packet(s, "OK");
break;
case '?':
+ if (!s->c_cpu) {
+ put_packet(s, "E22");
+ break;
+ }
/* TODO: Make this return the correct value for user-mode. */
snprintf(buf, sizeof(buf), "T%02xthread:%s;", GDB_SIGNAL_TRAP,
gdb_fmt_thread_id(s, s->c_cpu, thread_id, sizeof(thread_id)));
@@ -1394,6 +1398,10 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
/* Detach packet */
pid = 1;
+ if (!s->c_cpu || !s->g_cpu) {
+ put_packet(s, "E22");
+ break;
+ }
if (s->multiprocess) {
unsigned long lpid;
if (*p != ';') {
@@ -1429,6 +1437,10 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
put_packet(s, "OK");
break;
case 's':
+ if (!s->s_cpu) {
+ put_packet(s, "E22");
+ break;
+ }
if (*p != '\0') {
addr = strtoull(p, (char **)&p, 16);
gdb_set_cpu_pc(s, addr);
@@ -1441,6 +1453,10 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
target_ulong ret;
target_ulong err;
+ if (!s->s_cpu) {
+ put_packet(s, "E22");
+ break;
+ }
ret = strtoull(p, (char **)&p, 16);
if (*p == ',') {
p++;
@@ -1463,6 +1479,10 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
}
break;
case 'g':
+ if (!s->g_cpu) {
+ put_packet(s, "E22");
+ break;
+ }
cpu_synchronize_state(s->g_cpu);
len = 0;
for (addr = 0; addr < s->g_cpu->gdb_num_g_regs; addr++) {
@@ -1473,6 +1493,10 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
put_packet(s, buf);
break;
case 'G':
+ if (!s->g_cpu) {
+ put_packet(s, "E22");
+ break;
+ }
cpu_synchronize_state(s->g_cpu);
registers = mem_buf;
len = strlen(p) / 2;
@@ -1485,6 +1509,10 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
put_packet(s, "OK");
break;
case 'm':
+ if (!s->g_cpu) {
+ put_packet(s, "E22");
+ break;
+ }
addr = strtoull(p, (char **)&p, 16);
if (*p == ',')
p++;
@@ -1504,6 +1532,10 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
}
break;
case 'M':
+ if (!s->g_cpu) {
+ put_packet(s, "E22");
+ break;
+ }
addr = strtoull(p, (char **)&p, 16);
if (*p == ',')
p++;
@@ -1642,6 +1674,10 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
put_packet(s, "OK");
break;
} else if (strcmp(p,"C") == 0) {
+ if (!s->g_cpu) {
+ put_packet(s, "E22");
+ break;
+ }
/*
* "Current thread" remains vague in the spec, so always return
* the first thread of the current process (gdb returns the
@@ -1745,6 +1781,10 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
const char *xml;
target_ulong total_len;
+ if (!s->g_cpu) {
+ put_packet(s, "E22");
+ break;
+ }
process = gdb_get_cpu_process(s, s->g_cpu);
cc = CPU_GET_CLASS(s->g_cpu);
if (cc->gdb_core_xml_file == NULL) {
--
2.17.2
On 1/18/19 12:22 PM, Philippe Mathieu-Daudé wrote: > The "Hg" GDB packet is used to select the current thread, and can fail. > GDB doesn't not check for failure and emits further packets that can > access and dereference s->c_cpu or s->g_cpu. > > Add a check that returns "E22" (EINVAL) when those pointers are not set. > > Peter Maydell reported: > GDB doesn't check the "Hg" packet failures and emit a > "qXfer:features:read" packet which accesses th > Looking at the protocol trace from the gdb end: > (gdb) set debug remote 1 > (gdb) target remote :1234 > Remote debugging using :1234 > Sending packet: > $qSupported:multiprocess+;swbreak+;hwbreak+;qRelocInsn+;fork-events+;vfork-events+;exec-events+;vContSupported+;QThreadEvents+;no-resumed+;xmlRegisters=i386#6a...Ack > Packet received: PacketSize=1000;qXfer:features:read+;multiprocess+ > Packet qSupported (supported-packets) is supported > Sending packet: $vMustReplyEmpty#3a...Ack > Packet received: > Sending packet: $Hgp0.0#ad...Ack > Packet received: E22 > Sending packet: $qXfer:features:read:target.xml:0,ffb#79...Ack > [here is where QEMU crashes] > > What seems to happen is that GDB's attempt to set the > current thread with the "Hg" command fails because > gdb_get_cpu() fails, and so we return an E22 error status. > GDB (incorrectly) ignores this and issues a general command > anyway, and then QEMU crashes because we don't handle s->g_cpu > being NULL in this command's handling code. > > Fixes: c145eeae1cc > Reported-by: Peter Maydell <peter.maydell@linaro.org> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > RFC because these are many if..break but I can't think of a cleaner way > today. I can't think of a better way without an important refactoring of gdbstub.c. > The protocol isn't specific about the error, can it be "E03" for ESRCH > (No such process)? > --- > gdbstub.c | 40 ++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 40 insertions(+) > > diff --git a/gdbstub.c b/gdbstub.c > index bfc7afb509..480005b094 100644 > --- a/gdbstub.c > +++ b/gdbstub.c > @@ -1306,6 +1306,10 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf) > put_packet(s, "OK"); > break; > case '?': > + if (!s->c_cpu) { > + put_packet(s, "E22"); > + break; > + } > /* TODO: Make this return the correct value for user-mode. */ > snprintf(buf, sizeof(buf), "T%02xthread:%s;", GDB_SIGNAL_TRAP, > gdb_fmt_thread_id(s, s->c_cpu, thread_id, sizeof(thread_id))); > @@ -1394,6 +1398,10 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf) > /* Detach packet */ > pid = 1; > > + if (!s->c_cpu || !s->g_cpu) { > + put_packet(s, "E22"); > + break; > + } Even though this is probably true (s->[gc]_cpu == NULL probably means "no process attached"), the detach packet in itself carries the PID to detach from. So I would go for a code hardening, something like: process = gdb_get_process(s, pid); + if (!process->attached) { + put_packet(s, "E22"); + break; + } gdb_process_breakpoint_remove_all(s, process); process->attached = false; - if (pid == gdb_get_cpu_pid(s, s->c_cpu)) { + if (s->c_cpu && pid == gdb_get_cpu_pid(s, s->c_cpu)) { s->c_cpu = gdb_first_attached_cpu(s); } - if (pid == gdb_get_cpu_pid(s, s->g_cpu)) { + if (s->g_cpu && pid == gdb_get_cpu_pid(s, s->g_cpu)) { s->g_cpu = gdb_first_attached_cpu(s); } > if (s->multiprocess) { > unsigned long lpid; > if (*p != ';') { > @@ -1429,6 +1437,10 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf) > put_packet(s, "OK"); > break; > case 's': > + if (!s->s_cpu) { if (!s->c_cpu) { > + put_packet(s, "E22"); > + break; > + } > if (*p != '\0') { > addr = strtoull(p, (char **)&p, 16); > gdb_set_cpu_pc(s, addr); > @@ -1441,6 +1453,10 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf) > target_ulong ret; > target_ulong err; > > + if (!s->s_cpu) { if (!s->c_cpu) { > + put_packet(s, "E22"); > + break; > + } > ret = strtoull(p, (char **)&p, 16); > if (*p == ',') { > p++; > @@ -1463,6 +1479,10 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf) > } > break; > case 'g': > + if (!s->g_cpu) { > + put_packet(s, "E22"); > + break; > + } > cpu_synchronize_state(s->g_cpu); > len = 0; > for (addr = 0; addr < s->g_cpu->gdb_num_g_regs; addr++) { > @@ -1473,6 +1493,10 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf) > put_packet(s, buf); > break; > case 'G': > + if (!s->g_cpu) { > + put_packet(s, "E22"); > + break; > + } > cpu_synchronize_state(s->g_cpu); > registers = mem_buf; > len = strlen(p) / 2; > @@ -1485,6 +1509,10 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf) > put_packet(s, "OK"); > break; > case 'm': > + if (!s->g_cpu) { > + put_packet(s, "E22"); > + break; > + } > addr = strtoull(p, (char **)&p, 16); > if (*p == ',') > p++; > @@ -1504,6 +1532,10 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf) > } > break; > case 'M': > + if (!s->g_cpu) { > + put_packet(s, "E22"); > + break; > + } > addr = strtoull(p, (char **)&p, 16); > if (*p == ',') > p++; > @@ -1642,6 +1674,10 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf) > put_packet(s, "OK"); > break; > } else if (strcmp(p,"C") == 0) { > + if (!s->g_cpu) { > + put_packet(s, "E22"); > + break; > + } > /* > * "Current thread" remains vague in the spec, so always return > * the first thread of the current process (gdb returns the > @@ -1745,6 +1781,10 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf) > const char *xml; > target_ulong total_len; > > + if (!s->g_cpu) { > + put_packet(s, "E22"); > + break; > + } > process = gdb_get_cpu_process(s, s->g_cpu); > cc = CPU_GET_CLASS(s->g_cpu); > if (cc->gdb_core_xml_file == NULL) { > I think you are missing the check for 'p' and 'P' packets, and for the qOffsets packet in user mode (around line 1701). Thanks. -- Luc
Patchew URL: https://patchew.org/QEMU/20190118112213.11173-1-philmd@redhat.com/ Hi, This series failed the docker-mingw@fedora build test. Please find the testing commands and their output below. If you have Docker installed, you can probably reproduce it locally. === TEST SCRIPT BEGIN === #!/bin/bash time make docker-test-mingw@fedora SHOW_ENV=1 J=14 === TEST SCRIPT END === CC x86_64-softmmu/memory.o CC x86_64-softmmu/memory_mapping.o /tmp/qemu-test/src/gdbstub.c: In function 'gdb_handle_packet': /tmp/qemu-test/src/gdbstub.c:1440:17: error: 'GDBState' {aka 'struct GDBState'} has no member named 's_cpu'; did you mean 'c_cpu'? if (!s->s_cpu) { ^~~~~ c_cpu /tmp/qemu-test/src/gdbstub.c:1456:21: error: 'GDBState' {aka 'struct GDBState'} has no member named 's_cpu'; did you mean 'c_cpu'? if (!s->s_cpu) { ^~~~~ c_cpu --- CC aarch64-softmmu/memory.o CC aarch64-softmmu/memory_mapping.o /tmp/qemu-test/src/gdbstub.c: In function 'gdb_handle_packet': /tmp/qemu-test/src/gdbstub.c:1440:17: error: 'GDBState' {aka 'struct GDBState'} has no member named 's_cpu'; did you mean 'c_cpu'? if (!s->s_cpu) { ^~~~~ c_cpu /tmp/qemu-test/src/gdbstub.c:1456:21: error: 'GDBState' {aka 'struct GDBState'} has no member named 's_cpu'; did you mean 'c_cpu'? if (!s->s_cpu) { ^~~~~ c_cpu The full log is available at http://patchew.org/logs/20190118112213.11173-1-philmd@redhat.com/testing.docker-mingw@fedora/?type=message. --- Email generated automatically by Patchew [http://patchew.org/]. Please send your feedback to patchew-devel@redhat.com
© 2016 - 2024 Red Hat, Inc.