Re: [PATCH net-next v2 01/15] net: skbuff: make skb_copy_and_csum_bits() report unreadable frags

[Fernando Fernandez Mancera <fmancera@suse.de>]

On 10/21/26 3:46 PM, Josef Bacik wrote:
> skb_copy_and_csum_bits() has no way to fail.  When it reaches
> unreadable frags it zero-fills the rest of the buffer and returns 0,
> and since commit ab9414ed70bd ("net: skbuff: don't leave stale bytes in
> skb_copy_and_csum_bits()") that 0 is the correct checksum for what's in
> the buffer.  Its callers can't tell the copy didn't happen.
> 
> That matters in the !sg && !csum branch of skb_segment().  A devmem TCP
> skb keeps its whole payload in unreadable frags, so every segment ends
> up with a payload of zeroes, tcp_gso_segment() writes a valid TCP
> checksum over it, and the peer accepts the zeroes as stream data.  This
> is reachable when both SG and TX checksum offload are turned off on a
> device used for devmem TX: sk_setup_caps() still gives TCP SG and
> HW_CSUM, so the dmabuf send is accepted, validate_xmit_unreadable_skb()
> lets the skb through for a NETMEM_TX_NO_DMA device or a matching
> binding, and software GSO segments it with neither feature.  Before
> that commit the segments carried uninitialized memory with a checksum
> that almost never matched.
> 
> This was reproduced with a test module rather than on devmem hardware:
> skb_segment() on a TCPv4 GSO skb with 3000 bytes of payload in a page
> frag marked unreadable, with features == 0, returns 3 segments whose
> payloads are all zeroes, each with a checksum of 0, which is correct for
> zeroes.  There's no warning or error; the data is silently replaced.
> With this patch skb_segment() returns -EFAULT.
> 
> Make skb_copy_and_csum_bits() return 0 or -EFAULT and hand the checksum
> back through a pointer, and make it __must_check so a new caller can't
> ignore the failure.  Running out of skb before @len returns -EFAULT
> too; commit 417261779eab ("net: skbuff: don't BUG() on leftover length
> in skb_copy_and_csum_bits()") made that a zero-fill and a checksum of 0.
> Each caller takes the error path it already has for a failed copy:
> 
>   - skb_segment() fails the segmentation, as the csum branch already
>     does when skb_copy_bits() fails.
>   - icmp_glue_bits() and icmpv6_getfrag() return the error, so the ICMP
>     error isn't sent.
>   - __ip_append_data() and __ip6_append_data() drop the new skb and
>     return -EFAULT, as they do when getfrag fails.
>   - vnet_skb_shape() fails the same way it does when its skb_copy_bits()
>     call fails.
>   - xdr_skb_read_bits() returns 0 without consuming anything, as it does
>     when skb_copy_bits() fails, so desc.count stays non-zero and
>     csum_partial_copy_to_xdr() fails.  csum_partial_copy_to_xdr() used
>     to checksum the bytes it hadn't copied before checking desc.count.
>     desc.offset and desc.count always add up to skb->len, so that only
>     ran when it was about to fail anyway, and on an unreadable skb
>     skb_checksum() would WARN.  Drop it.
>   - skb_copy_and_csum_dev() can't return an error, so it leaves the
>     checksum alone and the frame goes out with a bad one.  Its callers
>     are drivers that never see unreadable skbs anyway.
> 
> The zero-fill stays, so a caller that does send the buffer still
> doesn't leak anything.
> 
> This was found by the Sashiko AI review of that commit, and
> Mina suggested reporting the error from the helper rather than checking
> for unreadable frags in skb_segment().
> 
> Link: https://lore.kernel.org/all/179148235783.434549.14322374227477832817@kernel.org/
> Suggested-by: Mina Almasry <almasrymina@google.com>
> Reviewed-by: Mina Almasry <almasrymina@google.com>
> Assisted-by: LLM
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Hm, I do wonder isn't this missing the Fixes tag for ab9414ed70bd and 
417261779eab? I don't have a strong opinion but since ab9414ed70bd will 
be backported to stable (quite likely..) it makes sense to me.

> ---
>   drivers/net/ethernet/sun/sunvnet_common.c | 10 +++--
>   include/linux/skbuff.h                    |  4 +-
>   net/core/skbuff.c                         | 63 +++++++++++++++++++++----------
>   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 ++---
>   8 files changed, 83 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/net/ethernet/sun/sunvnet_common.c b/drivers/net/ethernet/sun/sunvnet_common.c
> index 5cca95869052..4f229fcecc32 100644
> --- a/drivers/net/ethernet/sun/sunvnet_common.c
> +++ b/drivers/net/ethernet/sun/sunvnet_common.c
> @@ -1167,9 +1167,13 @@ static inline struct sk_buff *vnet_skb_shape(struct sk_buff *skb, int ncookies)
>   
>   			/* copy the rest, with csum calculation */
>   			*(__sum16 *)(skb->data + offset) = 0;
> -			csum = skb_copy_and_csum_bits(skb, start,
> -						      nskb->data + start,
> -						      skb->len - start);
> +			if (skb_copy_and_csum_bits(skb, start,
> +						   nskb->data + start,
> +						   skb->len - start, &csum)) {
> +				dev_kfree_skb(nskb);
> +				dev_kfree_skb(skb);
> +				return NULL;
> +			}
>   
>   			/* add in the header checksums */
>   			if (skb->protocol == htons(ETH_P_IP)) {
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index af1cbc2799b7..09db32f70b53 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -4277,8 +4277,8 @@ void skb_free_datagram(struct sock *sk, struct sk_buff *skb);
>   int skb_kill_datagram(struct sock *sk, struct sk_buff *skb, unsigned int flags);
>   int skb_copy_bits(const struct sk_buff *skb, int offset, void *to, int len);
>   int skb_store_bits(struct sk_buff *skb, int offset, const void *from, int len);
> -__wsum skb_copy_and_csum_bits(const struct sk_buff *skb, int offset, u8 *to,
> -			      int len);
> +int __must_check skb_copy_and_csum_bits(const struct sk_buff *skb, int offset,
> +					u8 *to, int len, __wsum *csum);

For consistency skb_copy_and_csum_bits() should use csump instead of 
csum in the prototype to match the implementation and kernel-doc 
parameter name.

>   int skb_splice_bits(struct sk_buff *skb, struct sock *sk, unsigned int offset,
>   		    struct pipe_inode_info *pipe, unsigned int len,
>   		    unsigned int flags);
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 93c2f440b9ec..a2b52a5a61a9 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -3630,14 +3630,27 @@ EXPORT_SYMBOL(skb_checksum);
>   
>   /* Both of above in one bottle. */
>   

I guess we can now remove this comment. I mean the kernel-doc being 
added here makes it a bit redundant in my opinion.

> -__wsum skb_copy_and_csum_bits(const struct sk_buff *skb, int offset,
> -				    u8 *to, int len)
> +/**
> + *	skb_copy_and_csum_bits - copy data out of an skb and checksum it
> + *	@skb: source buffer
> + *	@offset: offset in @skb to start copying from
> + *	@to: destination buffer
> + *	@len: number of bytes to copy
> + *	@csump: where to store the checksum of the copied bytes
> + *
> + *	Return: 0 on success, or -EFAULT if @skb has unreadable frags or runs
> + *	out before @len does.  On failure *@csump is not set and the part of
> + *	@to that couldn't be filled is zeroed.
> + */
> +int skb_copy_and_csum_bits(const struct sk_buff *skb, int offset,
> +			   u8 *to, int len, __wsum *csump)
>   {
>   	int start = skb_headlen(skb);
>   	int i, copy = start - offset;
>   	struct sk_buff *frag_iter;
>   	int pos = 0;
>   	__wsum csum = 0;
> +	int err;
>   
>   	/* Copy header. */
>   	if (copy > 0) {
> @@ -3646,7 +3659,7 @@ __wsum skb_copy_and_csum_bits(const struct sk_buff *skb, int offset,
>   		csum = csum_partial_copy_nocheck(skb->data + offset, to,
>   						 copy);
>   		if ((len -= copy) == 0)
> -			return csum;
> +			goto out;
>   		offset += copy;
>   		to     += copy;
>   		pos	= copy;
> @@ -3656,7 +3669,7 @@ __wsum skb_copy_and_csum_bits(const struct sk_buff *skb, int offset,
>   		/* Don't hand the caller a buffer with stale bytes in it. */
>   		if (len > 0)
>   			memset(to, 0, len);
> -		return 0;
> +		return -EFAULT;
>   	}
>   
>   	for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) {
> @@ -3688,7 +3701,7 @@ __wsum skb_copy_and_csum_bits(const struct sk_buff *skb, int offset,
>   			}
>   
>   			if (!(len -= copy))
> -				return csum;
> +				goto out;
>   			offset += copy;
>   			to     += copy;
>   		}
> @@ -3705,12 +3718,16 @@ __wsum skb_copy_and_csum_bits(const struct sk_buff *skb, int offset,
>   		if ((copy = end - offset) > 0) {
>   			if (copy > len)
>   				copy = len;
> -			csum2 = skb_copy_and_csum_bits(frag_iter,
> -						       offset - start,
> -						       to, copy);
> +			err = skb_copy_and_csum_bits(frag_iter, offset - start,
> +						     to, copy, &csum2);
> +			if (err) {
> +				/* The child zeroed its part; zero the rest. */
> +				memset(to + copy, 0, len - copy);
> +				return err;
> +			}
>   			csum = csum_block_add(csum, csum2, pos);
>   			if ((len -= copy) == 0)
> -				return csum;
> +				goto out;
>   			offset += copy;
>   			to     += copy;
>   			pos    += copy;
> @@ -3722,9 +3739,11 @@ __wsum skb_copy_and_csum_bits(const struct sk_buff *skb, int offset,
>   		/* Don't hand the caller a buffer with stale bytes in it. */
>   		if (len > 0)
>   			memset(to, 0, len);
> -		return 0;
> +		return -EFAULT;
>   	}
> -	return csum;
> +out:
> +	*csump = csum;
> +	return 0;
>   }
>   EXPORT_SYMBOL(skb_copy_and_csum_bits);
>   
> @@ -4006,9 +4025,13 @@ void skb_copy_and_csum_dev(const struct sk_buff *skb, u8 *to)
>   	skb_copy_from_linear_data(skb, to, csstart);
>   
>   	csum = 0;
> -	if (csstart != skb->len)
> -		csum = skb_copy_and_csum_bits(skb, csstart, to + csstart,
> -					      skb->len - csstart);
> +	/* On failure the rest of @to is zeroed; leave the checksum alone so
> +	 * the frame goes out with a bad one.
> +	 */
> +	if (csstart != skb->len &&
> +	    skb_copy_and_csum_bits(skb, csstart, to + csstart,
> +				   skb->len - csstart, &csum))
> +		return;
>   
>   	if (skb->ip_summed == CHECKSUM_PARTIAL) {
>   		long csstuff = csstart + skb->csum_offset;
> @@ -5057,11 +5080,13 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
>   			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);
> +				if (skb_copy_and_csum_bits(head_skb, offset,
> +							   skb_put(nskb, len),
> +							   len,
> +							   &SKB_GSO_CB(nskb)->csum)) {
> +					err = -EFAULT;
> +					goto err;
> +				}
>   				SKB_GSO_CB(nskb)->csum_start =
>   					skb_headroom(nskb) + doffset;
>   			} else {
> diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
> index 90c0e22c29be..35008785e3d2 100644
> --- a/net/ipv4/icmp.c
> +++ b/net/ipv4/icmp.c
> @@ -364,12 +364,15 @@ static int icmp_glue_bits(void *from, char *to, int offset, int len, int odd,
>   	DEFINE_RAW_FLEX(struct icmp_bxm, icmp_param, replyopts.opt.__data,
>   			IP_OPTIONS_DATA_FIXED_SIZE);
>   	__wsum csum;
> +	int err;
>   
>   	icmp_param = from;
>   
> -	csum = skb_copy_and_csum_bits(icmp_param->skb,
> -				      icmp_param->offset + offset,
> -				      to, len);
> +	err = skb_copy_and_csum_bits(icmp_param->skb,
> +				     icmp_param->offset + offset,
> +				     to, len, &csum);
> +	if (err)
> +		return err;
>   
>   	skb->csum = csum_block_add(skb->csum, csum, odd);
>   	if (icmp_param->data.icmph.type <= NR_ICMP_TYPES &&
> diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
> index 3396f177a18d..f7da4538d05b 100644
> --- a/net/ipv4/ip_output.c
> +++ b/net/ipv4/ip_output.c
> @@ -1162,9 +1162,14 @@ static int __ip_append_data(struct sock *sk,
>   			data += fragheaderlen + exthdrlen;
>   
>   			if (fraggap) {
> -				skb->csum = skb_copy_and_csum_bits(
> -					skb_prev, maxfraglen,
> -					data + transhdrlen, fraggap);
> +				if (skb_copy_and_csum_bits(skb_prev, maxfraglen,
> +							   data + transhdrlen,
> +							   fraggap,
> +							   &skb->csum)) {
> +					err = -EFAULT;
> +					kfree_skb(skb);
> +					goto error;
> +				}
>   				skb_prev->csum = csum_sub(skb_prev->csum,
>   							  skb->csum);
>   				data += fraggap;
> diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c
> index a95b0351824f..31ca3da80b1f 100644
> --- a/net/ipv6/icmp.c
> +++ b/net/ipv6/icmp.c
> @@ -322,9 +322,12 @@ static int icmpv6_getfrag(void *from, char *to, int offset, int len, int odd, st
>   	struct icmpv6_msg *msg = (struct icmpv6_msg *) from;
>   	struct sk_buff *org_skb = msg->skb;
>   	__wsum csum;
> +	int err;
>   
> -	csum = skb_copy_and_csum_bits(org_skb, msg->offset + offset,
> -				      to, len);
> +	err = skb_copy_and_csum_bits(org_skb, msg->offset + offset,
> +				     to, len, &csum);
> +	if (err)
> +		return err;
>   	skb->csum = csum_block_add(skb->csum, csum, odd);
>   	if (!(msg->type & ICMPV6_INFOMSG_MASK))
>   		nf_ct_attach(skb, org_skb);
> diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
> index 25fe0ba98b1a..d259e7dca8ba 100644
> --- a/net/ipv6/ip6_output.c
> +++ b/net/ipv6/ip6_output.c
> @@ -1724,9 +1724,14 @@ static int __ip6_append_data(struct sock *sk,
>   			skb->transport_header = (skb->network_header +
>   						 fragheaderlen);
>   			if (fraggap) {
> -				skb->csum = skb_copy_and_csum_bits(
> -					skb_prev, maxfraglen,
> -					data + transhdrlen, fraggap);
> +				if (skb_copy_and_csum_bits(skb_prev, maxfraglen,
> +							   data + transhdrlen,
> +							   fraggap,
> +							   &skb->csum)) {
> +					err = -EFAULT;
> +					kfree_skb(skb);
> +					goto error;
> +				}
>   				skb_prev->csum = csum_sub(skb_prev->csum,
>   							  skb->csum);
>   				data += fraggap;
> diff --git a/net/sunrpc/socklib.c b/net/sunrpc/socklib.c
> index d8d8842c7de5..5d780ad4ae3a 100644
> --- a/net/sunrpc/socklib.c
> +++ b/net/sunrpc/socklib.c
> @@ -49,7 +49,9 @@ xdr_skb_read_bits(struct xdr_skb_reader *desc, void *to, size_t len)
>   	if (desc->need_checksum) {
>   		__wsum csum;
>   
> -		csum = skb_copy_and_csum_bits(desc->skb, desc->offset, to, len);
> +		if (unlikely(skb_copy_and_csum_bits(desc->skb, desc->offset,
> +						    to, len, &csum)))
> +			return 0;
>   		desc->csum = csum_block_add(desc->csum, csum, desc->offset);
>   	} else {
>   		if (unlikely(skb_copy_bits(desc->skb, desc->offset, to, len)))
> @@ -142,11 +144,6 @@ int csum_partial_copy_to_xdr(struct xdr_buf *xdr, struct sk_buff *skb)
>   	desc.csum = csum_partial(skb->data, desc.offset, skb->csum);
>   	if (xdr_partial_copy_from_skb(xdr, &desc) < 0)
>   		return -1;
> -	if (desc.offset != skb->len) {
> -		__wsum csum2;
> -		csum2 = skb_checksum(skb, desc.offset, skb->len - desc.offset, 0);
> -		desc.csum = csum_block_add(desc.csum, csum2, desc.offset);
> -	}
>   	if (desc.count)
>   		return -1;
>   	if (csum_fold(desc.csum))
>