v1: https://lore.kernel.org/all/20261019-b4-skbuff-bug-on-followups-v1-0-41dbef50b9f7@toxicpanda.com/ (local)
v2: https://lore.kernel.org/all/20261021-b4-skbuff-bug-on-followups-v2-0-2aa780a03524@toxicpanda.com/ (local)
v2->v3:
- 1, 2, 6: move the frag_list recursion into a noinline function, so
that taking the address of a local there doesn't give the helpers a
stack protector (Paolo). On x86_64 defconfig plus CONFIG_IP_SCTP
none of skb_copy_and_csum_bits(), skb_checksum_full(),
skb_crc32c_full() or the two wrappers has one now, and
__ip_append_data(), __ip6_append_data() and skb_segment() are
unchanged. The callers that pass a local do gain one. Besides the
seven I listed on v2, that's __skb_gro_checksum_complete() and
skb_checksum_help(), which are converted in this version, and
skb_crc32c_csum_help().
- 1: put the Fixes: tags back (Fernando, Sashiko). csump in the
prototype, and drop the "Both of above in one bottle" comment
(Fernando). skb_copy_and_csum_dev() zeroes the whole frame when the
copy fails, since leaving the checksum alone doesn't guarantee a bad
one (Sashiko).
- 2, 6: the kernel-doc describes the unreadable check as it is, the
wrappers don't blame unreadable frags for -EINVAL, and the commit
messages say what changes for a failing frag_list member (Fernando,
Sashiko).
- 2: convert skb_checksum_help() too, as 6 does for its crc32c twin, and
add Fixes: 20ddd0fd2fcd.
- 3: convert __skb_gro_checksum_complete() too (Fernando, Sashiko).
Take the skb off CHECKSUM_COMPLETE on failure. Otherwise conntrack,
which doesn't drop on a bad checksum, leaves the pseudo-header sum in
skb->csum and TCP's validation accepts the packet after all
(Sashiko). Update the comments, and add Fixes: 65249feb6b3d.
- 4: the commit message was wrong that devmem TX reaches this checksum.
Its skbs carry SKBFL_SHARED_FRAG, so the segment fails to linearize
first. The patch is hardening, and the commit message says so now.
- 5: when the tail can't be checksummed but what's left can be read,
set CHECKSUM_NONE instead of failing the trim, which dropped packets
that used to be accepted (Sashiko). Retitled, and add Fixes: tags.
- 7, 8: add the Fixes: tags for the original commits, and correct the
history in 8's commit message (Sashiko). 8 also says what it takes
to reach the memcpy().
- 7: reject a negative @len or @hlen on entry. The reorder left nothing
ahead of the skb_put(), which panics on one.
- 8: the truesize check refuses the coalesce instead of only warning,
and runs before the head is stolen. With len unsigned and nothing
else, an skb with a len above INT_MAX went from refused to coalesced.
- 9: check data_len > len directly and zero the frame. The old check
missed CHECKSUM_PARTIAL (Sashiko). Retitled.
- 10: drop both len > 0 tests, and correct what the commit message says
they covered (Sashiko).
- 9, 10: drop the Fixes: tags. Both problems are older than the commits
the tags named, and neither has a known trigger.
- 11: Willem, this one changed again after you OK'd the walk up front.
A nested frag_list is only one way for the walk to run out of members.
A len that counts bytes the skb doesn't hold gets to the same NULL
pointer dereference, and the walk up front doesn't catch that. So the
patch now checks for a NULL list_skb in the three places where the
walk moves on to it, and that's all it does: no pass up front, which
also takes care of the cost you pointed out for BIG TCP, and none of
v2's skb_has_frag_list() tests. You asked on v2 whether the first two
checks were needed. For a nested frag_list they weren't, for this
they are. 12 has a case for each, and taking any one of the three
checks out brings the oops back. The path without SG is left alone
again. The commit message says that IP reassembly does nest
frag_lists, just not on GSO skbs (Sashiko). Retitled.
- 12: add four cases for a len that counts bytes the skb doesn't hold.
Retitled.
- 13: correct what the commit message says about when bounce_skb() runs
and when skb_copy_bits() fails.
- 14: update the comment above skb_nsg() (Sashiko).
- 15: one more stale comment, in net_test.c.
- Picked up Fernando's Reviewed-by on 4. Dropped Mina's on 1 and
Fernando's on 2, since the frag_list loop moved in both, and Willem's
on 12, since it grew.
- Shortened the commit messages.
- Rebased onto current net-next.
v1->v2:
- skb_segment(): check each frag_list member as the walk takes it,
instead of an extra pass over the frag_list up front (Willem).
- KUnit test: braces around the suppressed skb_segment() call (Willem),
and the comment update for the non-uniform case moved into the same
patch (Mina).
- Picked up Willem's Reviewed-by and Mina's Acked-bys.
--- Original email (v1) ---
Hello,
Jakub applied v2 of my skbuff BUG_ON() series[1] before I got v3 out, so
this is what v3 would have changed, as fixes on top of what landed, plus
the skb_copy_and_csum_bits() patch I'd been sending to net on its own.
Patch 1 is that patch[2], moved to net-next as Mina suggested, with his
Reviewed-by. It makes skb_copy_and_csum_bits() report failure, and the
rest of the series builds on it. Please drop the net version.
Patches 2 through 6 deal with skb_checksum() and skb_crc32c(). My v2
had them return 0 when they run out of skb, and that turned out not to
be safe: 0 is a valid checksum, and it can make a packet that was never
fully checksummed pass verification. So they get versions that report
failure, skb_checksum_full() and skb_crc32c_full(), and the core users
switch to them: checksum validation now rejects a packet it can't
checksum, skb_segment() and pskb_trim_rcsum() fail instead of using a
bad sum, and skb_crc32c_csum_help() drops the packet. skb_checksum()
and skb_crc32c() stay as wrappers that behave as before, and I'll
convert the rest of their callers per subsystem in later series.
Patches 7 through 10 fix problems Sashiko and an LLM review found in
what was applied. skb_zerocopy() refuses a zero-length copy that Open
vSwitch can trigger from a user namespace, skb_try_coalesce() and
skb_copy_and_csum_bits() can be handed a negative length, and
skb_headlen() can underflow in skb_copy_and_csum_dev().
Patches 11 and 12 make skb_segment() refuse a nested frag_list, which
could still oops on a NULL list_skb, and add KUnit cases for the layouts
it refuses. Patches 13 and 14 get rid of the same BUG_ON()s in
xen-netfront's bounce_skb() and tls's __skb_nsg(), and patch 15 updates
comments that still describe the old BUG_ON()s.
I tested this with the net_core KUnit tests, a test module that drives
each of these paths under KASAN and fault injection, an Open vSwitch
reproducer for patch 7, and a stress run over veth with netem. Thanks,
Josef
[1] https://lore.kernel.org/all/20261007-b4-skbuff-bug-on-v2-0-b9a5f732895b@toxicpanda.com/ (local)
[2] https://lore.kernel.org/all/20261013-b4-skb-segment-nosg-unreadable-v2-1-6c28d8ffa51e@toxicpanda.com/ (local)
---
Changes in v3:
- Link to v2: https://patch.msgid.link/20261021-b4-skbuff-bug-on-followups-v2-0-2aa780a03524@toxicpanda.com (local)
Changes in v2:
- Link to v1: https://patch.msgid.link/20261019-b4-skbuff-bug-on-followups-v1-0-41dbef50b9f7@toxicpanda.com (local)
---
Josef Bacik (15):
net: skbuff: make skb_copy_and_csum_bits() report unreadable frags
net: skbuff: add skb_checksum_full() that reports failure
net: skbuff: fail checksum validation when the skb can't be checksummed
net: skbuff: fail segmentation when a segment can't be checksummed
net: skbuff: drop hw csum if trim can't checksum tail
net: skbuff: add skb_crc32c_full() that reports failure
net: skbuff: let skb_zerocopy() copy a short skb without head_frag
net: skbuff: make skb_try_coalesce()'s len unsigned
net: skbuff: reject data_len > len in copy_and_csum_dev
net: skbuff: reject a negative length in skb_copy_and_csum_bits()
net: skbuff: don't run off frag_list in skb_segment()
net: test the skb layouts skb_segment() refuses
xen-netfront: don't BUG() when skb_copy_bits() fails in bounce_skb()
net/tls: don't BUG() on leftover length in __skb_nsg()
net: update comments that still describe the skb BUG_ON()s
drivers/net/ethernet/sun/sunvnet_common.c | 10 +-
drivers/net/xen-netfront.c | 7 +-
include/linux/skbuff.h | 12 +-
net/core/dev.c | 14 +-
net/core/filter.c | 6 +-
net/core/gro.c | 11 +-
net/core/net_test.c | 100 ++++++-
net/core/skbuff.c | 447 +++++++++++++++++++++++-------
net/ipv4/icmp.c | 9 +-
net/ipv4/ip_output.c | 11 +-
net/ipv6/icmp.c | 7 +-
net/ipv6/ip6_output.c | 11 +-
net/sunrpc/socklib.c | 9 +-
net/tls/tls_sw.c | 8 +-
14 files changed, 515 insertions(+), 147 deletions(-)
---
base-commit: 4dd16e31135ea8437a720222df34e8745f385d6d
change-id: 20261016-b4-skbuff-bug-on-followups-ad11adf285a4