[PATCH net-next 0/9] net: skbuff: convert most BUG_ON()s to WARN_ON_ONCE() and an error

[PATCH net-next 0/9] net: skbuff: convert most BUG_ON()s to WARN_ON_ONCE() and an error

[Josef Bacik]

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 (9):
      net: skbuff: don't leave stale bytes in skb_copy_and_csum_bits()
      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 | 98 +++++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 77 insertions(+), 21 deletions(-)
---
base-commit: 8b4e7209c842d8cb9516f1f5ef0a88aa2d8831a6
change-id: 20261006-b4-skbuff-bug-on-b2844a487925

[PATCH net-next 1/9] net: skbuff: don't leave stale bytes in skb_copy_and_csum_bits()

[Josef Bacik]

When skb_copy_and_csum_bits() reaches unreadable frags it returns 0
after copying only the linear part, and the rest of the caller's buffer
is left as it was.  The callers copy into a buffer that is about to go
out on the wire: an ICMP error quoting the offending packet, or a
driver's TX bounce buffer in skb_copy_and_csum_dev().  Neither buffer
is zeroed beforehand, so whatever was in memory there gets sent.

Zero the part of the buffer we didn't fill.  The checksum is already
wrong in this case, so the packet still gets dropped by the receiver,
it just doesn't carry anything it shouldn't.  Only zero for a positive
@len, a negative one from a broken caller must not turn into a huge
memset().

Fixes: 65249feb6b3d ("net: add support for skbs with unreadable frags")
Assisted-by: LLM
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 net/core/skbuff.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 5c4024a03e10..512ff9cfa269 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3633,8 +3633,12 @@ __wsum skb_copy_and_csum_bits(const struct sk_buff *skb, int offset,
 		pos	= copy;
 	}
 
-	if (!skb_frags_readable(skb))
+	if (!skb_frags_readable(skb)) {
+		/* Don't hand the caller a buffer with stale bytes in it. */
+		if (len > 0)
+			memset(to, 0, len);
 		return 0;
+	}
 
 	for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) {
 		int end;

-- 
2.55.0

[PATCH net-next 2/9] 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.  Warn once and return
-EINVAL instead of crashing.  Anybody running with panic_on_warn, which
includes syzbot, still stops right here.

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

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 512ff9cfa269..5d856948cef9 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -2303,9 +2303,11 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail,
 	u8 *data;
 	int i;
 
-	BUG_ON(nhead < 0);
+	if (WARN_ON_ONCE(nhead < 0))
+		return -EINVAL;
 
-	BUG_ON(skb_shared(skb));
+	if (WARN_ON_ONCE(skb_shared(skb)))
+		return -EINVAL;
 
 	skb_zcopy_downgrade_managed(skb);
 

-- 
2.55.0

[PATCH net-next 3/9] 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.
Warn once and take that path for the four layout checks.  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 | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 5d856948cef9..405d27e9bc9d 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -4916,7 +4916,10 @@ 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 (WARN_ON_ONCE(skb_headlen(list_skb) > len)) {
+				err = -EINVAL;
+				goto err;
+			}
 
 			nskb = skb_clone(list_skb, GFP_ATOMIC);
 			if (unlikely(!nskb))
@@ -4929,7 +4932,11 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
 			pos += skb_headlen(list_skb);
 
 			while (pos < offset + len) {
-				BUG_ON(i >= nfrags);
+				if (WARN_ON_ONCE(i >= nfrags)) {
+					kfree_skb(nskb);
+					err = -EINVAL;
+					goto err;
+				}
 
 				size = skb_frag_size(frag);
 				if (pos + size > offset + len)
@@ -5036,9 +5043,15 @@ 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 (WARN_ON_ONCE(!nfrags)) {
+						err = -EINVAL;
+						goto err;
+					}
 				} else {
-					BUG_ON(!list_skb->head_frag);
+					if (WARN_ON_ONCE(!list_skb->head_frag)) {
+						err = -EINVAL;
+						goto err;
+					}
 
 					/* to make room for head_frag. */
 					i--;

-- 
2.55.0

[PATCH net-next 4/9] 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.

Warn once and take those returns.

__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 | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 405d27e9bc9d..6cd7135e0dce 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -2201,7 +2201,11 @@ 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 (WARN_ON_ONCE(skb_copy_bits(skb, -headerlen, n->head,
+				       headerlen + skb->len))) {
+		kfree_skb(n);
+		return NULL;
+	}
 
 	skb_copy_header(n, skb);
 	return n;
@@ -2541,8 +2545,12 @@ 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 (WARN_ON_ONCE(skb_copy_bits(skb, -head_copy_len,
+				       n->head + head_copy_off,
+				       skb->len + head_copy_len))) {
+		kfree_skb(n);
+		return NULL;
+	}
 
 	skb_copy_header(n, skb);
 
@@ -6229,8 +6237,13 @@ 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 (WARN_ON_ONCE(skb_copy_bits(from, 0,
+						       skb_tail_pointer(to),
+						       len)))
+				return false;
+			skb_put(to, len);
+		}
 		*delta_truesize = 0;
 		return true;
 	}

