From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 91321 invoked by alias); 27 Oct 2016 15:23: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 91276 invoked by uid 89); 27 Oct 2016 15:23:53 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.3 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 spammy=stamp X-HELO: mail.svkt.org Received: from mail.svkt.org (HELO mail.svkt.org) (37.187.5.13) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 27 Oct 2016 15:23:43 +0000 Received: from localhost (LFbn-1-243-116.w86-242.abo.wanadoo.fr [86.242.18.116]) by mail.svkt.org (Postfix) with ESMTPSA id B3D70EBF6 for ; Thu, 27 Oct 2016 17:23:40 +0200 (CEST) Date: Thu, 27 Oct 2016 15:23:00 -0000 From: Lionel Flandrin To: gdb-patches@sourceware.org Subject: Re: Check for truncated registers in process_g_packet Message-ID: <20161027152339.efln6aapxgaienje@localhost.localdomain> References: <20161018111023.4hzeyfzzpaneyfds@localhost.localdomain> <33a1f569-995b-342a-dbb9-ea14ab377d1a@ericsson.com> <20161018160657.rdvxgcam3uibsgst@localhost.localdomain> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="usteflfobvpwjp4c" Content-Disposition: inline In-Reply-To: <20161018160657.rdvxgcam3uibsgst@localhost.localdomain> User-Agent: NeoMutt/20160916 (1.7.0) X-SW-Source: 2016-10/txt/msg00767.txt.bz2 --usteflfobvpwjp4c Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Content-length: 3823 On Tue, Oct 18, 2016 at 06:07:40PM +0200, Lionel Flandrin wrote: > On Tue, Oct 18, 2016 at 11:49:01AM -0400, Simon Marchi wrote: > > On 16-10-18 07:10 AM, Lionel Flandrin wrote: > > > Hello, > > >=20 > > > While investigating an unrelated issue in remote.c I noticed that the > > > bound checking for 'g' packets was bogus: > > >=20 > > > 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. > > >=20 > > > 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. > > >=20 > > > 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. > >=20 > > Hi Lionel, > >=20 > > This patch looks good to me, a few minor comments below about formattin= g. > > Someone else with the approval stamp must look at it, but hopefully it = will > > save them a bit of work. >=20 > Thank you for the feedback, here's the updated patch: >=20 > 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 >=20=20 > * 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 =3D buf_len / 2; > + long sizeof_g_packet =3D buf_len / 2; >=20=20 > for (i =3D 0; i < gdbarch_num_regs (gdbarch); i++) > { > + long offset =3D rsa->regs[i].offset; > + long reg_size =3D register_size (gdbarch, i); > + > if (rsa->regs[i].pnum =3D=3D -1) > continue; >=20=20 > - if (rsa->regs[i].offset >=3D rsa->sizeof_g_packet) > + if (offset >=3D sizeof_g_packet) > rsa->regs[i].in_g_packet =3D 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 =3D 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 =3D sizeof_g_packet; > } >=20=20 > regs =3D (char *) alloca (rsa->sizeof_g_packet); > @@ -7204,10 +7217,11 @@ process_g_packet (struct regcache *regcache) > for (i =3D 0; i < gdbarch_num_regs (gdbarch); i++) > { > struct packet_reg *r =3D &rsa->regs[i]; > + long reg_size =3D register_size (gdbarch, i); >=20=20 > if (r->in_g_packet) > { > - if (r->offset * 2 >=3D 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")); >=20 I'm politely bumping this so that it doesn't get forgotten. Sorry for the noise. --=20 Lionel Flandrin --usteflfobvpwjp4c Content-Type: application/pgp-signature; name="signature.asc" Content-length: 819 -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAABCgAGBQJYEhv7AAoJEFyWjSt2xM6VuVIQAKZtMT7tIaqCwBlqABbCjny8 eV9Uge3NkAj5SIFgFOPLyvcIDurSWrI+TrGJ74PWrAU1YKmltU4ODbmeQBKEZO2f WoLJXEXgbu2FVrJGiVZygknp0kVgSw6c1AUil2KahA6vHOKM5VOB5Nnbj6k+5rBm EjZgANKgcrnTowTlP1P8CMgN386Vtzj85DExIaFpnDY23isP9o3R2JF6mqo3s/WR B/2lF0LjSY2wHqagIMIMinmAoI4wCyUGFViNp4wsZCr0ZoLaFSHmmve0Uxvk8N8+ S0DFLG18LAuyZmU7wgM1Ihn6kHojmwOEGz4agu5+8E9GoiQ9SRFQUmgz3FPCIe8x GaPfAhazhnkZHrAdX5W3CeWky0xSp/u/tM2mj7SMnMuEP+ccE+IIxtYeGdPk+XYU 8J3cS9COhLNMgfyw5NUEskPEfWPCd2dGMWl4+pNXaqw3ObP1nOjDLiuJHle9C/+w R1pd11dTHDB8R4Bry0+TQSIG8k11Q9WOcH37xblc5uz3CWMLcpYaVJRjuXoDryBM PL3LxuhbW57tSt7iimcDkDqmBvWBsB7CXoN6jskocd/VXuyC/RaKPQQKA0sxl3q+ 6r/V2aXbMAzSfwVPqEDSprM4N7pRATsZkamcHSk1Ubkd+WLl9X4/13Zkc18rXmu5 Y4gkBO3qkRItSXRAxECT =Ni1F -----END PGP SIGNATURE----- --usteflfobvpwjp4c--