From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 22745 invoked by alias); 18 Oct 2016 16:07:54 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 22730 invoked by uid 89); 18 Oct 2016 16:07:53 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.8 required=5.0 tests=BAYES_00,RCVD_IN_BRBL_LASTEXT,RP_MATCHES_RCVD,SPF_PASS autolearn=no version=3.3.2 spammy=macro@imgtec.com, sk:macroi, sk:macro@i, macroimgteccom X-HELO: mail.svkt.org Received: from server.svkt.org (HELO mail.svkt.org) (91.121.119.113) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 18 Oct 2016 16:07:43 +0000 Received: from localhost (unknown [10.31.130.2]) by mail.svkt.org (Postfix) with ESMTPSA id 2C6BE33C05; Tue, 18 Oct 2016 16:07:41 +0000 (UTC) Date: Tue, 18 Oct 2016 16:07:00 -0000 From: Lionel Flandrin To: Simon Marchi Cc: gdb-patches@sourceware.org Subject: Re: Check for truncated registers in process_g_packet Message-ID: <20161018160657.rdvxgcam3uibsgst@localhost.localdomain> References: <20161018111023.4hzeyfzzpaneyfds@localhost.localdomain> <33a1f569-995b-342a-dbb9-ea14ab377d1a@ericsson.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <33a1f569-995b-342a-dbb9-ea14ab377d1a@ericsson.com> User-Agent: NeoMutt/ (1.7.0) X-SW-Source: 2016-10/txt/msg00514.txt.bz2 On Tue, Oct 18, 2016 at 11:49:01AM -0400, Simon Marchi wrote: > On 16-10-18 07:10 AM, Lionel Flandrin wrote: > > Hello, > > > > While investigating an unrelated issue in remote.c I noticed that the > > bound checking for 'g' packets was bogus: > > > > The previous code would only check that the first byte of the register > > was within bounds before passing the buffer to regcache_raw_supply. If > > it turned out that the register in the 'g' packet was incomplete then > > regcache_raw_supply would proceed to memcpy out-of-bounds. > > > > Since the buffer is allocated with alloca it's relatively unlikely to > > crash (you just end up dumping gdb's stack into the cache) but it's > > still a bit messy. > > > > I changed this logic to check for truncated registers and raise an > > error if one is encountered. Hopefully it should make debugging remote > > stubs a bit easier. > > Hi Lionel, > > This patch looks good to me, a few minor comments below about formatting. > Someone else with the approval stamp must look at it, but hopefully it will > save them a bit of work. Thank you for the feedback, here's the updated patch: diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 4b642b8..3ace874 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,8 @@ +2016-10-18 Lionel Flandrin + + * remote.c (process_g_packet): Detect truncated registers in 'g' + packets and raise an error. + 2016-10-18 Maciej W. Rozycki * i386-tdep.c (i386_mpx_info_bounds): Make sure the architecture diff --git a/gdb/remote.c b/gdb/remote.c index af7508a..e1b5ad7 100644 --- a/gdb/remote.c +++ b/gdb/remote.c @@ -7163,18 +7163,31 @@ process_g_packet (struct regcache *regcache) the 'p' packet must be used. */ if (buf_len < 2 * rsa->sizeof_g_packet) { - rsa->sizeof_g_packet = buf_len / 2; + long sizeof_g_packet = buf_len / 2; for (i = 0; i < gdbarch_num_regs (gdbarch); i++) { + long offset = rsa->regs[i].offset; + long reg_size = register_size (gdbarch, i); + if (rsa->regs[i].pnum == -1) continue; - if (rsa->regs[i].offset >= rsa->sizeof_g_packet) + if (offset >= sizeof_g_packet) rsa->regs[i].in_g_packet = 0; + else if (offset + reg_size > sizeof_g_packet) + error (_("Truncated register %d in remote 'g' packet"), i); else rsa->regs[i].in_g_packet = 1; } + + /* Looks valid enough, we can assume this is the correct length + for a 'g' packet. It's important not to adjust + rsa->sizeof_g_packet if we have truncated registers otherwise + this "if" won't be run the next time the method is called + with a packet of the same size and one of the internal errors + below will trigger instead. */ + rsa->sizeof_g_packet = sizeof_g_packet; } regs = (char *) alloca (rsa->sizeof_g_packet); @@ -7204,10 +7217,11 @@ process_g_packet (struct regcache *regcache) for (i = 0; i < gdbarch_num_regs (gdbarch); i++) { struct packet_reg *r = &rsa->regs[i]; + long reg_size = register_size (gdbarch, i); if (r->in_g_packet) { - if (r->offset * 2 >= strlen (rs->buf)) + if ((r->offset + reg_size) * 2 > strlen (rs->buf)) /* This shouldn't happen - we adjusted in_g_packet above. */ internal_error (__FILE__, __LINE__, _("unexpected end of 'g' packet reply")); -- Lionel Flandrin