-- 
2.55.0

[PATCH net-next 5/9] 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().

Warn once and return what each function already returns when it can't
do the work:

 - __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 | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 6cd7135e0dce..4070e0c25f63 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3613,7 +3613,8 @@ __wsum skb_checksum(const struct sk_buff *skb, int offset, int len, __wsum csum)
 		}
 		start = end;
 	}
-	BUG_ON(len);
+	if (WARN_ON_ONCE(len))
+		return 0;
 
 	return csum;
 }
@@ -3778,7 +3779,8 @@ u32 skb_crc32c(const struct sk_buff *skb, int offset, int len, u32 crc)
 		}
 		start = end;
 	}
-	BUG_ON(len);
+	if (WARN_ON_ONCE(len))
+		return 0;
 
 	return crc;
 }
@@ -5333,7 +5335,8 @@ __skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len,
 		}
 		start = end;
 	}
-	BUG_ON(len);
+	if (WARN_ON_ONCE(len))
+		return -EINVAL;
 	return elt;
 }
 

-- 
2.55.0

[PATCH net-next 6/9] 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, warn once and copy the whole
frame with skb_copy_bits() without filling in the checksum.  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 | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 4070e0c25f63..c8c2c0319b87 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3971,7 +3971,17 @@ 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 (WARN_ON_ONCE(csstart < 0 || csstart > skb_headlen(skb) ||
+			 (skb->ip_summed == CHECKSUM_PARTIAL &&
+			  csstart + skb->csum_offset + sizeof(__sum16) >
+			  skb->len))) {
+		/* 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 7/9] 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.

Warn once, zero the part of the buffer we didn't fill and return 0.  As
with skb_checksum(), the checksum is wrong and the packet gets dropped
by whoever receives it.  @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 | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index c8c2c0319b87..e29eda2eaf3f 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3709,7 +3709,12 @@ __wsum skb_copy_and_csum_bits(const struct sk_buff *skb, int offset,
 		}
 		start = end;
 	}
-	BUG_ON(len);
+	if (WARN_ON_ONCE(len)) {
+		/* 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 8/9] 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.  Warn once and return that.

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

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index e29eda2eaf3f..8c6a45a20eb0 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3906,7 +3906,8 @@ 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 (WARN_ON_ONCE(!from->head_frag && !hlen))
+		return -EFAULT;
 
 	/* dont bother with small payloads */
 	if (len <= skb_tailroom(to))

-- 
2.55.0

[PATCH net-next 9/9] 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.  Warn once and return 0.

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, warn once
and return 0 there.

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

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 8c6a45a20eb0..59f74850150b 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -4321,7 +4321,8 @@ 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 (WARN_ON_ONCE(shiftlen > skb->len))
+		return 0;
 
 	if (skb_headlen(skb))
 		return 0;
@@ -4401,6 +4402,12 @@ 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 (WARN_ON_ONCE(todo > 0))
+		return 0;
+
 	/* Ready to "commit" this state change to tgt */
 	skb_shinfo(tgt)->nr_frags = to;
 
@@ -4418,8 +4425,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 0/9] net: skbuff: convert most BUG_ON()s to WARN_ON_ONCE() and an error

[Willem de Bruijn]

Josef Bacik wrote:
> 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>;

