Re: Check for truncated registers in process_g_packet

[Lionel Flandrin <lionel@svkt.org>]

[-- Attachment #1: Type: text/plain, Size: 3845 bytes --]

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,
> > > 
> > > 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  <lionel@svkt.org>
> +
> +	* remote.c (process_g_packet): Detect truncated registers in 'g'
> +	packets and raise an error.
> +
>  2016-10-18  Maciej W. Rozycki  <macro@imgtec.com>
>  
>  	* 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"));
> 

I'm politely bumping this so that it doesn't get forgotten. Sorry for
the noise.

-- 
Lionel Flandrin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]