[PATCH net-next v2 0/8] net: skbuff: replace most BUG_ON()s with error returns

[PATCH net-next v2 0/8] net: skbuff: replace most BUG_ON()s with error returns

[Josef Bacik]

v1: https://lore.kernel.org/all/20261006-b4-skbuff-bug-on-v1-0-1b4434c5357c@toxicpanda.com/

v1->v2:
- Use DEBUG_NET_WARN_ON_ONCE() instead of WARN_ON_ONCE(), so
  panic_on_warn systems take the error path instead of panicking
  (Willem, Fernando).
- Split out the skb_copy_and_csum_bits() unreadable-frags fix and sent
  it to net on its own with Cc: stable (Willem):
  https://lore.kernel.org/all/20261007-b4-skb-copy-csum-stale-bytes-v1-1-adbbde033fb3@toxicpanda.com/
- Rebased onto current net-next.

--- Original email (v1) ---

I'm going through and reducing BUG_ON() usage in areas that have created
the most problems for us.  89 commits in the tree quote "kernel BUG at
net/core/skbuff.c", 31 of them since 2024, and some of those could be
triggered from inside a user namespace.

The first patch is a fix: skb_copy_and_csum_bits() leaves stale bytes
in a buffer headed for the wire when it hits unreadable frags.  The
BUG_ON() conversion of the same function needs the same handling, so
it's here rather than sent separately.

The rest of the series converts 17 of the 19 BUG_ON()s in skbuff.c.
Each one becomes

	if (WARN_ON_ONCE(cond))
		<error path>;

where the error path is a failure return the function already has and
its callers already handle: -EINVAL from pskb_expand_head() and
skb_segment(), NULL from skb_copy(), 0 from skb_shift(), and so on.
Each patch says what its error path is and why it's safe.  Anybody who
wants the old behaviour, syzbot included, gets it with panic_on_warn.

A few don't have an obvious error return:

 - skb_copy_and_csum_bits() zeroes the part of the caller's buffer it
   couldn't fill instead of leaving stale bytes in it.
 - skb_copy_and_csum_dev() copies the frame without a checksum.  It
   also now catches a csum_start before the head and a csum_offset past
   the end of the frame, which the BUG_ON() missed.
 - skb_shift()'s second check ran after the shift had been committed.
   It moves up to just before the commit, where nothing has changed
   yet, and returns 0 there.

Two BUG_ON()s are left on purpose.  __pskb_pull_tail() and
skb_pull_rcsum() have callers that can't otherwise fail, so they don't
check the return.  Some of them would carry on and BUG() somewhere
else, or push back a pull that never happened.  Those need their
callers fixed first and will come as separate series.
skb_over_panic() and skb_under_panic() keep their BUG() as well; that's
overflow hardening and should stay fatal.

skbuff.o text on x86_64 defconfig grows by 114 bytes.  The fast path
takes the same branch it does today; the extra bytes are the error
paths that BUG() used to replace.

Testing: x86_64 defconfig with CONFIG_WERROR boots, and every patch
builds net/core/skbuff.o on its own with allmodconfig, W=1 and
CONFIG_DEBUG_NET.  A test module drives 12 of the 17 converted
BUG_ON()s with a bad argument or a malformed skb, skb_shift() through
a test-only export.  Each one warns once and returns its documented
error, and the skbs are left alone.  The same module covers the
unreadable-frags fix.  test_bpf's skb_segment tests pass.  The other
five were only reviewed: skb_crc32c() isn't built in defconfig, and
the four skb_segment() layout checks need a crafted frag_list.

Thanks,
Josef

---
Josef Bacik (8):
      net: skbuff: don't BUG() on bad arguments to pskb_expand_head()
      net: skbuff: don't BUG() on a bad frag_list layout in skb_segment()
      net: skbuff: don't BUG() when skb_copy_bits() fails in copy helpers
      net: skbuff: don't BUG() on leftover length in skb_checksum() and friends
      net: skbuff: don't BUG() on a bad csum_start in skb_copy_and_csum_dev()
      net: skbuff: don't BUG() on leftover length in skb_copy_and_csum_bits()
      net: skbuff: don't BUG() on a missing head_frag in skb_zerocopy()
      net: skbuff: remove the BUG_ON()s from skb_shift()

 net/core/skbuff.c | 118 +++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 98 insertions(+), 20 deletions(-)
