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