Re: [PATCH net-next 6/9] net: skbuff: don't BUG() on a bad csum_start in skb_copy_and_csum_dev()

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 0

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] The commit message gets wrong what used to stop a bad
  csum_offset in skb_copy_and_csum_dev() (net/core/skbuff.c).

--

net: skbuff: don't BUG() on a bad csum_start in skb_copy_and_csum_dev()

This patch replaces the BUG_ON() on csum_start in skb_copy_and_csum_dev()
with a WARN_ON_ONCE(). The new check also catches a negative csum_start
and a csum_offset that would put the checksum field past skb->len. When
the check fires, the whole frame is copied with skb_copy_bits() and the
checksum is not filled in. If that copy also fails, the buffer is zeroed.

> It also trusts
> csum_offset when it stores the folded checksum.  So far
> skb_copy_and_csum_bits() BUG()ing on a short skb has covered for that,
> but once it returns instead, a bad csum_offset would write past the end
> of the caller's buffer.

[Severity: Medium]
Is this description accurate? skb_copy_and_csum_bits() never sees
csum_offset, so it isn't clear how its BUG_ON(len) could have covered for
it.

skb_copy_and_csum_dev() passes it a length that depends only on csstart
and skb->len:

	csum = skb_copy_and_csum_bits(skb, csstart, to + csstart,
				      skb->len - csstart);

The BUG_ON(len) at the end of skb_copy_and_csum_bits() fires only when the
head, frags and frag_list together hold less data than skb->len. If the
skb's lengths are consistent, it returns normally whatever csum_offset is.

The store after it had no bound before this patch:

	if (skb->ip_summed == CHECKSUM_PARTIAL) {
		long csstuff = csstart + skb->csum_offset;

		*((__sum16 *)(to + csstuff)) = csum_fold(csum);
	}

Without this patch, a CHECKSUM_PARTIAL skb with
csstart + csum_offset + 2 > skb->len would already write up to about 64KB
past the end of the driver's bounce buffer. The affected drivers are
8139too, sc92031, via-rhine, ksz884x, ep93xx and ec_bhf. Is that right?

A later patch in the series, "net: skbuff: don't BUG() on leftover length
in skb_copy_and_csum_bits()", does replace BUG_ON(len) with WARN_ON_ONCE()
plus a memset(). That doesn't change the point above.

If so, the new csum_offset condition fixes an out-of-bounds write that
already exists. It doesn't only guard against one that the later patch
would make possible. Could the commit message describe it that way, so
stable backporters can see it?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261006-b4-skbuff-bug-on-v1-0-1b4434c5357c%40toxicpanda.com