Good idea. I was thinking of doing exactly this sweep after addressing
one case recently in commit ee1972def665 ("net: downgrade BUG_ON
EIOCBQUEUED in sock_sendmsg_nosec")

Instead of WARN_ON_ONCE, which still triggers a panic on systems with
panic_on_warn, DEBUG_NET_WARN_ON_ONCE?

Re: [PATCH net-next 1/9] net: skbuff: don't leave stale bytes in skb_copy_and_csum_bits()

[Willem de Bruijn]

Josef Bacik wrote:
> When skb_copy_and_csum_bits() reaches unreadable frags it returns 0
> after copying only the linear part, and the rest of the caller's buffer
> is left as it was.  The callers copy into a buffer that is about to go
> out on the wire: an ICMP error quoting the offending packet, or a
> driver's TX bounce buffer in skb_copy_and_csum_dev().  Neither buffer
> is zeroed beforehand, so whatever was in memory there gets sent.
> 
> Zero the part of the buffer we didn't fill.  The checksum is already
> wrong in this case, so the packet still gets dropped by the receiver,
> it just doesn't carry anything it shouldn't.  Only zero for a positive
> @len, a negative one from a broken caller must not turn into a huge
> memset().
> 
> Fixes: 65249feb6b3d ("net: add support for skbs with unreadable frags")
> Assisted-by: LLM
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

This should be a stand-alone fix sent to net (and stable)?

Re: [PATCH net-next 2/9] net: skbuff: don't BUG() on bad arguments to pskb_expand_head()

[Willem de Bruijn]

Josef Bacik wrote:
> 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.  Warn once and return
> -EINVAL instead of crashing.  Anybody running with panic_on_warn, which
> includes syzbot, still stops right here.
> 
> Assisted-by: LLM
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  net/core/skbuff.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 512ff9cfa269..5d856948cef9 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -2303,9 +2303,11 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail,
>  	u8 *data;
>  	int i;
>  
> -	BUG_ON(nhead < 0);
> +	if (WARN_ON_ONCE(nhead < 0))
> +		return -EINVAL;

Another option besides WARN_ON_ONCE or even DEBUG_NET_WARN_ON_ONCE
when returning an error is a net_warn_ratelimited for such cases.

>  
> -	BUG_ON(skb_shared(skb));
> +	if (WARN_ON_ONCE(skb_shared(skb)))
> +		return -EINVAL;
>  
>  	skb_zcopy_downgrade_managed(skb);
>  
> 
> -- 
> 2.55.0
> 

Re: [PATCH net-next 0/9] net: skbuff: convert most BUG_ON()s to WARN_ON_ONCE() and an error

[Fernando Fernandez Mancera]

On 10/7/26 4:48 PM, Willem de Bruijn wrote:
> Josef Bacik wrote:
>> 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>;
> 
> Good idea. I was thinking of doing exactly this sweep after addressing
> one case recently in commit ee1972def665 ("net: downgrade BUG_ON
> EIOCBQUEUED in sock_sendmsg_nosec")
> 
> Instead of WARN_ON_ONCE, which still triggers a panic on systems with
> panic_on_warn, DEBUG_NET_WARN_ON_ONCE?

I agree with using DEBUG_NET_WARN_ON_ONCE at least for paths that can be 
triggered from userspace. I did something similar in Netfilter subsystem 
not so long ago.

Re: [PATCH net-next 1/9] net: skbuff: don't leave stale bytes in skb_copy_and_csum_bits()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] skbuff: skb_copy_and_csum_bits() discards linear part csum on unreadable frags
--

commit 554d4ec9904f25474f32c877a62ca154d0b20324
Author: Josef Bacik <josef@toxicpanda.com>

net: skbuff: don't leave stale bytes in skb_copy_and_csum_bits()

This commit zeroes out the remainder of a caller's buffer when
skb_copy_and_csum_bits encounters unreadable fragments. This prevents
stale uninitialized memory from being leaked when the buffer is
transmitted on the wire.

> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 5c4024a03e105..512ff9cfa269f 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -3633,8 +3633,12 @@ __wsum skb_copy_and_csum_bits(const struct sk_buff *skb, int offset,
>  		pos	= copy;
>  	}
>  
> -	if (!skb_frags_readable(skb))
> +	if (!skb_frags_readable(skb)) {
> +		/* Don't hand the caller a buffer with stale bytes in it. */
> +		if (len > 0)
> +			memset(to, 0, len);
>  		return 0;

[Severity: Medium]
By returning 0 here instead of the accumulated csum, does this discard
the checksum computed for the successfully copied linear part of the packet?

When icmp_push_reply() uses skb_copy_and_csum_bits() via icmp_glue_bits()
to quote an offending packet with unreadable fragments, the offending
packet typically has its headers in the linear part. Returning 0 discards
this partial checksum.

Since the ICMP payload contains the non-zero linear part, returning 0
causes the ICMP packet's overall checksum to be computed incorrectly,
resulting in the error packet being silently dropped by the destination.

Would it be better to return csum here instead of 0, so that the checksum
is mathematically correct for the zero-padded buffer and allows the ICMP
error to be successfully delivered?

> +	}
>  
>  	for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) {
>  		int end;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261006-b4-skbuff-bug-on-v1-0-1b4434c5357c@toxicpanda.com?part=1

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] net: incomplete defense against negative len in skb_copy_and_csum_bits
--

commit a487a003d242af9e08ff639f329ea71c902adabb
Author: Josef Bacik <josef@toxicpanda.com>

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

This commit replaces a BUG_ON(len) with a WARN_ON_ONCE(len) at the end of
skb_copy_and_csum_bits(). It also clears any remaining space in the destination
buffer if len is positive, preventing stale bytes from being sent.

> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index c8c2c0319b87a..e29eda2eaf3f6 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -3709,7 +3709,12 @@ __wsum skb_copy_and_csum_bits(const struct sk_buff *skb, int offset,
>  		}
>  		start = end;
>  	}
> -	BUG_ON(len);
> +	if (WARN_ON_ONCE(len)) {
> +		/* Don't hand the caller a buffer with stale bytes in it. */
> +		if (len > 0)
> +			memset(to, 0, len);
> +		return 0;
> +	}

