[seedyjahateh]

The problem

The GDB stub writes attacker-chosen memory from data it never validated.

handle_write_mem() in debug/src/gdb_stub.c parses an M<addr>,<len>:<hexdata> packet. It bounds len to 128 and finds the : separator, but never checks that the packet actually carries 2 * len hex digits after it:

const char *data = strchr(pkt, ':');
if (!data) { send_packet(stub, "E01"); return; }
data++;
uint8_t buf[128];
for (uint32_t i = 0; i < len; i++)
    buf[i] = (hex_val(data[i * 2]) << 4) | hex_val(data[i * 2 + 1]);

A peer sending $M0,80:#xx declares 128 bytes and supplies none. The loop reads 256 bytes starting at the NUL terminator — straight through the uninitialised remainder of the caller's char pkt[512] — and then hands those bytes to eos_gdb_write_mem() at an address the peer chose. Because %x accepts arbitrarily many leading zeros, the address field can also be padded to push the separator past index 500, making the same loop read beyond pkt[512] entirely.

hex_val() compounds it by mapping every non-hex byte to 0, so a malformed payload was accepted as zero bytes rather than rejected.

This is not theoretical. Running the new tests against unmodified gdb_stub.c crashes the process with an access violation (0xC0000005), because the decoded garbage is memcpy'd to the peer-supplied address.

The approach

Two checks in handle_write_mem(), both replying E01 and returning without touching memory:

1. Require strlen(data) >= 2 * len before decoding.
2. Decode through a new hex_val_checked() that returns -1 on a non-hex byte, and reject on the first invalid digit.

hex_val() is left in place for the checksum path, where a lenient parse only affects an integrity comparison that is verified separately. Nothing else in the file is changed.

Testing

tests/test_debug.c gains a scripted transport — a mock EosGdbIO that feeds crafted packets and captures replies. The stub's packet handling had no test coverage at all; the existing cases only exercised init, breakpoint bookkeeping, and start/stop, never the parser.

Four rejection cases plus a qSupported round-trip to confirm normal packets still work:

• M0,80: — declares 128 bytes, supplies none
• M0,4:aabb — declares 8 digits, supplies 4
• M0,2:zzzz — non-hex payload
• M0,1000:aabb — length over the 128 limit

Verification (GCC 16.2.0, -DEOS_BUILD_TESTS=ON):

Check	Result
test_debug against unfixed source	crash, 0xC0000005
test_debug against fixed source	PASS, 12/12
ctest full suite	20/21 — identical to baseline
gcc -std=c11 -Wall -Wextra on gdb_stub.c	zero warnings
cmake -DEOS_PRODUCT=robot (CI-gated profile)	builds clean

test_package fails with a segfault both before and after this change. It is pre-existing on master and already addressed by #48 and #52, so I have not touched it.

Considerations and limitations

The tests cover only packets the stub must reject. eos_gdb_read_mem() / eos_gdb_write_mem() take a uint32_t address and cast it straight to a pointer, which truncates on a 64-bit host — so exercising an accepted M packet from a host test would dereference a truncated address. Fixing that means changing the signatures to uintptr_t, which is an API break and belongs in its own PR. I have left it alone and am flagging it rather than working around it silently.

Also observed, deliberately not changed: eos_gdb_handle_exception() does if (n < 0) break;, so a single bad checksum ends the debug session instead of letting the peer retransmit — which is what the GDB remote protocol expects after a -. I initially fixed a related framing issue in recv_packet() (an over-long packet body left trailing bytes in the stream), then reverted it: with the current break, the desync cannot actually be reached, and the change was not covered by a failing-before test. Both are worth a follow-up.

Environment. Validated on Windows with GCC 16.2.0 via a portable MinGW-w64 toolchain. I could not run the MSVC or Clang legs locally; the change is plain C11 with no platform-specific constructs, but CI is the real check for those.

[Copilot]

Pull request overview