---
base-commit: 45ad84d2800e4a092fb8d96006a533b2d0ab13f6
change-id: 20261006-b4-skbuff-bug-on-b2844a487925

[PATCH net-next v2 1/8] net: skbuff: don't BUG() on bad arguments to pskb_expand_head()

[Josef Bacik]

pskb_expand_head() BUG()s if it is handed a negative nhead or a shared
skb.  Both are bugs in the caller, and both keep getting hit: commit
2cbb259ec4f8 ("bpf: Reject negative head_room in __bpf_skb_change_head")
and commit 64e6a754d33d ("llc: do not use skb_get() before
dev_queue_xmit()") each fixed a crash here that took down the whole box.

The function already returns an error that every caller has to handle,
and at this point it hasn't touched the skb.  Return -EINVAL instead of
crashing.  The warning is DEBUG_NET_WARN_ON_ONCE(), so debug kernels
still point at the caller, while production kernels, including ones
running with panic_on_warn, just fail the call.

Assisted-by: LLM
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 net/core/skbuff.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 43ebe61c7fc4..d226fe77d484 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -2303,9 +2303,15 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail,
 	u8 *data;
 	int i;
 
-	BUG_ON(nhead < 0);
+	if (unlikely(nhead < 0)) {
+		DEBUG_NET_WARN_ON_ONCE(1);
+		return -EINVAL;
+	}
 
-	BUG_ON(skb_shared(skb));
+	if (unlikely(skb_shared(skb))) {
+		DEBUG_NET_WARN_ON_ONCE(1);
+		return -EINVAL;
+	}
 
 	skb_zcopy_downgrade_managed(skb);
 

-- 
2.55.0

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

[Josef Bacik]

skb_segment()'s frag_list walk assumes the GRO-shaped layout it expects
and BUG()s when the layout doesn't match.  Anybody who can get a
malformed GSO skb to a segmentation point gets to crash the box.  Most
recently commit d5dc1e69fd72 ("inet: frags: strip GSO state from
fragments before reassembly") fixed one that an unprivileged user could
trigger with two writes to a tap device in their own user namespace.
commit 3382a1ed7f77 ("net: fix udp gso skb_segment after pull from
frag_list") fixed another.

skb_segment() already has an error path for a bad layout: the
too-many-frags check sets -EINVAL and frees the partial segment list.
Take that path for the four layout checks, with a
DEBUG_NET_WARN_ON_ONCE() for debug kernels.  The packet gets dropped,
which is what should happen to a packet we can't segment.

The one check that runs after skb_clone() and before the clone is
linked into the segment list frees the clone itself.

Assisted-by: LLM
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 net/core/skbuff.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index d226fe77d484..a6821ab13969 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;
+			}
 
 			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;
+				}
 
 				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;
+					}
 
 					/* to make room for head_frag. */
 					i--;

-- 
2.55.0

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

[Josef Bacik]

skb_copy(), skb_copy_expand() and skb_try_coalesce() all BUG() if
skb_copy_bits() fails.  skb_copy_bits() only fails when the skb's
lengths don't add up, which is a bug somewhere else, usually in a
driver building the skb.

Each of these functions already has a failure return its callers handle:

 - skb_copy() and skb_copy_expand() free the new skb and return NULL,
   as they do when the allocation fails.
 - skb_try_coalesce() returns false and the caller keeps the skbs
   separate.  Copy into the tailroom before skb_put() so that @to is
   untouched on failure.

Take those returns, with a DEBUG_NET_WARN_ON_ONCE() for debug kernels.

__pskb_pull_tail() has the same BUG_ON(), but several of its callers
can't otherwise fail and don't check its return, so it's left for a
separate change that fixes them first.

Assisted-by: LLM
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 net/core/skbuff.c | 27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index a6821ab13969..ffc78b1a8ab8 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -2201,7 +2201,12 @@ struct sk_buff *skb_copy(const struct sk_buff *skb, gfp_t gfp_mask)
 	/* Set the tail pointer and length */
 	skb_put(n, skb->len);
 
-	BUG_ON(skb_copy_bits(skb, -headerlen, n->head, headerlen + skb->len));
+	if (unlikely(skb_copy_bits(skb, -headerlen, n->head,
+				   headerlen + skb->len))) {
+		DEBUG_NET_WARN_ON_ONCE(1);
+		kfree_skb(n);
+		return NULL;
+	}
 
 	skb_copy_header(n, skb);
 	return n;