[Severity: High]
The commit message states that "a negative @len from a broken caller must not
turn into a huge memset()". If a broken caller passes a negative len, will we
actually reach this check, or will it cause memory corruption earlier in the
function?

Earlier in skb_copy_and_csum_bits(), if offset is smaller than
skb_headlen(skb), copy starts as positive. If a broken caller passes a
negative len, copy > len evaluates to true, forcing copy = len:

	if (copy > 0) {
		if (copy > len)
			copy = len;
		csum = csum_partial_copy_nocheck(skb->data + offset, to,
						 copy);

Could this pass the negative copy to csum_partial_copy_nocheck(), which
interprets it as a huge unsigned size and causes a massive out-of-bounds
read and write before ever reaching the new guard?

>  	return csum;
>  }
>  EXPORT_SYMBOL(skb_copy_and_csum_bits);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261006-b4-skbuff-bug-on-v1-0-1b4434c5357c@toxicpanda.com?part=7

Re: [PATCH net-next 1/9] net: skbuff: don't leave stale bytes 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's own claim that "the checksum is already wrong in
  this case, so the packet still gets dropped by the receiver" does not…
- [Low] The commit message names two wire paths that would send stale
  bytes: the ICMP error quote, and "a driver's TX bounce buffer in…

--

net: skbuff: don't leave stale bytes in skb_copy_and_csum_bits()

This patch zeroes the rest of the destination buffer in
skb_copy_and_csum_bits() when the function reaches unreadable frags.
Before, those bytes were left as they were. The commit message says
this affects ICMP error quoting and driver TX bounce buffers. It also
says the returned checksum is already wrong, so the receiver drops the
packet anyway.

> The callers copy into a buffer that is about to go
> out on the wire: an ICMP error quoting the offending packet, or a
> driver's TX bounce buffer in skb_copy_and_csum_dev().  Neither buffer
> is zeroed beforehand, so whatever was in memory there gets sent.

[Severity: Low]
Can the skb_copy_and_csum_dev() path see unreadable frags at all?

These drivers call skb_copy_and_csum_dev():

  ep93xx_eth, ec_bhf, ksz884x, 8139too, sc92031, via-rhine

The sunvnet TX path does too. None of them sets dev->netmem_tx, so they
are all NETMEM_TX_NONE.

validate_xmit_skb() calls validate_xmit_unreadable_skb() first, and
that frees these skbs before ndo_start_xmit is reached:

net/core/dev.c:validate_xmit_unreadable_skb() {
    ...
	if (dev->netmem_tx == NETMEM_TX_NONE)
		goto out_free;
    ...
}

Only bnxt, gve, mlx5, fbnic and netkit set netmem_tx. None of them
calls skb_copy_and_csum_dev().

The ICMP/ICMPv6 quote path through icmp_glue_bits() still justifies the
fix. Could the driver bounce buffer example be dropped from the commit
message? This is a Fixes-tagged patch, and the example could mislead
stable backporters.

> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 5c4024a03e105..512ff9cfa269f 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -3633,8 +3633,12 @@ __wsum skb_copy_and_csum_bits(const struct sk_buff *skb, int offset,
>  		pos	= copy;
>  	}
>  
> -	if (!skb_frags_readable(skb))
> +	if (!skb_frags_readable(skb)) {
> +		/* Don't hand the caller a buffer with stale bytes in it. */
> +		if (len > 0)
> +			memset(to, 0, len);
>  		return 0;
> +	}

[Severity: High]
The commit message says "The checksum is already wrong in this case, so
the packet still gets dropped by the receiver". Does that hold when
offset >= skb_headlen(skb), so nothing is copied from the linear area?

In that case the whole range is zeroed and 0 is returned. 0 is the
correct partial checksum for an all-zero buffer, so the data and the
checksum now agree.

In the !sg && !csum arm, skb_segment() stores this return value without
checking skb_frags_readable():

net/core/skbuff.c:skb_segment() {
    ...
				SKB_GSO_CB(nskb)->csum =
					skb_copy_and_csum_bits(head_skb, offset,
							       skb_put(nskb,
								       len),
							       len);
    ...
}

tcp_gso_segment() then folds it into the TCP checksum:

	th->check = gso_make_checksum(skb, ~th->check);

The result is a TCP checksum that is valid over a payload of zeros.

