Re: [PATCH net-next 10/15] net: skbuff: reject a negative length in skb_copy_and_csum_bits()

[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's reason for dropping the `if (len > 0)` guard
  on the final zero-fill in skb_copy_and_csum_bits() (net/core/skbuff.c)…
- [Low] The patch narrows the zero-fill promise in
  skb_copy_and_csum_bits()'s kernel-doc: "on -EFAULT the part of @to…

--

net: skbuff: reject a negative length in skb_copy_and_csum_bits()

This commit makes skb_copy_and_csum_bits() return -EINVAL, with a
DEBUG_NET_WARN_ON_ONCE(), when it is given a negative len. It also drops
the len > 0 guard on the final zero-fill. The kernel-doc now promises the
zero-fill only for -EFAULT.

Link: https://lore.kernel.org/all/20261007-b4-skbuff-bug-on-v2-0-b9a5f732895b@toxicpanda.com/

> Commit 417261779eab ("net: skbuff: don't BUG() on leftover length in
> skb_copy_and_csum_bits()") guards its zero-fill against a negative @len
> from a broken caller, but such a @len never gets that far.  The head,
> frag and frag_list copies all clamp their length to @len, so a negative
> one has already turned into a huge copy, or an endless frag walk, by
> then.

[Severity: Low]
Is this an accurate description of the old code? The head, frag and
frag_list copies clamp to len only when they run. All three are skipped
when offset is at or past the end of the skb's data.

Take a linear skb with no frags, offset == skb->len and len == -1:

    copy = start - offset;                            /* 0, no head copy */
    for (i = 0; i < skb_shinfo(skb)->nr_frags; i++)   /* no frags */
    skb_walk_frags(skb, frag_iter)                    /* empty */

In the parent commit, the negative len then reaches the final block
unchanged:

net/core/skbuff.c:skb_copy_and_csum_bits() {
    ...
	if (unlikely(len)) {
		DEBUG_NET_WARN_ON_ONCE(1);
		/* Don't hand the caller a buffer with stale bytes in it. */
		if (len > 0)
			memset(to, 0, len);
		return -EFAULT;
	}
    ...
}

There, the len > 0 check is what prevents a memset of close to SIZE_MAX
bytes.

The code change itself looks right, because the new check on entry keeps
len non-negative for the rest of the function. The Fixes: tag relies on
this description of what 417261779eab guarded, though. Could the commit
message say that the guard only covered the final zero-fill and not the
clamped copies?

> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 782a6bacd568..56cec58bccb7 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -3678,9 +3678,10 @@ EXPORT_SYMBOL(skb_checksum);
>   *	@len: number of bytes to copy
>   *	@csump: where to store the checksum of the copied bytes
>   *
> - *	Return: 0 on success, or -EFAULT if @skb has unreadable frags or runs
> - *	out before @len does.  On failure *@csump is not set and the part of
> - *	@to that couldn't be filled is zeroed.
> + *	Return: 0 on success, -EINVAL if @len is negative, or -EFAULT if @skb
> + *	has unreadable frags or runs out before @len does.  On failure *@csump
> + *	is not set, and on -EFAULT the part of @to that couldn't be filled is
> + *	zeroed.
>   */
>  int skb_copy_and_csum_bits(const struct sk_buff *skb, int offset,
>  			   u8 *to, int len, __wsum *csump)
> @@ -3692,6 +3693,14 @@ int skb_copy_and_csum_bits(const struct sk_buff *skb, int offset,
>  	__wsum csum = 0;
>  	int err;
>  
> +	/* Everything below clamps copies to @len, so a negative one would
> +	 * turn into a huge copy.
> +	 */
> +	if (unlikely(len < 0)) {
> +		DEBUG_NET_WARN_ON_ONCE(1);
> +		return -EINVAL;
> +	}

[Severity: Low]
This early return leaves to untouched, and the kernel-doc above now
promises the zero-fill only for -EFAULT. Does skb_copy_and_csum_dev() need
an update as well? Its comment still says that any failure zeroes the
buffer:

net/core/skbuff.c:skb_copy_and_csum_dev() {
    ...
	csum = 0;
	/* On failure the rest of @to is zeroed; leave the checksum alone so
	 * the frame goes out with a bad one.
	 */
	if (csstart != skb->len &&
	    skb_copy_and_csum_bits(skb, csstart, to + csstart,
				   skb->len - csstart, &csum))
		return;
    ...
}

The drivers that call it (8139too, via-rhine, sc92031, ep93xx, ec_bhf,
ksz884x) transmit to after it returns. The csstart checks keep
skb->len - csstart non-negative as an unsigned value, but they don't keep
it within INT_MAX. A corrupt skb with more than INT_MAX bytes past csstart
would get -EINVAL here, and the bytes after csstart would go out stale.

That also goes against the first patch in the series ("net: skbuff: make
skb_copy_and_csum_bits() report unreadable frags"), which says "The
zero-fill stays, so a caller that does send the buffer still doesn't leak
anything."

The drivers' own size limits keep valid skbs from getting here. Before
this patch, the same corrupt input caused a huge copy instead. The comment
in skb_copy_and_csum_dev() is still unchanged at the end of the series,
including after "net: update comments that still describe the skb
BUG_ON()s".

Could that comment be updated? Alternatively, skb_copy_and_csum_dev()
could zero the rest of to itself when the helper fails.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261019-b4-skbuff-bug-on-followups-v1-0-41dbef50b9f7%40toxicpanda.com