@@ -2545,8 +2550,13 @@ struct sk_buff *skb_copy_expand(const struct sk_buff *skb,
 		head_copy_off = newheadroom - head_copy_len;
 
 	/* Copy the linear header and data. */
-	BUG_ON(skb_copy_bits(skb, -head_copy_len, n->head + head_copy_off,
-			     skb->len + head_copy_len));
+	if (unlikely(skb_copy_bits(skb, -head_copy_len,
+				   n->head + head_copy_off,
+				   skb->len + head_copy_len))) {
+		DEBUG_NET_WARN_ON_ONCE(1);
+		kfree_skb(n);
+		return NULL;
+	}
 
 	skb_copy_header(n, skb);
 
@@ -6233,8 +6243,15 @@ bool skb_try_coalesce(struct sk_buff *to, struct sk_buff *from,
 		return false;
 
 	if (len <= skb_tailroom(to) && skb_frags_readable(from)) {
-		if (len)
-			BUG_ON(skb_copy_bits(from, 0, skb_put(to, len), len));
+		if (len) {
+			if (unlikely(skb_copy_bits(from, 0,
+						   skb_tail_pointer(to),
+						   len))) {
+				DEBUG_NET_WARN_ON_ONCE(1);
+				return false;
+			}
+			skb_put(to, len);
+		}
 		*delta_truesize = 0;
 		return true;
 	}

-- 
2.55.0

[PATCH net-next v2 4/8] net: skbuff: don't BUG() on leftover length in skb_checksum() and friends

[Josef Bacik]

skb_checksum(), skb_crc32c() and __skb_to_sgvec() walk the head, frags
and frag_list and BUG() if they run out of skb before they run out of
@len.  By then nothing has been read past the end of the skb.  The walk
stopped at the end of the data, and the check only tells us the caller
asked for a range the skb doesn't have.  commit 06a0afcfe2f5 ("xfrm: do
pskb_pull properly in __xfrm_transport_prep") fixed one such caller that
crashed in __skb_to_sgvec().

Return what each function already returns when it can't do the work,
with a DEBUG_NET_WARN_ON_ONCE() for debug kernels:

 - __skb_to_sgvec() returns -EINVAL.  Every skb_to_sgvec() caller has
   checked for a negative return since it learned to return -EMSGSIZE.
 - skb_checksum() and skb_crc32c() return 0, as they already do for
   unreadable frags.  The resulting checksum is wrong, so the packet
   fails verification on receive or goes out with a bad checksum on
   transmit, rather than taking the machine down.

Assisted-by: LLM
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 net/core/skbuff.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index ffc78b1a8ab8..32d7f5ed25eb 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3619,7 +3619,10 @@ __wsum skb_checksum(const struct sk_buff *skb, int offset, int len, __wsum csum)
 		}
 		start = end;
 	}
-	BUG_ON(len);
+	if (unlikely(len)) {
+		DEBUG_NET_WARN_ON_ONCE(1);
+		return 0;
+	}
 
 	return csum;
 }
@@ -3780,7 +3783,10 @@ u32 skb_crc32c(const struct sk_buff *skb, int offset, int len, u32 crc)
 		}
 		start = end;
 	}
-	BUG_ON(len);
+	if (unlikely(len)) {
+		DEBUG_NET_WARN_ON_ONCE(1);
+		return 0;
+	}
 
 	return crc;
 }
@@ -5339,7 +5345,10 @@ __skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len,
 		}
 		start = end;
 	}
-	BUG_ON(len);
+	if (unlikely(len)) {
+		DEBUG_NET_WARN_ON_ONCE(1);
+		return -EINVAL;
+	}
 	return elt;
 }
 

-- 
2.55.0

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

[Josef Bacik]

skb_copy_and_csum_dev() copies everything up to the checksum start out
of the linear area, and BUG()s if the checksum start is past the end of
the linear area.  It misses the other direction: a CHECKSUM_PARTIAL skb
that has been pulled past its csum_start gives a negative offset, which
gets past the check and becomes a ~4GB copy.  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.