For a devmem TCP TX skb, the linear area holds only the headers and all
payload sits in net_iov frags. So every segment has offset >= headlen.

This path looks reachable after something like "ethtool -K dev sg off
tx off" on a NETMEM_TX_DMA device (bnxt, gve, mlx5, fbnic).
validate_xmit_unreadable_skb() lets the skb through, because netmem_tx
is DMA and the binding dev matches. Turning SG off also drops TSO, so
netif_needs_gso() is true and the skb takes this path:

validate_xmit_skb()->skb_gso_segment()->...->tcp_gso_segment()->
    skb_segment()->skb_copy_and_csum_bits()

skb_segment() runs with sg and csum both false.

skb_orphan_frags() returns 0 here because msg_zerocopy_alloc() sets
SKBFL_DONT_ORPHAN.

tcp_sendmsg_locked() checks NETIF_F_SG only at send time. Skbs already
in the write or retransmit queues are affected either way.

Would the peer then accept and ACK these zeroed segments as real stream
data? Before this patch the stale bytes did not match the returned 0,
so the segment was dropped.

Would it be better to return a checksum that is deliberately wrong
here? Another option is for skb_segment() to reject unreadable skbs, as
the csum arm already does when skb_copy_bits() returns -EFAULT.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261006-b4-skbuff-bug-on-v1-0-1b4434c5357c%40toxicpanda.com

Re: [PATCH net-next 3/9] 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 (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] Commit "net: skbuff: don't BUG() on a bad frag_list layout in
  skb_segment()" replaces four BUG_ON()s in skb_segment()…
- [Low] The comment in bpf_skb_net_grow() (net/core/filter.c:3698-3702)
  still says "There is a BUG_ON() when segmenting the frag_list with…
- [Low] The KUnit test gso_test_func() in net/core/net_test.c (lines
  223-227) turns off NETIF_F_SG for GSO_TEST_FRAG_LIST_NON_UNIFORM.

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but
  skb_segment() (net/core/skbuff.c) still dereferences list_skb without…

--

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

This patch replaces the four BUG_ON() frag_list layout checks in
skb_segment() with WARN_ON_ONCE(). Each one now takes the existing -EINVAL
error path. The check in the clone path runs after skb_clone(), so it also
frees the clone before bailing out.

> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 5d856948cef92..405d27e9bc9d1 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -4916,7 +4916,10 @@ 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 (WARN_ON_ONCE(skb_headlen(list_skb) > len)) {
> +				err = -EINVAL;
> +				goto err;
> +			}

[Severity: Medium]
The commit message says "Anybody who can get a malformed GSO skb to a
segmentation point gets to crash the box". Malformed input can reach all
four of these checks. Is WARN_ON_ONCE() the right tool for them?

If panic_on_warn is set (boot parameter or the kernel.panic_on_warn
sysctl), check_panic_on_warn() turns each of these warnings into a panic:

kernel/panic.c:check_panic_on_warn() {
	if (panic_on_warn)
		panic("%s: panic_on_warn set ...\n", origin);
	...
}

On those systems the crash is still there. On other systems a malformed
skb still taints the kernel and prints a stack trace.

The "Do not WARN lightly" section of coding-style.rst says WARN*() should
not be used for conditions that user space can trigger.

A little further down in the same loop, the too many frags check handles
a malformed layout with net_warn_ratelimited() and -EINVAL.

Would net_warn_ratelimited() or DEBUG_NET_WARN_ON_ONCE(), plus the -EINVAL
drop, be a better fit for all four converted checks?

[Severity: Low]
This isn't a bug, but a comment in bpf_skb_net_grow() still refers to the
BUG_ON() that this patch removes:

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.
		 */
}

After this patch skb_segment() has no BUG_ON() left. A lowered MSS now
makes this check return -EINVAL instead.

The skb_linearize() call is still needed, to avoid a dropped packet. Should
the comment give that reason instead?

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

[Severity: Low]
This isn't a bug, but gso_test_func() in net/core/net_test.c says SG is
turned off to avoid this BUG_ON():

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;
}

With this patch there is no BUG_ON(i >= nfrags) left. The check returns
ERR_PTR(-EINVAL) instead, and the test would report that through its
IS_ERR(segs) path.

Should this comment be updated? Would a negative test for the new -EINVAL
path also be worth adding?

