Re: [PATCH net-next v2 2/8] net: skbuff: don't BUG() on a bad frag_list layout in skb_segment()

[netdev-bot+sashiko@kernel.org]

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

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

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 patch is incomplete.
- [Low] This patch makes the comment in bpf_skb_net_grow()
  (net/core/filter.c:3698-3702) out of date.
- [Low] This patch makes the KUnit GSO test comment in gso_test_func()
  (net/core/net_test.c:223-227) out of date.

--

net: skbuff: don't BUG() on a bad frag_list layout in skb_segment()

This commit changes the four BUG_ON() layout checks in the frag_list walk
of skb_segment() to use the existing -EINVAL error path, adding a
DEBUG_NET_WARN_ON_ONCE() for debug kernels. The check that runs after
skb_clone() also frees the clone, which has not been linked yet, before
it bails out.

> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index d226fe77d4847..a6821ab139691 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -4916,7 +4916,11 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
>  
>  		if (hsize <= 0 && i >= nfrags && skb_headlen(list_skb) &&
>  		    (skb_headlen(list_skb) == len || sg)) {
> -			BUG_ON(skb_headlen(list_skb) > len);
> +			if (unlikely(skb_headlen(list_skb) > len)) {
> +				DEBUG_NET_WARN_ON_ONCE(1);
> +				err = -EINVAL;
> +				goto err;
> +			}

[Severity: Low]
This isn't a bug, but with this BUG_ON() and the head_frag BUG_ON() below
both converted, the comment in bpf_skb_net_grow() looks out of date:

net/core/filter.c:bpf_skb_net_grow() {
    ...
		/* Due to header growth, MSS needs to be downgraded.
		 * There is a BUG_ON() when segmenting the frag_list with
		 * head_frag true, so linearize the skb after downgrading
		 * the MSS.
		 */
    ...
}

The linearize is still needed, because the packet would otherwise be
dropped with -EINVAL. Should the comment say the packet is dropped
instead of referring to a BUG_ON()? No later patch in the series appears
to touch this comment.

>  
>  			nskb = skb_clone(list_skb, GFP_ATOMIC);
>  			if (unlikely(!nskb))
> @@ -4929,7 +4933,12 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
>  			pos += skb_headlen(list_skb);
>  
>  			while (pos < offset + len) {
> -				BUG_ON(i >= nfrags);
> +				if (unlikely(i >= nfrags)) {
> +					DEBUG_NET_WARN_ON_ONCE(1);
> +					kfree_skb(nskb);
> +					err = -EINVAL;
> +					goto err;
> +				}

[Severity: Low]
This isn't a bug, but the KUnit GSO test still names this assertion:

net/core/net_test.c:gso_test_func() {
    ...
	/* TODO: this should also work with SG,
	 * rather than hit BUG_ON(i >= nfrags)
	 */
	if (tcase->id == GSO_TEST_FRAG_LIST_NON_UNIFORM)
		features &= ~NETIF_F_SG;
    ...
}

Should this comment be updated now that BUG_ON(i >= nfrags) is gone? It
is still unchanged at the end of the series.

Also, since the test clears NETIF_F_SG for this case, no in-tree test
seems to exercise the new kfree_skb(nskb) unwind or the
kfree_skb_list(segs) / ERR_PTR(-EINVAL) return. Would a negative test
case for this path be worth adding?

>  
>  				size = skb_frag_size(frag);
>  				if (pos + size > offset + len)
> @@ -5036,9 +5045,17 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
>  				skb_shinfo(nskb)->flags |= skb_shinfo(frag_skb)->flags & SKBFL_SHARED_FRAG;
>  
>  				if (!skb_headlen(list_skb)) {
> -					BUG_ON(!nfrags);
> +					if (unlikely(!nfrags)) {
> +						DEBUG_NET_WARN_ON_ONCE(1);
> +						err = -EINVAL;
> +						goto err;
> +					}
>  				} else {
> -					BUG_ON(!list_skb->head_frag);
> +					if (unlikely(!list_skb->head_frag)) {
> +						DEBUG_NET_WARN_ON_ONCE(1);
> +						err = -EINVAL;
> +						goto err;
> +					}

[Severity: Medium]
Can skb_segment() still oops on a malformed frag_list through a NULL
list_skb?

For each frag_list member, the walk counts the linear part and frags[].
It never descends into a member's own nested frag_list, but
head_skb->len includes those bytes. With a nested frag_list, list_skb can
become NULL while bytes are still owed, and nothing checks for that.

In the SG loop, take a member with headlen 0, one page frag and a nested
frag_list. It passes the new !nfrags check, and list_skb advances to
NULL. Once that frag is used up, the next pass does this:

		while (pos < offset + len) {
			if (i >= nfrags) {
				if (skb_orphan_frags(list_skb, GFP_ATOMIC) ||
				    skb_zerocopy_clone(nskb, list_skb,
						       GFP_ATOMIC))
					goto err;

				i = 0;
				nfrags = skb_shinfo(list_skb)->nr_frags;

skb_orphan_frags(NULL) and skb_zerocopy_clone(nskb, NULL) both return 0,
because skb_zcopy() checks skb first. skb_shinfo(NULL) is then
dereferenced before the new !nfrags and !head_frag checks can run.

The clone branch has the same problem. Take a member with headlen == len,
no page frags, and a nested frag_list. The new > len check passes, the
inner while loop never runs, and list_skb = list_skb->next sets it to
NULL. pskb_trim() also drops the nested data from the clone. Since
offset + len is still below head_skb->len, the next iteration evaluates:

		if (hsize <= 0 && i >= nfrags && skb_headlen(list_skb) &&

with list_skb == NULL. In the gso_by_frags case, len = list_skb->len at
the top of the loop has the same issue.

tcp_gso_segment() appears to pass such an skb to skb_segment() unchanged.
I couldn't find an in-tree producer of nested frag_lists that reaches
here with NETIF_F_SG set. GRO sets NAPI_GRO_CB(skb)->flush when the skb
already has a frag_list.

Since the goal is to stop malformed layouts from crashing the box, should
the walk return -EINVAL when list_skb is NULL before each use, or reject
nested frag_lists up front? Later patches in the series don't appear to
add such a check.

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