[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC PATCH] gdbstub: Avoid NULL dereference in gdb_hand
From: |
Luc Michel |
Subject: |
Re: [Qemu-devel] [RFC PATCH] gdbstub: Avoid NULL dereference in gdb_handle_packet() |
Date: |
Sat, 19 Jan 2019 15:50:00 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.4.0 |
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 <address@hidden>
> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
> ---
> 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
signature.asc
Description: OpenPGP digital signature