[PATCH] net: slip: fix out-of-bounds access on short CSLIP packets

[PATCH] net: slip: fix out-of-bounds access on short CSLIP packets

[Hui Peng]

In drivers/net/slip/slip.c, verify packet length before invoking
slhc_uncompress() / slhc_remember() on compressed SLIP frames so
truncated CSLIP headers cannot read or write past the receive buffer.

Fixes: b5451d783ade ("slip: Move the SLIP drivers")
Assisted-by: LLM
Signed-off-by: Hui Peng <benquike@gmail.com>
---
diff --git a/drivers/net/slip/slip.c b/drivers/net/slip/slip.c
index fdd6f63fb849..4389985979cf 100644
--- a/drivers/net/slip/slip.c
+++ b/drivers/net/slip/slip.c
@@ -335,7 +335,7 @@ static void sl_bump(struct slip *sl)
 			}
 			/* make sure we've reserved enough space for uncompress
 			   to use */
-			if (count + 80 > sl->buffsize) {
+			if (count + 120 > sl->buffsize) {
 				dev->stats.rx_over_errors++;
 				return;
 			}

Re: [PATCH] net: slip: fix out-of-bounds access on short CSLIP packets

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 (1) · Low: 2 (1)

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] Commit message describes work that the patch does not do, and
  names the wrong bug class.
- [Low] The new reserve literal 120 in sl_bump()
  (drivers/net/slip/slip.c:338) is undocumented and unlinked to the code…
- [Low] The Fixes tag is wrong.

Pre-existing issues:
- [Medium] The rx_over_errors early return in sl_bump()
  (drivers/net/slip/slip.c:338-341) discards a compressed frame without…
- [Low] Claimed TOCTOU race between the reserve check in sl_bump() and
  sl_realloc_bufs() swapping/shrinking sl->rbuff and sl->buffsize.

--

net: slip: fix out-of-bounds access on short CSLIP packets

The patch changes a single constant in sl_bump() in drivers/net/slip/slip.c,
raising the reserve required before a compressed frame is handed to
slhc_uncompress() from 80 to 120 bytes.  Frames that do not leave that much
room in sl->rbuff are dropped and counted in dev->stats.rx_over_errors.

No Link: tags present in the commit header.

> net: slip: fix out-of-bounds access on short CSLIP packets
>
>     In drivers/net/slip/slip.c, verify packet length before invoking
>     slhc_uncompress() / slhc_remember() on compressed SLIP frames so
>     truncated CSLIP headers cannot read or write past the receive buffer.

[Severity: Medium]
Does the changelog match what the diff does?  The only change is the
tail-reserve constant in sl_bump() going from 80 to 120.  No packet length
verification is added, and the slhc_remember() arm is not touched at all:

drivers/net/slip/slip.c:sl_bump() {
	...
	} else if (c >= SL_TYPE_UNCOMPRESSED_TCP) {
		...
		sl->rbuff[0] &= 0x4f;
		if (slhc_remember(sl->slcomp, sl->rbuff, count) <= 0)
			return;
	}
	...
}

Is the "truncated CSLIP header" read the message describes actually
reachable?  slhc_uncompress() rejects isize < 3, bounds the checksum fetch
with "if (cp + 2 > end)", passes end into decode() for every delta field:

drivers/net/slip/slhc.c:decode() {
	...
	if (*cpp >= end)
		return -1;
	...
}

and rejects len < 0.  slhc_remember() has its own runt checks:

drivers/net/slip/slhc.c:slhc_remember() {
	...
	if (ihl < 5 || isize < ihl * 4 + sizeof(struct tcphdr))
		goto runt;
	...
}

The problem the new constant does address looks like a different one:
header re-expansion past the end of rbuff on a write, not a read of a
truncated header.  In slhc_uncompress(), hdrlen = ip->ihl * 4 +
thp->doff * 4 can reach 120, and the reconstruction writes into the caller
buffer:

drivers/net/slip/slhc.c:slhc_uncompress() {
	...
	hdrlen = ip->ihl * 4 + thp->doff * 4;
	...
	len = isize - (cp - icp);
	if (len < 0)
		goto bad;
	len += hdrlen;
	...
	memmove(icp + hdrlen, cp, len - hdrlen);
	...
}

