diff mbox series

[RFC] gdbstub: Avoid NULL dereference in gdb_handle_packet()

Message ID 20190118112213.11173-1-philmd@redhat.com
State New
Headers show
Series [RFC] gdbstub: Avoid NULL dereference in gdb_handle_packet() | expand

Commit Message

Philippe Mathieu-Daudé Jan. 18, 2019, 11:22 a.m. UTC 
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(+)

Comments

Luc Michel Jan. 19, 2019, 2:50 p.m. UTC | #1 
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.
no-reply@patchew.org Feb. 2, 2019, 6:26 p.m. UTC | #2 
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
diff mbox series

Patch

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) {