This PR hardens the GDB remote stub’s M<addr>,<len>:<hexdata> write-memory packet handling by validating payload length and rejecting non-hex digits, and adds host-side tests that exercise these rejection paths to prevent the prior out-of-bounds read / attacker-controlled memory write.

Changes:

• Add hex_val_checked() and enforce payload validation in handle_write_mem() before decoding/writing memory.
• Add a scripted EosGdbIO transport in tests/test_debug.c and new regression tests for malformed M packets (plus a qSupported sanity round-trip).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
debug/src/gdb_stub.c	Validates M packet payload length and hex digits before decoding into the write buffer.
tests/test_debug.c	Adds a scripted mock transport and new regression tests covering invalid M packet cases.

Suppressed comments (1)

tests/test_debug.c:103

• gdb_push() appends to the global gdb_rx script buffer without any bounds checks. As written, adding longer packets or more scripted steps will silently overflow gdb_rx and cause UB in the test process. Add a capacity assertion (or clamp) before writing "$#".

    gdb_rx[gdb_rx_len++] = '$';
    for (size_t i = 0; i < n; i++) {
        sum = (uint8_t)(sum + (uint8_t)body[i]);
        gdb_rx[gdb_rx_len++] = body[i];
    }

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[seedyjahateh]

Thanks — valid on both counts, addressed in ef47f28. Test harness only; no production code changed, so the rejection tests and the crash evidence in the description are unaffected.

mock_write() violated the EosGdbIO.write contract. It truncated once gdb_tx neared capacity but still returned the requested length. The failure mode is the bad one: tx_has() would report a reply missing that the stub had in fact sent, so a passing assertion could silently become a false negative later. It now writes the full buffer and asserts there is room, failing loudly instead.

gdb_push() had no capacity check — the same class of problem in the input direction, and arguably worse, since a longer scripted packet would have overflowed a static buffer inside the test process. It now asserts before writing ('$' + body + '#' + two checksum digits).

Worth noting the irony that a test suite for an out-of-bounds write had two unbounded buffers of its own; both are now bounded.

Re-verified after the change: test_debug 12/12, ctest 20/21 — the outstanding failure is the pre-existing test_package segfault on master, unchanged and already covered by #48 and #52. Zero warnings under -Wall -Wextra.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

[srpatcha]

Review — verified, ready to merge

Built merged onto current master: 24/24 ctest pass.

I checked the specific concern a reviewer would have here, which is whether len can overflow buf[128]. It cannot — sscanf(...) != 2 || len > 128 already rejects that on entry. So the bug you fixed is the out-of-bounds read past the end of the received packet, which is exactly how you described it, and the fix is correct.

hex_val_checked as a separate function rather than changing hex_val is the right call: the existing lenient behaviour is fine for the read path and only the write path needs to reject.

The scripted transport in test_debug.c is genuinely good work. Honouring the EosGdbIO.write contract by returning the byte count — with the comment explaining that silently truncating would hide a reply the stub really sent — is the kind of detail most test harnesses get wrong.

Your note that the accept path is not exercised because eos_gdb_write_mem casts a uint32_t straight to a pointer is honest and correct. That is worth its own issue: on a 64-bit host the address is truncated, so the accept path is untestable as written.

[srpatcha]

Verified locally against current master: merges clean, builds with no new errors, ctest 24/24.

This is a remote-triggerable write and worth having. Two distinct holes, both closed:

• The decode loop read len * 2 characters from data with nothing establishing they existed, so a truncated M packet read past the received buffer and the bytes that happened to follow it were written to an address the peer chose.
• hex_val() maps every non-hex byte to 0, so a malformed payload silently decoded to zeros and still got written. Splitting out hex_val_checked rather than changing hex_val is the right call — the read path's use of it is a display concern where lenience is harmless.

Checked the buffer: buf[128] is safe because the existing len > 128 test rejects before the loop, so the new length check does not need to carry that too.

Also appreciated the note that the accept path is deliberately untested because eos_gdb_read_mem/write_mem cast a uint32_t straight to a pointer and would dereference a truncated address on a 64-bit host. That is the honest scope for this PR; the pointer-width problem is a separate change.