Re: [PATCH net-next v2 3/8] net: skbuff: don't BUG() when skb_copy_bits() fails in copy helpers

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

Critical: 0 · High: 0 · Medium: 1 (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] skb_try_coalesce() in net/core/skbuff.c stores the unsigned
  `from->len` in `int len`.

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but bounce_skb()
  in drivers/net/xen-netfront.c is an open-coded copy of skb_copy().

--

net: skbuff: don't BUG() when skb_copy_bits() fails in copy helpers

This patch removes the BUG_ON(skb_copy_bits()) calls from skb_copy(),
skb_copy_expand() and skb_try_coalesce(). On failure, skb_copy() and
skb_copy_expand() now free the new skb and return NULL, and
skb_try_coalesce() returns false. skb_try_coalesce() now copies into the
tailroom before calling skb_put(), so that @to is left untouched on failure.

[Severity: Medium]
This isn't a bug introduced by this patch, but bounce_skb() in
drivers/net/xen-netfront.c is an open-coded copy of skb_copy(), and it
still has the same BUG_ON():

drivers/net/xen-netfront.c:bounce_skb() {
    ...
	/* Set the data pointer */
	skb_reserve(n, headerlen);
	/* Set the tail pointer and length */
	skb_put(n, skb->len);

	BUG_ON(skb_copy_bits(skb, -headerlen, n->head, headerlen + skb->len));
    ...
}

bounce_skb() already frees n and returns NULL on the misaligned head path,
and xennet_start_xmit() handles that:

	nskb = bounce_skb(skb);
	if (!nskb)
		goto drop;

Could bounce_skb() get the same NULL-return conversion, either in this
patch or in a follow-up? No later patch in the series touches this site.

The other remaining BUG_ON(skb_copy_bits()) calls in net/ipv4/icmp.c and
net/ipv6/ip6_output.c look like a different case. There, the length is
checked beforehand or the destination fragment is already sized for the
copy.

[ ... ]

[Severity: Medium]
If from->len is corrupt, can copying before skb_put() turn it into an
out-of-bounds write past the tailroom of to?

skb_try_coalesce() stores the unsigned from->len in an int:

	int i, delta, len = from->len;

If from->len is above INT_MAX, len is negative. A negative len passes the
check above, because skb_tailroom() returns int.

It also gets past the first check in skb_copy_bits(). With offset 0 and
len == (int)skb->len, this compares 0 > 0:

	if (offset > (int)skb->len - len)
		goto fault;

After that, copy = start - offset is positive (headlen), copy > len is
true, and so copy is set to the negative len.
skb_copy_from_linear_data_offset() takes a const unsigned int len, so the
memcpy() into skb_tail_pointer(to) becomes roughly 2 GiB. When headlen is 0,
the frag path does the same thing through skb_frag_foreach_page().

Before this patch, skb_put(to, len) ran first. With the offset-based tail
on 64-bit, any from->len in roughly [2^31, 2^32 - tail) hit skb_over_panic()
before anything was copied.

This needs skb metadata that is already corrupt, for example a driver
passing a bogus frag size to skb_add_rx_frag(). No concrete producer has
been found. It also doesn't match this line in the commit message:

  "Copy into the tailroom before skb_put() so that @to is untouched on
   failure."

Would it work to reject len < 0, or to compare from->len as unsigned
against skb_tailroom(to), before the copy?

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