>  
>  				size = skb_frag_size(frag);
>  				if (pos + size > offset + len)
> @@ -5036,9 +5043,15 @@ 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 (WARN_ON_ONCE(!nfrags)) {
> +						err = -EINVAL;
> +						goto err;
> +					}
>  				} else {
> -					BUG_ON(!list_skb->head_frag);
> +					if (WARN_ON_ONCE(!list_skb->head_frag)) {
> +						err = -EINVAL;
> +						goto err;
> +					}

[Severity: Medium]
This is a pre-existing issue, but what happens here if the frag_list is
already used up and list_skb is NULL?

Just above this hunk, the i >= nfrags branch in skb_segment() does:

	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_zcopy() checks skb before using it, so both helpers return 0 for a
NULL skb. Nothing catches the NULL list_skb.
skb_shinfo(list_skb)->nr_frags and skb_headlen(list_skb) then dereference
NULL before any of the new checks run.

pos only advances over each frag_list member's linear data and page
frags. It does not cover a nested frag_list. Take a member with headlen 0,
one page frag, a nested frag_list and a NULL next pointer. Could it leave
pos < offset + len after list_skb has become NULL?

Two other places make the same assumption: the skb_headlen(list_skb) test
in the clone path at the top of the loop, and len = list_skb->len for
GSO_BY_FRAGS.

GRO does not appear to build nested frag_lists today, because
dev_gro_receive() flushes skbs that already have a frag_list. No current
producer of this layout is known.

Since the goal is to drop malformed layouts rather than crash, would a
check like this at those places make sense?

	if (unlikely(!list_skb)) {
		err = -EINVAL;
		goto err;
	}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261006-b4-skbuff-bug-on-v1-0-1b4434c5357c%40toxicpanda.com

Re: [PATCH net-next 5/9] 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 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 skb_to_sgvec() kernel-doc in net/core/skbuff.c still says it
  "Returns either the number of scatterlist items used, or -EMSGSIZE if…

--

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

skb_checksum(), skb_crc32c() and __skb_to_sgvec() used to BUG() when
they ran out of skb data before @len was used up. This patch changes
each of them to warn once and return an error instead. __skb_to_sgvec()
now returns -EINVAL, and the two checksum helpers return 0.

> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 6cd7135e0dce2..4070e0c25f63e 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c

[ ... ]

> @@ -5333,7 +5335,8 @@ __skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len,
>  		}
>  		start = end;
>  	}
> -	BUG_ON(len);
> +	if (WARN_ON_ONCE(len))
> +		return -EINVAL;

[Severity: Low]
Should the skb_to_sgvec() kernel-doc be updated to match this new return
value? It still lists -EMSGSIZE as the only error:

net/core/skbuff.c:
 *	Fill the specified scatter-gather list with mappings/pointers into a
 *	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.
 */
int skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len)
{
	int nsg = __skb_to_sgvec(skb, sg, offset, len, 0);

	if (nsg <= 0)
		return nsg;

This -EINVAL is also passed up through the frag_list recursion by
"if (unlikely(ret < 0)) return ret;". skb_to_sgvec() and
skb_to_sgvec_nomark() then return it to their callers unchanged.

All in-tree callers check for any negative return, so nothing breaks
today. However, a caller written to the documented contract that only
checks for -EMSGSIZE would miss the new error.

The comment is still unchanged at the end of the series.

>  	return elt;
>  }
>

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261006-b4-skbuff-bug-on-v1-0-1b4434c5357c%40toxicpanda.com

Re: [PATCH net-next 6/9] 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 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 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] The commit message gets wrong what used to stop a bad
  csum_offset in skb_copy_and_csum_dev() (net/core/skbuff.c).

--

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

This patch replaces the BUG_ON() on csum_start in skb_copy_and_csum_dev()
with a WARN_ON_ONCE(). The new check also catches a negative csum_start
and a csum_offset that would put the checksum field past skb->len. When
the check fires, 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: Medium]
Is this description accurate? skb_copy_and_csum_bits() never sees
csum_offset, so it isn't clear how its BUG_ON(len) could have covered for
it.

skb_copy_and_csum_dev() passes it a length that depends only on csstart
and skb->len:

	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() fires only when the
head, frags and frag_list together hold less data than skb->len. If the
skb's lengths are consistent, it returns normally whatever csum_offset is.

The store after it had no bound before this patch:

	if (skb->ip_summed == CHECKSUM_PARTIAL) {
		long csstuff = csstart + skb->csum_offset;

		*((__sum16 *)(to + csstuff)) = csum_fold(csum);
	}

Without this patch, a CHECKSUM_PARTIAL skb with
csstart + csum_offset + 2 > skb->len would already write up to about 64KB
past the end of the driver's bounce buffer. The affected drivers are
8139too, sc92031, via-rhine, ksz884x, ep93xx and ec_bhf. Is that right?

A later patch in the series, "net: skbuff: don't BUG() on leftover length
in skb_copy_and_csum_bits()", does replace BUG_ON(len) with WARN_ON_ONCE()
plus a memset(). That doesn't change the point above.

