Fix unlikely getpkt buffer overflow

  Problem reported by Manish Sharma.
* gdbserver/remote-utils.cc (getpkt): Do not overflow buf.
---
 gdbserver/remote-utils.cc | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)
  

Hi,

Thanks for fixing this.  I have just one minor comment:


Paul Eggert <eggert@cs.ucla.edu> writes:

> Problem reported by Manish Sharma.
> * gdbserver/remote-utils.cc (getpkt): Do not overflow buf.
> ---
>  gdbserver/remote-utils.cc | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/gdbserver/remote-utils.cc b/gdbserver/remote-utils.cc
> index 34801d0b76f..6634b01c2f0 100644
> --- a/gdbserver/remote-utils.cc
> +++ b/gdbserver/remote-utils.cc
> @@ -979,6 +979,7 @@ getpkt (char *buf)
>  	    return -1;
>  	}
>  
> +      bool fits_in_buf = true;
>        bp = buf;
>        while (1)
>  	{
> @@ -987,7 +988,9 @@ getpkt (char *buf)
>  	    return -1;
>  	  if (c == '#')
>  	    break;
> -	  *bp++ = c;
> +	  *bp = c;
> +	  fits_in_buf = bp - buf < PBUFSIZ;
> +	  bp += fits_in_buf;

GDB tries to avoid implicit casting between types.  So I think this
should be written as:

  bp += (fits_in_buf ? 1 : 0);

With that change:

Approved-By: Andrew Burgess <aburgess@redhat.com>

Do you have access to push this?  Or would you like me to do that for
you?

Thanks,
Andrew
  

On 3/24/26 05:23, Andrew Burgess wrote:
> Do you have access to push this?  Or would you like me to do that for
> you?
No, I lack access. Please push, and thanks.
  

Paul Eggert <eggert@cs.ucla.edu> writes:

> On 3/24/26 05:23, Andrew Burgess wrote:
>> Do you have access to push this?  Or would you like me to do that for
>> you?
> No, I lack access. Please push, and thanks.

Hi Paul,

I was thinking about this patch some more, and took another look
through.  I'd like to propose the revised version below.

The core of your fix to ensure the packet buffer doesn't overflow is
unchanged.  What has changed is:

  1. In noack_mode we return an error (-1) from getpkt if the incoming
     packet is too long, or it has a bad checksum.  This should prevent
     the rest of gdbserver thinking that the packet that it has is
     valid.

  2. In ack mode, if a packet is too long, after sending the NAK getpkt
     immediately returns an error (-1).  GDB's only response to a NAK is
     to resend the packet, and if that packet is too long then getpkt is
     going to send back NAK, and so on...

     Now GDB should give up after 3 attempts, but it seems pointless to
     have gdbserver depend on GDB giving up.  Better I think to just
     give up once we see an oversized packet.

  3. I deleted a couple of comments I found with FIXME notes requesting
     that we add buffer overflow detection to getpkt.  You've now
     resolved that issue!

  4. I have added a comment without your buffer overflow detection to
     explain one aspect that gave me reason to double check the logic,
     we appear to write to the buffer before checking for overflow.  I
     believe this is absolutely fine, but relies on the buffering being
     allocated as 'PBUFSIZ + 1', e.g. in the extreme case of PBUFSIZ
     being 0, the first character write will be going into the '+ 1'
     byte, then bp will not be incremented as the packet will be
     considered oversized.

I'm going to test this overnight, and assuming nothing comes up I'll
push this tomorrow.

Thanks,
Andrew

---

commit bdc413c24576a3a5428c90e2abb35833780da665
Author: Paul Eggert <eggert@cs.ucla.edu>
Date:   Sun Mar 22 14:08:47 2026 -0700

    gdbserver: fix unlikely getpkt buffer overflow
    
    This problem was reported by Manish Sharma.
    
    Within gdbserver, in getpkt, there is no bounds checking as we parse
    the incoming packet.  An unexpectedly large packet can therefore
    overflow the allocated buffer.  Fixed by adding bounds checking.
    
    If a packet is too long then in ACK mode we send out the NAK, but then
    immediately return -1 as the result from getpkt.  Currently the only
    thing that GDB can do when it sees a '-' (NAK) is resend the packet.
    If the original packet was too long then the resent packet will also
    be too long.  gdbserver would then be stuck re-reading the incoming
    too long packet.  Now GDB does give up after 3 retries, but this means
    gdbserver is relying on GDB to give up sending, when in reality,
    gdbserver knows it's not going to be able to recover.  So I propose
    that gdbserver should just give up once it sees a packet that is too
    long.
    
    While looking at the error handling in this case I noticed that in the
    noack_mode case, if we get a packet with a bad checksum, or a packet
    that is too long, getpkt will return success and gdbserver will try to
    interpret whatever it has.  This seems like a bad idea.  So I've
    updated this code path to also return an error.
    
    Then there are a couple of places where we had a comment like this:
    
      /* FIXME:  Eventually add buffer overflow checking (to getpkt?)  */
    
    Now that getpkt does have buffer overflow checking, I've removed these
    comments.
    
    Approved-By: Andrew Burgess <aburgess@redhat.com>

diff --git a/gdbserver/remote-utils.cc b/gdbserver/remote-utils.cc
index 34801d0b76f..d7049baf083 100644
--- a/gdbserver/remote-utils.cc
+++ b/gdbserver/remote-utils.cc
@@ -979,6 +979,7 @@ getpkt (char *buf)
 	    return -1;
 	}
 
