[binutils-gdb] Check for truncated registers in process_g_packet

[Pedro Alves <palves at sourceware dot org>]

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=9dc193c3be85aafa60ceff57d3b0430af607b4ce

commit 9dc193c3be85aafa60ceff57d3b0430af607b4ce
Author: Lionel Flandrin <lionel@svkt.org>
Date:   Tue Nov 8 10:27:36 2016 +0000

    Check for truncated registers in process_g_packet
    
    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.
    
    gdb/ChangeLog:
    2016-11-08  Lionel Flandrin  <lionel@svkt.org>
    
    	* remote.c (process_g_packet): Detect truncated registers in 'g'
    	packets and raise an error.

Diff:
---
 gdb/ChangeLog |  5 +++++
 gdb/remote.c  | 20 +++++++++++++++++---
 2 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index fbdb168..f029938 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2016-11-08  Lionel Flandrin  <lionel@svkt.org>
+
+	* remote.c (process_g_packet): Detect truncated registers in 'g'
+	packets and raise an error.
+
 2016-11-07  Doug Evans  <dje@google.com>
 
 	* guile/scm-value.c (gdbscm_value_field): Fix call to value_struct_elt.
diff --git a/gdb/remote.c b/gdb/remote.c
index cf960e5..6c40f02 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -7579,18 +7579,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);
@@ -7620,10 +7633,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"));