The function returns void and its callers are drivers copying a frame
into a bounce buffer just before handing it to the hardware.  There's
nothing for them to back out of, so check both ends of csum_start and
that the checksum field fits in the frame, and if not, copy the whole
frame with skb_copy_bits() without filling in the checksum, with a
DEBUG_NET_WARN_ON_ONCE() for debug kernels.  The frame
goes out with a bad checksum and is dropped by the receiver.  If even
that copy fails, zero the buffer so the driver doesn't send stale bytes.

Assisted-by: LLM
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 net/core/skbuff.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 32d7f5ed25eb..c896770203f7 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) ||
+		     (skb->ip_summed == CHECKSUM_PARTIAL &&
+		      csstart + skb->csum_offset + sizeof(__sum16) >
+		      skb->len))) {
+		DEBUG_NET_WARN_ON_ONCE(1);
+		/* Send the frame without the checksum filled in, or send
+		 * zeroes if we can't even copy it.
+		 */
+		if (skb_copy_bits(skb, 0, to, skb->len))
+			memset(to, 0, skb->len);
+		return;
+	}
 
 	skb_copy_from_linear_data(skb, to, csstart);
 

-- 
2.55.0

[PATCH net-next v2 6/8] net: skbuff: don't BUG() on leftover length in skb_copy_and_csum_bits()

[Josef Bacik]

skb_copy_and_csum_bits() has the same check as skb_checksum(): it BUG()s
if it runs out of skb before it runs out of @len.  This one gets hit:
commit 7d63b6712538 ("icmp: guard against too small mtu") and commit
f99cd56230f5 ("net: Remove acked SYN flag from packet in the transmit
queue correctly") each fixed a crash here from icmp_glue_bits().

It can't just return, though.  Its callers copy into a buffer that is
about to go out on the wire, an ICMP error quoting the offending packet
for example, so bailing out early would send whatever was left in the
rest of that buffer.

Zero the part of the buffer we didn't fill and return 0, with a
DEBUG_NET_WARN_ON_ONCE() for debug kernels.  As with skb_checksum(), the
checksum usually won't match the data, so the receiver will usually drop
the packet, but either way it carries nothing it shouldn't.  @len is an
int, so only zero when it is positive; a negative @len from a broken
caller must not turn into a huge memset().

Assisted-by: LLM
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 net/core/skbuff.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index c896770203f7..7fd2f8142cc4 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3713,7 +3713,13 @@ __wsum skb_copy_and_csum_bits(const struct sk_buff *skb, int offset,
 		}
 		start = end;
 	}
-	BUG_ON(len);
+	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 0;
+	}
 	return csum;
 }
 EXPORT_SYMBOL(skb_copy_and_csum_bits);

-- 
2.55.0

[PATCH net-next v2 7/8] net: skbuff: don't BUG() on a missing head_frag in skb_zerocopy()

[Josef Bacik]

skb_zerocopy() BUG()s if @from has no head_frag and the caller passed
hlen == 0, meaning the caller didn't ask for the head to be copied and
the head can't be referenced as a page either.  The check runs before
anything is touched, and skb_zerocopy() already documents -EFAULT for
bad skb geometry.  Return that, with a DEBUG_NET_WARN_ON_ONCE() for
debug kernels.

Assisted-by: LLM
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 net/core/skbuff.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 7fd2f8142cc4..629de22d98e4 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3913,7 +3913,10 @@ skb_zerocopy(struct sk_buff *to, struct sk_buff *from, int len, int hlen)
 	struct page *page;
 	unsigned int offset;
 
-	BUG_ON(!from->head_frag && !hlen);
+	if (unlikely(!from->head_frag && !hlen)) {
+		DEBUG_NET_WARN_ON_ONCE(1);
+		return -EFAULT;
+	}
 
 	/* dont bother with small payloads */
 	if (len <= skb_tailroom(to))

-- 
2.55.0

[PATCH net-next v2 8/8] net: skbuff: remove the BUG_ON()s from skb_shift()

[Josef Bacik]

skb_shift() has two BUG_ON()s.  The first fires if the caller asks to
shift more than @skb holds.  Nothing has been touched yet, and returning
0 already means "shifted nothing", which the TCP callers handle by
falling back.  Return 0, with a DEBUG_NET_WARN_ON_ONCE() for debug
kernels.

