Re: [PATCH] kgdb: Fix buffer overflow in the 'M' and 'X' packet handlers

[Doug Anderson]

Hi,

On Wed, Sep 16, 2026 at 1:03 AM Fang Xieyan <fangxy@xxxxxxxxxxxx> wrote:
>
> diff --git a/kernel/debug/gdbstub.c b/kernel/debug/gdbstub.c
> index e271a43..a67ca1a 100644
> --- a/kernel/debug/gdbstub.c
> +++ b/kernel/debug/gdbstub.c
> @@ -373,6 +373,12 @@ static int write_mem_msg(int binary)
>
>         if (kgdb_hex2long(&ptr, &addr) > 0 && *(ptr++) == ',' &&
>             kgdb_hex2long(&ptr, &length) > 0 && *(ptr++) == ':') {
> +               /*
> +                * The in-place decode touches 2 * length bytes at ptr,
> +                * inside remcom_in_buffer[BUFMAX].
> +                */
> +               if (length > (BUFMAX - (ptr - remcom_in_buffer)) / 2)
> +                       return -EINVAL;

FWIW, my AI wasn't terribly happy with this patch. I've skimmed the AI
response, and my AI sure looks right to me. Given that your patch was
generated with AI assistance, I won't feel too bad pasting my AI's
response here. :-P If you think my AI is wrong or you think I should
look at all this more closely, let me know. Otherwise, I'll wait until
my AI is happy (or looks like it's full of crap) before delving too
deeply myself.

AI response follows:

While this correctly identifies a real out-of-bounds access in
write_mem_msg(), the fix conflates 'M' (hex memory write) with 'X'
(binary memory write). As written, this patch introduces a regression
for valid GDB binary writes, leaves kgdb_ebin2mem() vulnerable to
out-of-bounds reads on escaped payloads, and does not check against the
actual received packet length.

1. Regression: Breaks valid GDB 'X' packets larger than ~BUFMAX / 2

The comment and commit message state that the in-place decode touches
2 * length bytes at ptr and that a write over the limit never decoded
valid data. That is only true for 'M' packets (kgdb_hex2mem):

- In 'M' packets, each byte is encoded as 2 ASCII hex characters, and
  kgdb_hex2mem() uses the upper half (ptr + length to
  ptr + 2 * length - 1) as scratch space while decoding backwards. So
  'M' genuinely requires 2 * length bytes in remcom_in_buffer[BUFMAX].

- In 'X' packets (kgdb_ebin2mem), the payload is sent as raw binary (1
  byte per byte), except for '$', '#', '}', and '*' which are escaped
  with 0x7d ('}'). kgdb_ebin2mem() decodes forward in-place into
  ptr[0 ... length - 1] and does not use ptr + length ... ptr + 2 *
  length as scratch space.

GDB enables 'X' binary downloads by default ("set remote
binary-download-packet on") specifically so it can pack nearly BUFMAX
bytes per packet instead of BUFMAX / 2. When writing a large object or
running "load", GDB routinely sends 'X' packets where length is between
BUFMAX / 2 and BUFMAX - header_len (e.g., length = 900 when BUFMAX =
1024).

With this patch, kgdb replies to those valid 'X' packets with $E22#...
(-EINVAL). GDB only falls back from 'X' to 'M' when the target returns
an empty packet (""), indicating 'X' is unsupported. An E22 reply
causes GDB to abort the memory write entirely ("Cannot access memory at
address ...").

2. Bug: kgdb_ebin2mem() can still overflow on 0x7d escape sequences

In kgdb_ebin2mem():

  while (count-- > 0) {
    c[size] = *buf++;
    if (c[size] == 0x7d)
      c[size] = *buf++ ^ 0x20;
    size++;
  }

Here count (length) is the number of decoded output bytes, not the
number of input bytes consumed from buf. Every 0x7d byte causes a
second *buf++ read without decrementing count.

If the check in write_mem_msg() is relaxed for 'X' to allow length up
to the remaining space in remcom_in_buffer, a packet with length <=
avail that contains K escape bytes (0x7d) will read length + K bytes
from buf. If length + K > avail, buf runs past the end of
remcom_in_buffer[BUFMAX], triggering the same KASAN global-out-of-bounds
read in kgdb_ebin2mem().

To fix this properly, kgdb_ebin2mem() needs an explicit end pointer
(buf_end) and must check buf < buf_end before both *buf++ dereferences
in the loop.

3. Incomplete bounds check against actual received packet length

As the commit message notes, a client can claim any length regardless
of how much payload it actually sent. However, checking against BUFMAX
only checks against the static buffer capacity, not the number of bytes
actually received by get_packet().

If a client sends a short packet (e.g., claiming length = 256 while
only sending 4 hex chars), length <= (BUFMAX - header) / 2 passes.
kgdb_hex2mem() then reads hundreds of bytes past the NUL terminator
placed by get_packet() into stale data left in remcom_in_buffer from
previous packets (where non-hex chars decode to -1 / 0xff via
hex_to_bin()) and writes 256 bytes of garbage into kernel memory.

Suggested approach:

Record the received packet length in get_packet() (note that binary 'X'
packets can contain raw 0x00 bytes, so strlen() cannot be used for 'X'
packets) and pass the packet end pointer (or available received bytes)
down:

- For hex writes (!binary): check that 2 * length fits within both the
  received payload length and remcom_in_buffer[BUFMAX].
- For binary writes (binary): pass the packet end pointer into
  kgdb_ebin2mem() and check buf < buf_end before each byte read in the
  loop, returning -EINVAL if the input stream ends before count bytes
  are decoded.

(Side note: while looking at other callers of kgdb_hex2mem() in
gdbstub.c, note that qRcmd handling in gdb_cmd_query() passes len (the
hex char count from strlen()) instead of len / 2 as count to
kgdb_hex2mem(), which causes kgdb_hex2mem() to touch 2 * len bytes and
overflow remcom_in_buffer[BUFMAX] whenever len > (BUFMAX - 6) / 2.)