From patchwork Fri Jan 18 11:22:13 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Philippe_Mathieu-Daud=C3=A9?= X-Patchwork-Id: 1027219 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=nongnu.org (client-ip=209.51.188.17; helo=lists.gnu.org; envelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org; receiver=) Authentication-Results: ozlabs.org; dmarc=fail (p=none dis=none) header.from=redhat.com Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 43gz843pwWz9sBQ for ; Fri, 18 Jan 2019 22:22:59 +1100 (AEDT) Received: from localhost ([127.0.0.1]:36583 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gkSEj-0007Fm-D6 for incoming@patchwork.ozlabs.org; Fri, 18 Jan 2019 06:22:57 -0500 Received: from eggs.gnu.org ([209.51.188.92]:35260) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gkSEM-0007Ff-Oe for qemu-devel@nongnu.org; Fri, 18 Jan 2019 06:22:35 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1gkSEH-00046U-DT for qemu-devel@nongnu.org; Fri, 18 Jan 2019 06:22:30 -0500 Received: from mx1.redhat.com ([209.132.183.28]:46596) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1gkSEG-00041X-GH for qemu-devel@nongnu.org; Fri, 18 Jan 2019 06:22:28 -0500 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 667557F086; Fri, 18 Jan 2019 11:22:21 +0000 (UTC) Received: from x1w.redhat.com (unknown [10.40.205.134]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 4BE6B60C47; Fri, 18 Jan 2019 11:22:15 +0000 (UTC) From: =?utf-8?q?Philippe_Mathieu-Daud=C3=A9?= To: "Edgar E . Iglesias" , Peter Maydell , Alistair Francis , Luc Michel , qemu-devel@nongnu.org Date: Fri, 18 Jan 2019 12:22:13 +0100 Message-Id: <20190118112213.11173-1-philmd@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.26]); Fri, 18 Jan 2019 11:22:21 +0000 (UTC) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 209.132.183.28 Subject: [Qemu-devel] [RFC PATCH] gdbstub: Avoid NULL dereference in gdb_handle_packet() X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: =?utf-8?q?Philippe_Mathieu-Daud=C3=A9?= Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: "Qemu-devel" 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 Signed-off-by: Philippe Mathieu-Daudé --- 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) {