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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message gives the wrong reason for the new csum_offset
  bound in skb_copy_and_csum_dev() (net/core/skbuff.c).
- [Low] The concern says the 'don't BUG()' subject and framing hide that
  skb_copy_and_csum_dev() closes silent memory-corruption paths: a…

--

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

This patch replaces the BUG_ON() in skb_copy_and_csum_dev() with a check
on both ends of csum_start and on whether the checksum field fits in the
frame. When the check fails, 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: Low]

Is this description accurate? skb_copy_and_csum_dev() asks
skb_copy_and_csum_bits() for exactly the bytes left in the skb:

    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() only fires when the
head, frags and frag_list add up to less than skb->len. csum_offset plays
no part in that check.

For a well-formed skb with a bad csum_offset, the pre-patch code would not
BUG and would go on to:

    long csstuff = csstart + skb->csum_offset;
    *((__sum16 *)(to + csstuff)) = csum_fold(csum);

csum_offset is a __u16. Couldn't this store land up to ~64KB past
to + skb->len and overflow the driver's bounce buffer (for example
tx_buf[entry] in 8139too) today?

The follow-up patch that replaces BUG_ON(len) in skb_copy_and_csum_bits()
with a memset and return doesn't change this. A short skb with a valid
csum_offset still writes inside [0, skb->len), and a well-formed skb with
a bad csum_offset overflows with or without that patch.

It isn't clear which producer could create such a csum_offset, since entry
points like skb_partial_csum_set() validate it. Still, could the commit
message say that the new csum_offset check closes an existing
out-of-bounds store, rather than one that only shows up after the next
patch? The code change itself looks correct.

> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 32d7f5ed25eb4..c896770203f71 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -3977,7 +3977,18 @@ void skb_copy_and_csum_dev(const struct sk_buff *skb, u8 *to)
>  	else
>  		csstart = skb_headlen(skb);
>  
> -	BUG_ON(csstart > skb_headlen(skb));
> +	if (unlikely(csstart < 0 || csstart > skb_headlen(skb) ||

[Severity: Low]

The commit message says a negative offset "gets past the check and
becomes a ~4GB copy". Is that only the case on 64-bit?

csstart is a long and skb_headlen() returns unsigned int. On 64-bit the
comparison is signed. A negative csstart gets past the old BUG_ON() and
becomes a huge unsigned int length in skb_copy_from_linear_data().

On 32-bit the comparison is unsigned, so the old BUG_ON() would have
caught it.

The patch closes a silent memory-corruption path on 64-bit, plus the
csum_offset store described above. Should it have a Fixes: tag or a
Cc: stable? The trailers only have Assisted-by: and Signed-off-by:. No
producer is named that would show whether this is reachable, and that may
affect whether a stable backport is needed.

> +		     (skb->ip_summed == CHECKSUM_PARTIAL &&
> +		      csstart + skb->csum_offset + sizeof(__sum16) >
> +		      skb->len))) {

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261007-b4-skbuff-bug-on-v2-0-b9a5f732895b%40toxicpanda.com