The second fires if the frags run out before @shiftlen does, but it
only checks after the shift has been committed to both skbs, when
there's nothing left to back out to.  The loop that builds the new frag
layout only writes @tgt's frag slots past its nr_frags, and the one
branch that modifies @skb's frags also finishes the shift.  So if the
loop ends with bytes still left to shift, nothing visible has changed
yet.  That's the same state the MAX_SKB_FRAGS bail-out inside the loop
returns 0 from.  Move the check up to just before the commit and
return 0 there the same way.

Assisted-by: LLM
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 net/core/skbuff.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 629de22d98e4..7d23c2d10550 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -4331,7 +4331,10 @@ int skb_shift(struct sk_buff *tgt, struct sk_buff *skb, int shiftlen)
 	int from, to, merge, todo;
 	skb_frag_t *fragfrom, *fragto;
 
-	BUG_ON(shiftlen > skb->len);
+	if (unlikely(shiftlen > skb->len)) {
+		DEBUG_NET_WARN_ON_ONCE(1);
+		return 0;
+	}
 
 	if (skb_headlen(skb))
 		return 0;
@@ -4411,6 +4414,14 @@ int skb_shift(struct sk_buff *tgt, struct sk_buff *skb, int shiftlen)
 		}
 	}
 
+	/* The frags ran out before shiftlen did.  Nothing has been committed
+	 * yet, so back out.
+	 */
+	if (unlikely(todo > 0)) {
+		DEBUG_NET_WARN_ON_ONCE(1);
+		return 0;
+	}
+
 	/* Ready to "commit" this state change to tgt */
 	skb_shinfo(tgt)->nr_frags = to;
 
@@ -4428,8 +4439,6 @@ int skb_shift(struct sk_buff *tgt, struct sk_buff *skb, int shiftlen)
 		skb_shinfo(skb)->frags[to++] = skb_shinfo(skb)->frags[from++];
 	skb_shinfo(skb)->nr_frags = to;
 
-	BUG_ON(todo > 0 && !skb_shinfo(skb)->nr_frags);
-
 onlymerged:
 	/* Most likely the tgt won't ever need its checksum anymore, skb on
 	 * the other hand might need it if it needs to be resent

-- 
2.55.0

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

[netdev-bot+sashiko]

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

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

[netdev-bot+sashiko]

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.

> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index a6821ab139691..ffc78b1a8ab85 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -2201,7 +2201,12 @@ struct sk_buff *skb_copy(const struct sk_buff *skb, gfp_t gfp_mask)
>  	/* Set the tail pointer and length */
>  	skb_put(n, skb->len);
>
> -	BUG_ON(skb_copy_bits(skb, -headerlen, n->head, headerlen + skb->len));
> +	if (unlikely(skb_copy_bits(skb, -headerlen, n->head,
> +				   headerlen + skb->len))) {
> +		DEBUG_NET_WARN_ON_ONCE(1);
> +		kfree_skb(n);
> +		return NULL;
> +	}
>
>  	skb_copy_header(n, skb);
>  	return n;

[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.

[ ... ]

> @@ -6233,8 +6243,15 @@ bool skb_try_coalesce(struct sk_buff *to, struct sk_buff *from,
>  		return false;
>
>  	if (len <= skb_tailroom(to) && skb_frags_readable(from)) {
> -		if (len)
> -			BUG_ON(skb_copy_bits(from, 0, skb_put(to, len), len));
> +		if (len) {
> +			if (unlikely(skb_copy_bits(from, 0,
> +						   skb_tail_pointer(to),
> +						   len))) {
> +				DEBUG_NET_WARN_ON_ONCE(1);
> +				return false;
> +			}
> +			skb_put(to, len);

[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?

> +		}
>  		*delta_truesize = 0;
>  		return true;
>  	}

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

Re: [PATCH net-next v2 4/8] net: skbuff: don't BUG() on leftover length in skb_checksum() and friends

[netdev-bot+sashiko]

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] skb_checksum() and skb_crc32c() now return 0 when they run out
  of skb before @len.
- [Low] The fix is incomplete for one sibling walker in another file.
- [Low] The kernel-doc for skb_to_sgvec() still says it returns "either
  the number of scatterlist items used, or -EMSGSIZE if the contents…

--

net: skbuff: don't BUG() on leftover length in skb_checksum() and friends

This patch replaces the trailing BUG_ON(len) in skb_checksum(),
skb_crc32c() and __skb_to_sgvec() with DEBUG_NET_WARN_ON_ONCE(). In
that case skb_checksum() and skb_crc32c() now return 0, and
__skb_to_sgvec() returns -EINVAL.

> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index ffc78b1a8ab85..32d7f5ed25eb4 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -3619,7 +3619,10 @@ __wsum skb_checksum(const struct sk_buff *skb, int offset, int len, __wsum csum)
>  		}
>  		start = end;
>  	}
> -	BUG_ON(len);
> +	if (unlikely(len)) {
> +		DEBUG_NET_WARN_ON_ONCE(1);
> +		return 0;
> +	}