If so, the new csum_offset condition fixes an out-of-bounds write that
already exists. It doesn't only guard against one that the later patch
would make possible. Could the commit message describe it that way, so
stable backporters can see it?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261006-b4-skbuff-bug-on-v1-0-1b4434c5357c%40toxicpanda.com

Re: [PATCH net-next 7/9] 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: 0 · Medium: 2 · 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] In net/core/skbuff.c, skb_copy_and_csum_bits() now has a new
  exhaustion branch (line 3712).
- [Medium] In net/core/skbuff.c, skb_segment()'s non-SG path has two
  branches for copying a segment's payload from head_skb.

--

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 WARN_ON_ONCE(). When the skb runs out before len does, the
uncopied rest of the destination buffer is now zero-filled and 0 is
returned as the checksum. The commit message says the wrong checksum
will make the receiver drop the packet.

> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index c8c2c0319b87a..e29eda2eaf3f6 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -3709,7 +3709,12 @@ __wsum skb_copy_and_csum_bits(const struct sk_buff *skb, int offset,
>  		}
>  		start = end;
>  	}
> -	BUG_ON(len);
> +	if (WARN_ON_ONCE(len)) {
> +		/* Don't hand the caller a buffer with stale bytes in it. */
> +		if (len > 0)
> +			memset(to, 0, len);
> +		return 0;
> +	}

[Severity: Medium]
Is the 0 returned here certain to give a wrong checksum? The commit
message says:

  As with skb_checksum(), the checksum is wrong and the packet gets
  dropped by whoever receives it.

Zero bytes add nothing to a ones' complement sum. So 0 is the correct
partial checksum for the buffer as written whenever the discarded csum
over the copied prefix was 0 mod 0xffff.

This always happens when nothing was copied, because offset was already
past the real data. It can also happen when the copied prefix sums to
zero, for example a full IPv4 header with a valid header checksum.

In the ICMP case named in the commit message, icmp_glue_bits() folds
the return value in and always reports success:

net/ipv4/icmp.c:icmp_glue_bits() {
    ...
	csum = skb_copy_and_csum_bits(icmp_param->skb,
				      icmp_param->offset + offset,
				      to, len);

	skb->csum = csum_block_add(skb->csum, csum, odd);
    ...
	return 0;
}

Then icmp_push_reply() does:

		icmph->checksum = csum_fold(csum);
		skb->ip_summed = CHECKSUM_NONE;
		ip_push_pending_frames(sk, fl4);

Could the ICMP error go out with a valid checksum over a zero-filled
quote?

icmpv6_getfrag() looks to be in the same position. So does
skb_copy_and_csum_dev(), which stores csum_fold() of the result in the
frame, and so does vnet_skb_shape() in sunvnet_common.c.

The commit message also says:

  Its callers copy into a buffer that is about to go out on the wire

That isn't true of xdr_skb_read_bits() in net/sunrpc/socklib.c, which
copies from a received skb:

	if (desc->need_checksum) {
		__wsum csum;

		csum = skb_copy_and_csum_bits(desc->skb, desc->offset, to, len);
		desc->csum = csum_block_add(desc->csum, csum, desc->offset);
	} else {
		if (unlikely(skb_copy_bits(desc->skb, desc->offset, to, len)))
			return 0;
	}

	desc->count -= len;
	desc->offset += len;
	return len;

The skb_copy_bits() branch notices the short copy and returns 0. The
checksum branch still advances by the full len. After that, only the
csum_fold(desc.csum) check in csum_partial_copy_to_xdr() can reject the
zero-filled RPC data, and whether it does depends on the data.

Could callers get a failure they can detect? Another option is to
return a value that can never match the real partial sum, so the
checksum is always wrong. Failing that, should the commit message be
reworded?

[Severity: Medium]
How should skb_segment() handle this now? Its non-SG path copies a
segment's payload from head_skb in one of two branches:

net/core/skbuff.c:skb_segment() {
    ...
		if (!sg) {
			if (!csum) {
				if (!nskb->remcsum_offload)
					nskb->ip_summed = CHECKSUM_NONE;
				SKB_GSO_CB(nskb)->csum =
					skb_copy_and_csum_bits(head_skb, offset,
							       skb_put(nskb,
								       len),
							       len);
				SKB_GSO_CB(nskb)->csum_start =
					skb_headroom(nskb) + doffset;
			} else {
				if (skb_copy_bits(head_skb, offset, skb_put(nskb, len), len))
					goto err;
			}
			continue;
		}
    ...
}

Take a head_skb that holds less data than skb->len says. The csum
branch drops the GSO skb through goto err. Before this patch, the !csum
branch (no SG and no checksum offload) hit BUG_ON(len). Now that branch
keeps a zero-filled payload, stores 0 in SKB_GSO_CB(nskb)->csum and
emits the segment.

