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

[PATCH net] 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 usually
won't match the data any more, so the receiver will usually drop the
packet, but either way it no longer carries 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")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
This was patch 1 of the skbuff BUG_ON() series; Willem asked for it to go
to net on its own:
https://lore.kernel.org/r/willemdebruijn.kernel.235bf1cecf85f@gmail.com

Tested on net with a module that marks a nonlinear skb unreadable: the
part of the buffer past the linear data is zeroed, and a negative @len
leaves the buffer alone.

Thanks,
Josef
---
 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 4aea06d5167d..41beaf625421 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;

---
base-commit: 23609bce9e1de525d1d0e73fc68c6e7971d0b49e
change-id: 20261007-b4-skb-copy-csum-stale-bytes-4bf7b713258c

Re: [PATCH net] net: skbuff: don't leave stale bytes in skb_copy_and_csum_bits()

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH net] net: skbuff: don't leave stale bytes in skb_copy_and_csum_bits()

[Josef Bacik]

On Wed, Oct 07, 2026 at 05:34:41PM +0000, netdev-bot+sinfo@kernel.org wrote:
> This is an automated message. This series looks like a fix, but its
> commit messages seem to be missing some information:
>
>  - How the issue was discovered, e.g. hit in production, hit during
>    development, syzbot report, manual code inspection, LLM or static
>    analysis tool scan.

Found during development.  I was converting the BUG_ON()s in skbuff.c
[1], and the conversion of the leftover-length BUG_ON() in
skb_copy_and_csum_bits() zeroes the part of the caller's buffer it
couldn't fill.  An LLM review of that change pointed out that the
existing unreadable-frags early return in the same function leaves the
buffer unfilled.  I confirmed it by reading the code and with a test
module that marks a nonlinear skb unreadable.  It hasn't been hit in
production that I know of.

[1] https://lore.kernel.org/all/20261007-b4-skbuff-bug-on-v2-0-b9a5f732895b@toxicpanda.com/

Thanks,
Josef

Re: [PATCH net] net: skbuff: don't leave stale bytes in skb_copy_and_csum_bits()

[Mina Almasry]

On Wed, Oct 7, 2026 at 10:29 AM Josef Bacik <josef@toxicpanda.com> 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 usually
> won't match the data any more, so the receiver will usually drop the
> packet, but either way it no longer carries 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")
> Cc: stable@vger.kernel.org
> Assisted-by: LLM
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Reviewed-by: Mina Almasry <almasrymina@google.com>

We probably need some better csum handling with unreadable skbs
eventually. I took a shortcut in the initial implementation and
returned an invalid csum because there was no way to return error from
the csum functions. With LLMs now this is probably easier to fix.

-- 
Thanks,
Mina

Re: [PATCH net] 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] With this patch, skb_copy_and_csum_bits()
  (net/core/skbuff.c:3636-3641) zero-fills the destination and returns 0…
- [Low] The commit message misstates where the bug actually matters.

--

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

When skb_copy_and_csum_bits() reaches unreadable frags, this patch zeroes
the rest of the destination buffer before it returns 0. The goal is to
stop stale memory from being sent on the wire. A negative len is left
alone so it doesn't turn into a huge memset().

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

[Severity: Low]
Can the skb_copy_and_csum_dev() example actually see an unreadable skb?
Its callers are 8139too, via-rhine, ep93xx, ec_bhf, ksz884x and sc92031.
All of them leave dev->netmem_tx at NETMEM_TX_NONE, and
validate_xmit_unreadable_skb() frees unreadable skbs for those devices
before ndo_start_xmit runs:

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

The same seems to hold for sunvnet's vnet_skb_shape().

Also, not every caller sends the buffer. xdr_skb_read_bits() in
net/sunrpc/socklib.c copies into an RPC receive buffer.

Some reachable paths aren't mentioned: the !sg && !csum branch of
skb_segment(), and the fraggap copies in __ip_append_data() and
__ip6_append_data(). The ICMP quoting path through icmp_glue_bits() and
icmpv6_getfrag() is mentioned, but no concrete trigger is given.

The change is in the shared helper, so the code covers all of these. This
is tagged for stable, though. Could the commit message list the paths that
can actually be reached?

> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 4aea06d5167d1..41beaf625421a 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]
If offset is at or past skb_headlen(skb), the header copy is skipped and
csum stays 0. After this change the whole destination is zeros, and 0 is
the correct partial checksum for an all-zero buffer.

Can this lead to silent payload corruption when software GSO runs on a
devmem TX skb? skb_segment() uses the return value directly in its
!sg && !csum branch:

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);
				...
			} else {
				if (skb_copy_bits(head_skb, offset, skb_put(nskb, len), len))
					goto err;
			}
    ...
}

tcp_gso_segment() then sets th->check via gso_make_checksum(skb,
~th->check). That folds the TCP header and pseudo-header with
partial = 0, so the zero-filled payload gets a valid checksum. Devmem
payload lives entirely in net_iov frags, which means every segment's
offset is past headlen.

The path looks reachable, with CAP_NET_ADMIN needed to turn off TX
offloads:

- sk_setup_caps() ORs NETIF_F_SG | NETIF_F_HW_CSUM into sk_route_caps.
  As a result, tcp_sendmsg_locked() still accepts a dmabuf_id after
  something like "ethtool -K <dev> tx off".

- validate_xmit_unreadable_skb() passes the skb for a NETMEM_TX_DMA
  device with a matching binding, or for any NETMEM_TX_NO_DMA device
  such as netkit.

- skb_gso_segment() -> tcp_gso_segment() -> skb_segment() then runs with
  sg and csum both false.

Before this patch the payload was stale bytes with a payload checksum of
0, so the receiver almost always dropped the segment. With the patch, the
peer would accept the zeros as stream data.

The commit message says:

  The checksum usually won't match the data any more, so the receiver
  will usually drop the packet

On this path the checksum always matches. The csum branch next to it in
skb_segment() fails on the same input, because skb_copy_bits() returns
-EFAULT and the code does goto err.

Should the !csum branch in skb_segment() fail the same way? Another option
is for skb_copy_and_csum_bits() to return a checksum that can't match the
zero-filled data.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261007-b4-skb-copy-csum-stale-bytes-v1-1-adbbde033fb3%40toxicpanda.com

Re: [PATCH net] net: skbuff: don't leave stale bytes in skb_copy_and_csum_bits()

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Wed, 07 Oct 2026 17:29:34 +0000 you 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.
> 
> [...]

Here is the summary with links:
  - [net] net: skbuff: don't leave stale bytes in skb_copy_and_csum_bits()
    https://git.kernel.org/netdev/net/c/ab9414ed70bd

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html