[Severity: Medium]
Is 0 a safe value to return here? 0 is also a valid checksum. Returning
it drops the caller's seed and the partial sum, so callers can't tell a
failed walk from a real result.

The commit message says:

  The resulting checksum is wrong, so the packet fails verification on
  receive or goes out with a bad checksum on transmit

However, __skb_checksum_complete() adds the pseudo-header in skb->csum
after the call:

net/core/skbuff.c:__skb_checksum_complete() {
	csum = skb_checksum(skb, 0, skb->len, 0);

	sum = csum_fold(csum_add(skb->csum, csum));
    ...
	if (!skb_shared(skb)) {
		/* Save full packet checksum */
		skb->csum = csum;
		skb->ip_summed = CHECKSUM_COMPLETE;
		skb->csum_complete_sw = 1;
		skb->csum_valid = !sum;
	}

	return sum;
}

When csum is 0, sum becomes csum_fold(skb->csum). That is 0 whenever the
pseudo-header sum, which depends only on saddr, daddr, len and proto,
folds to 0xffff.

In that case, would tcp_checksum_complete() and the UDP checksum paths
accept a packet whose payload was never fully checksummed? The skb would
also have csum_valid set and a wrong CHECKSUM_COMPLETE value in
skb->csum.

skb_crc32c() has a similar problem through sctp_compute_cksum():

include/net/sctp/checksum.h:sctp_compute_cksum() {
    ...
	new = ~skb_crc32c(skb, offset, skb->len - offset, ~0);
    ...
}

A 0 return becomes 0xffffffff whatever the packet contains. So
sctp_rcv_checksum() would accept the packet whenever the checksum on the
wire equals that value.

Callers that pass their seed into skb_checksum(), such as
__skb_checksum_complete_head(), do fail as described, because
csum_fold(0) is 0xffff.

Hitting this still needs an skb whose skb->len is larger than the data
in head, frags and frag_list. Before this patch, the same case hit the
BUG().

[ ... ]

> @@ -5339,7 +5345,10 @@ __skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len,
>  		}
>  		start = end;
>  	}
> -	BUG_ON(len);
> +	if (unlikely(len)) {
> +		DEBUG_NET_WARN_ON_ONCE(1);
> +		return -EINVAL;
> +	}
>  	return elt;
>  }

[Severity: Low]
Should __skb_nsg() in net/tls/tls_sw.c get the same change? It copies
this head/frags/frag_list walk, and it still ends with:

net/tls/tls_sw.c:__skb_nsg() {
    ...
        BUG_ON(len);
        return elt;
}

tls_decrypt_sg() calls skb_nsg() on the same range that it later passes
to skb_to_sgvec():

	n_sgin = skb_nsg(skb, rxm->offset + prot->prepend_size,
			 rxm->full_len - prot->prepend_size);
    ...
	err = skb_to_sgvec(skb, &sgin[1],
			   rxm->offset + prot->prepend_size,
			   rxm->full_len - prot->prepend_size);
	if (err < 0)
		goto exit_free;

If that range ever ran past the end of the skb, the path
tls_decrypt_sg()->skb_nsg()->__skb_nsg() would still BUG(). That happens
before the new -EINVAL path in __skb_to_sgvec() is reached. The commit
message doesn't mention __skb_nsg().

A later commit in the series, "net: skbuff: don't BUG() on leftover
length in skb_copy_and_csum_bits()", covers the skb_copy_and_csum_bits()
case. After that, the TLS copy seems to be the only one left.