With at least 3 input bytes consumed, those writes reach up to
sl->rbuff + count + 117, so with the old 80-byte reserve and rbuff =
kmalloc(buffsize + 4) a slot primed with maximal IP and TCP options
(ihl = 15, doff = 15, via a preceding uncompressed TCP frame) could be
written out of bounds.  Could the changelog describe that sequence, the
derivation of 120, and the fact that this is a heap write overflow rather
than a read of a short header?

>     Fixes: b5451d783ade ("slip: Move the SLIP drivers")

[Severity: Low]
Is this Fixes tag pointing at the right commit?  b5451d783ade only moved
files, and its changelog says so: "Move the Serial Line Internet Protocol
(SLIP) drivers into drivers/net/slip/ and make the necessary Kconfig and
Makefile changes."  git blame of the pre-patch line gives:

1da177e4c3f4 drivers/net/slip.c (Linus Torvalds 2005-04-16) if (count + 80 > sl->buffsize) {

so the undersized reserve predates the move and predates git history.
For a bug this old, would it be better to drop the Fixes tag and rely on
Cc: stable, so pre-3.1 trees are not skipped by stable tooling?

> diff --git a/drivers/net/slip/slip.c b/drivers/net/slip/slip.c
> index fdd6f63fb849..4389985979cf 100644
> --- a/drivers/net/slip/slip.c
> +++ b/drivers/net/slip/slip.c
> @@ -335,7 +335,7 @@ static void sl_bump(struct slip *sl)
>  			}
>  			/* make sure we've reserved enough space for uncompress
>  			   to use */
> -			if (count + 80 > sl->buffsize) {
> +			if (count + 120 > sl->buffsize) {

[Severity: Low]
Could this 120 be a named constant, with the comment explaining where it
comes from?  Nothing here ties the literal to the arithmetic it has to
track in slhc_uncompress():

	hdrlen = ip->ihl * 4 + thp->doff * 4;	/* up to 60 + 60 */

which is how the old 80 drifted from the real bound.  The sibling caller
open-codes yet another unexplained literal for the same expansion bound:

drivers/net/ppp/ppp_generic.c:ppp_receive_nonmp_frame() {
	...
	if (skb_tailroom(skb) < 124 || skb_cloned(skb)) {
	...
}

Would a shared macro next to slhc_uncompress() keep the two callers and
slhc.c from drifting apart again?

Also, does this narrow what is accepted?  Compressed frames with
sl->buffsize - 120 < count <= sl->buffsize - 80 were previously
decompressed and delivered, and are now dropped with
dev->stats.rx_over_errors++.  At the default MTU of 296 (buffsize =
max(2 * mtu, 1152) = 1152) that is roughly the 1033..1072 byte range.
Could the changelog mention that acceptance and statistics change?

>  				dev->stats.rx_over_errors++;
>  				return;
>  			}

[Severity: Medium]
This isn't a bug introduced by this patch, but should this early return
call slhc_toss(sl->slcomp) before dropping the frame?  The same omission
exists with the 80-byte reserve and on the "compressed packet ignored"
return; this patch only widens the rejection window by 40 bytes.

When the frame is discarded here, the decompressor never sees it, so
comp->recv_current and SLF_TOSS keep their old values:

drivers/net/slip/slhc.c:slhc_uncompress() {
	...
	if(changes & NEW_C){
		...
		comp->flags &=~ SLF_TOSS;
		comp->recv_current = x;
	} else {
		if(comp->flags & SLF_TOSS){
			comp->sls_i_tossed++;
			return 0;
		}
	}
	...
}

and the caller just resets the counter and carries on:

drivers/net/slip/slip.c:slip_unesc() {
	...
			sl_bump(sl);
		clear_bit(SLF_ESCAPE, &sl->flags);
		sl->rcount = 0;
	...
}

If the dropped frame carried NEW_C selecting receive slot B, recv_current
still points at slot A and SLF_TOSS stays clear.  Since slhc_compress()
omits the connection index once its xmit_current matches, can the next
frame for B arrive with an implicit connection id and be decoded against
slot A's saved cs_ip/cs_tcp, producing a packet with A's addresses and
ports plus B's payload, and leaving A's cached state wrong until an
uncompressed TCP frame for A re-primes it?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260919223432.3882500-1-benquike%40gmail.com