gso_make_checksum() then builds the segment checksum from that value.
If a segment starts past the real data, 0 is the exact sum of its
all-zero payload. Can a zero-filled TCP or UDP segment then go out with
a valid checksum?

This seems to go against the earlier patch in this series, "net:
skbuff: don't BUG() on a bad frag_list layout in skb_segment()". That
patch says a packet that can't be segmented should be dropped through
the -EINVAL error path.

This commit message doesn't mention this caller. The later patches in
the series (skb_zerocopy, skb_shift) don't touch skb_segment().

Could skb_copy_and_csum_bits() report the failure so the !csum branch
can also goto err? Or could skb_segment() check head_skb before the
copy?

>  	return csum;
>  }
>  EXPORT_SYMBOL(skb_copy_and_csum_bits);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261006-b4-skbuff-bug-on-v1-0-1b4434c5357c%40toxicpanda.com

Re: [PATCH net-next 8/9] 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 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_zerocopy() now WARNs and returns -EFAULT on a legitimate,
  user-reachable input: an empty (len == 0) skb without head_frag.
- [Low] The kernel-doc 'Return value' section of skb_zerocopy() lists
  exactly two causes of -EFAULT: 'skb_copy_bits() found some problem…
- [Low] The patch removes a BUG() in skb_zerocopy() that userspace can
  actually reach, but the commit message has no Fixes: tag (the first…

--

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

This commit replaces the BUG_ON(!from->head_frag && !hlen) check in
skb_zerocopy() with a WARN_ON_ONCE() that returns -EFAULT. The commit
message describes the condition as a caller bug. It also says -EFAULT is
already documented for bad skb geometry.

> 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.  Warn once and return that.

[Severity: Low]
Should this carry a Fixes: tag?

Userspace can reach the BUG() removed here through openvswitch.
OVS_PACKET_CMD_EXECUTE only needs GENL_UNS_ADMIN_PERM. A 14-byte Ethernet
frame with the actions POP_ETH and then USERSPACE ends up here:

ovs_packet_cmd_execute()
  __dev_alloc_skb(NET_IP_ALIGN + len, GFP_KERNEL)  /* head_frag == 0 */
  ...
  do_execute_actions()
    pop_eth()                    /* skb->len goes from 14 to 0 */
    output_userspace()
      ovs_dp_upcall()
        queue_userspace_packet()
          skb_zerocopy(user_skb, skb, 0, 0)
            BUG_ON(!from->head_frag && !hlen)

There are two likely candidates for the tag. One is the commit that added
the BUG_ON() to skb_zerocopy(). The other is the commit that made POP_ETH
followed by USERSPACE produce empty upcalls.

The first patch in this series, "net: skbuff: don't leave stale bytes in
skb_copy_and_csum_bits()", has a Fixes: tag.

> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index e29eda2eaf3f6..8c6a45a20eb0f 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -3906,7 +3906,8 @@ 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 (WARN_ON_ONCE(!from->head_frag && !hlen))
> +		return -EFAULT;

[Severity: Medium]
Can this WARN fire on a valid empty copy?

When len == 0 there is nothing to copy, so hlen == 0 is not a caller bug
in that case. The kernel-doc names skb_zerocopy_headlen() as the source
of @hlen, and it returns 0 for an empty skb without head_frag:

net/core/skbuff.c:skb_zerocopy_headlen() {
    ...
	if (!from->head_frag || ...) {
		hlen = skb_headlen(from);
		if (!hlen)
			hlen = from->len;
	}
    ...
}

The openvswitch path above hits this case. queue_userspace_packet()
computes skb_len = min(skb->len, cutlen), which is 0. hlen is also 0 on
both the OVS_DP_F_UNALIGNED path and the aligned path. The skb was
allocated with GFP_KERNEL, so head_frag is 0.

If the check came after the small payload fast path, this call would
simply return 0:

	if (len <= skb_tailroom(to))
		return skb_copy_bits(from, 0, skb_put(to, len), len);

Because the check comes first, userspace can trigger a WARN splat (or a
panic with panic_on_warn), and the empty upcall fails with -EFAULT.

Would adding len && to the condition, or moving the check below the fast
path, avoid this?

[Severity: Low]
Should the skb_zerocopy() kernel-doc be updated as well? Its return value
section lists only two causes of -EFAULT:

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

The new return happens before skb_copy_bits() is called and has nothing
to do with unreadable frags. The @hlen description ("size of linear
headroom in destination buffer") also doesn't say that hlen must be
non-zero when @from has no head_frag.

The commit message says skb_zerocopy() "already documents -EFAULT for
bad skb geometry". The doc only uses that wording for skb_copy_bits()
failures.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261006-b4-skbuff-bug-on-v1-0-1b4434c5357c%40toxicpanda.com