[Severity: Low]
This isn't a bug, but the kernel-doc for skb_to_sgvec() still says:

 *	region of the buffer space attached to a socket buffer. Returns either
 *	the number of scatterlist items used, or -EMSGSIZE if the contents
 *	could not fit.

skb_to_sgvec() and skb_to_sgvec_nomark() return this new -EINVAL to
their callers unchanged. Could the comment mention it as well? Nothing
later in the series updates it.

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

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]

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

Re: [PATCH net-next v2 6/8] net: skbuff: don't BUG() on leftover length in skb_copy_and_csum_bits()

[netdev-bot+sashiko]

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

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

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).

- [High] The patch zero-fills the destination on only one of
  skb_copy_and_csum_bits()'s two short-copy exits (net/core/skbuff.c).
- [Low] The commit message justifies the new behaviour with "Its callers
  copy into a buffer that is about to go out on the wire, an ICMP error…

--

net: skbuff: don't BUG() on leftover length in skb_copy_and_csum_bits()

This patch replaces the BUG_ON(len) at the end of skb_copy_and_csum_bits()
with a DEBUG_NET_WARN_ON_ONCE(). It zero-fills the part of the destination
buffer that was not copied and returns a checksum of 0, so a short copy
should not crash and should not leave stale bytes in the buffer.

> It can't just return, though.  Its callers copy into a buffer that is
> about to go out on the wire, an ICMP error quoting the offending packet
> for example, so bailing out early would send whatever was left in the
> rest of that buffer.

[Severity: Low]
Does this hold for every caller? xdr_skb_read_bits() in
net/sunrpc/socklib.c is on the receive path. It copies a received UDP skb
into RPC receive buffers:

net/sunrpc/socklib.c:xdr_skb_read_bits() {
    ...
		csum = skb_copy_and_csum_bits(desc->skb, desc->offset, to, len);
		desc->csum = csum_block_add(desc->csum, csum, desc->offset);
    ...
}

With this change, that path would put zeros in the RPC receive buffer and
get a partial checksum of 0. Only the later check in
csum_partial_copy_to_xdr() rejects the result:

	if (csum_fold(desc.csum))
		return -1;

This is still better than BUG(). Could the commit message also mention that
receive-side callers are affected?

> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index c896770203f71..7fd2f8142cc41 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -3713,7 +3713,13 @@ __wsum skb_copy_and_csum_bits(const struct sk_buff *skb, int offset,
>  		}
>  		start = end;
>  	}
> -	BUG_ON(len);
> +	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 0;
> +	}

[Severity: High]
Does this cover every short-copy exit in skb_copy_and_csum_bits()? Right
after the linear head copy, the function still has:

	if (!skb_frags_readable(skb))
		return 0;

That exit leaves the remaining len bytes at to unwritten. So the stale-bytes
case the commit message describes can still happen in this function.

As one example, take an ICMP error that quotes a header-split skb with
unreadable (devmem) frags, where only the headers are in the linear area.
__icmp_send() caps the quote by skb->len and room, not by skb_headlen():

	icmp_param->data_len = icmp_param->skb->len - icmp_param->offset;
	if (icmp_param->data_len > room)
		icmp_param->data_len = room;

The call chain would then be:

__icmp_send()
  ip_append_data()
    __ip_append_data()        /* reserves non-zeroed skb data */
      icmp_glue_bits()
        skb_copy_and_csum_bits()
          if (!skb_frags_readable(skb))
                  return 0;

icmp_glue_bits() uses the return value only as a checksum and reports
success. icmp_push_reply() then calls ip_push_pending_frames().

Could this send several hundred bytes of uninitialized skb head memory on
the wire in the ICMP quote?

The frag_list recursion seems to have the same gap. If skb_gro_receive()
chains an unreadable skb onto frag_list, the recursive call returns 0
without writing anything:

			csum2 = skb_copy_and_csum_bits(frag_iter,
						       offset - start,
						       to, copy);

The outer loop still does len -= copy and to += copy, so the new memset()
at the end never covers that slice.

The unreadable early return predates this patch. It came from commit
65249feb6b3d ("net: add support for skbs with unreadable frags"). However,
the new comment and the commit message's "either way it carries nothing it
shouldn't" both depend on that exit being handled too.

skb_checksum() treats the same condition as an error:

	if (WARN_ON_ONCE(!skb_frags_readable(skb)))
		return 0;