+      bool fits_in_buf = true;
       bp = buf;
       while (1)
 	{
@@ -987,7 +988,11 @@ getpkt (char *buf)
 	    return -1;
 	  if (c == '#')
 	    break;
-	  *bp++ = c;
+	  /* The buffer is always allocated as 'PBUFSIZ + 1' so we know
+	     that this write will always be within the buffer.  */
+	  *bp = c;
+	  fits_in_buf = bp - buf < PBUFSIZ;
+	  bp += (fits_in_buf ? 1: 0);
 	  csum += c;
 	}
       *bp = 0;
@@ -995,22 +1000,30 @@ getpkt (char *buf)
       c1 = fromhex (readchar ());
       c2 = fromhex (readchar ());
 
-      if (csum == (c1 << 4) + c2)
+      unsigned char sentsum = (c1 << 4) + c2;
+      bool csum_ok = csum == sentsum;
+      if (csum_ok && fits_in_buf)
 	break;
 
+      if (!csum_ok)
+	fprintf (stderr, "Bad checksum, sentsum=0x%x, csum=0x%x, buf=%s\n",
+		 sentsum, csum, buf);
+      if (!fits_in_buf)
+	fprintf (stderr, "Packet too long\n");
       if (cs.noack_mode)
 	{
-	  fprintf (stderr,
-		   "Bad checksum, sentsum=0x%x, csum=0x%x, "
-		   "buf=%s [no-ack-mode, Bad medium?]\n",
-		   (c1 << 4) + c2, csum, buf);
-	  /* Not much we can do, GDB wasn't expecting an ack/nac.  */
-	  break;
+	  fprintf (stderr, "[no-ack-mode, Bad medium?]\n");
+	  /* Not much we can do, GDB wasn't expecting an ack/nak, just
+	     return an error to indicate the packet was bad.  */
+	  return -1;
 	}
 
-      fprintf (stderr, "Bad checksum, sentsum=0x%x, csum=0x%x, buf=%s\n",
-	       (c1 << 4) + c2, csum, buf);
-      if (!write_prim ("-", 1))
+      /* Send '-' (NAK) back to GDB.  If that fails, or if the incoming
+	 packet was too long, then return an error.  For the packet too
+	 long case there's no point repeating the loop, all GDB can do is
+	 resend the original packet, which will be too long again, and
+	 we'll be stuck in this loop forever.  */
+      if (!write_prim ("-", 1) || !fits_in_buf)
 	return -1;
     }
 
@@ -1526,7 +1539,6 @@ look_up_one_symbol (const char *name, CORE_ADDR *addrp, int may_ask_gdb)
   if (putpkt (cs.own_buf) < 0)
     return -1;
 
-  /* FIXME:  Eventually add buffer overflow checking (to getpkt?)  */
   len = getpkt (cs.own_buf);
   if (len < 0)
     return -1;
@@ -1656,7 +1668,6 @@ relocate_instruction (CORE_ADDR *to, CORE_ADDR oldloc)
   if (putpkt (cs.own_buf) < 0)
     return -1;
 
-  /* FIXME:  Eventually add buffer overflow checking (to getpkt?)  */
   len = getpkt (cs.own_buf);
   if (len < 0)
     return -1;
  

On 2026-03-31 08:43, Andrew Burgess wrote:
> I was thinking about this patch some more, and took another look
> through.  I'd like to propose the revised version below.

Thanks, looks good to me.
  

Paul Eggert <eggert@cs.ucla.edu> writes:

> On 2026-03-31 08:43, Andrew Burgess wrote:
>> I was thinking about this patch some more, and took another look
>> through.  I'd like to propose the revised version below.
>
> Thanks, looks good to me.

I have now pushed this patch.

Thanks,
Andrew