Would it make sense to send the unreadable exit through the same guarded
zero-fill, for example with a shared label? That would also cover the
frag_list recursion, since each recursive call would zero its own slice.

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

Re: [PATCH net-next v2 7/8] net: skbuff: don't BUG() on a missing head_frag in skb_zerocopy()

[netdev-bot+sashiko]

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

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

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 calls `!from->head_frag && !hlen` in
  skb_zerocopy() a caller bug or "bad skb geometry".
- [Low] The patch adds a new -EFAULT return to skb_zerocopy() without
  updating its kernel-doc.

--

net: skbuff: don't BUG() on a missing head_frag in skb_zerocopy()

This patch replaces the BUG_ON() in skb_zerocopy(), which fires when @from
has no head_frag and hlen is 0. In its place is a DEBUG_NET_WARN_ON_ONCE()
and an -EFAULT return. The commit message describes the condition as bad
skb geometry passed in by the caller.

> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 7fd2f8142cc41..629de22d98e48 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -3913,7 +3913,10 @@ skb_zerocopy(struct sk_buff *to, struct sk_buff *from, int len, int hlen)
>  	struct page *page;
>  	unsigned int offset;
>  
> -	BUG_ON(!from->head_frag && !hlen);
> +	if (unlikely(!from->head_frag && !hlen)) {
> +		DEBUG_NET_WARN_ON_ONCE(1);
> +		return -EFAULT;
> +	}
>  
>  	/* dont bother with small payloads */
>  	if (len <= skb_tailroom(to))

[Severity: Medium]
Is this condition always a caller bug? The commit message says:

  "The check runs before anything is touched, and skb_zerocopy() already
  documents -EFAULT for bad skb geometry."

An ordinary zero-length packet from Open vSwitch seems able to reach it:

ovs_packet_cmd_execute()
  __dev_alloc_skb(NET_IP_ALIGN + len, GFP_KERNEL) /* kmalloc head */
  ... POP_ETH action on a 14 byte untagged frame
    pop_eth()->skb_eth_pop()->skb_pull_rcsum(skb, ETH_HLEN) /* len 0 */
  ... USERSPACE action
    output_userspace()->ovs_dp_upcall()->queue_userspace_packet()
      skb_zerocopy(user_skb, skb, 0, 0)

Because the allocation uses GFP_KERNEL, the head comes from kmalloc and
head_frag stays 0.

In queue_userspace_packet(), skb_len = min(skb->len, cutlen) is 0, and
hlen is 0 on both branches:

	if (dp->user_features & OVS_DP_F_UNALIGNED)
		hlen = min(skb_zerocopy_headlen(skb), cutlen);
	else
		hlen = skb_len;

When a !head_frag skb has no linear data, skb_zerocopy_headlen() returns
from->len, which is 0 here.

OVS_PACKET_CMD_EXECUTE uses GENL_UNS_ADMIN_PERM, so CAP_NET_ADMIN inside a
user namespace is enough to get here. With the old BUG_ON(), that made
this a crash that userspace could trigger.

Should the commit message describe this path and carry a Fixes: tag and a
stable note, so stable kernels pick up the change?

With len == 0, the small-payload fast path below always copies zero bytes
and returns 0. That is what an skb with head_frag already gets.

After this patch, the same request returns -EFAULT for kmalloc-head skbs.
The upcall is then dropped and counted in n_lost. On CONFIG_DEBUG_NET
kernels it also fires a WARN that userspace can trigger, and with
panic_on_warn set that WARN panics the machine.

nfqnl_build_packet_message() never hits this because it only calls
skb_zerocopy() when data_len is non-zero. queue_userspace_packet() has no
such guard.

Could the check move below the "len <= skb_tailroom(to)" fast path? Or
could it be enforced only when the head really has to be referenced as a
page, i.e. hlen == 0 && plen > 0?

[Severity: Low]
This isn't a bug, but the kernel-doc for skb_zerocopy() still gives only
these causes for -EFAULT:

 *	-EFAULT: skb_copy_bits() found some problem with skb geometry, or readable head
 *      payload would be mixed with unreadable frags.

The new early return gives -EFAULT before skb_copy_bits() or any frag
readability check runs.

Should the Return value section mention this case? It could also state the
precondition the old BUG_ON() enforced: hlen must be non-zero when @from
has no head_frag.

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