[PATCH] net: skbuff: don't BUG() when skb_copy_bits() fails in __pskb_pull_tail

[PATCH] net: skbuff: don't BUG() when skb_copy_bits() fails in __pskb_pull_tail

[shao.mingyin]

From: Shao Mingyin <shao.mingyin@zte.com.cn>

__pskb_pull_tail() BUG()s if skb_copy_bits() returns an error.
skb_copy_bits() fails when the lengths do not add up, or when a
frag_list child carries unreadable frags (devmem net_iov, commit
65249feb6b3d ("net: add support for skbs with unreadable frags")).  The
top-level check above does not cover frag_list children, and
skb_gro_receive() links them without propagating skb->unreadable, so a
mixed skb trips the BUG().

Nothing has been committed at that point, so return NULL the way the
rest of the function already handles failures, with a
DEBUG_NET_WARN_ON_ONCE() for debug kernels.  skb_condense(), the one
net/core caller that ignores the return, now checks it and keeps the
original truesize when the pull fails, as the frags are still there.

The other direct callers that ignore the return do not build a
frag_list child: xen-netfront drops the skb when the failed pull leaves
nr_frags at MAX_SKB_FRAGS, and niu, mlx5 and ftmac100 pull at most what
their own skb holds.  A length mismatch there is a driver bug, which
the debug warning reports.

This is the separate change that commit 279877d86c82 ("net: skbuff:
don't BUG() when skb_copy_bits() fails in copy helpers") left for the
callers; the audit is above.  No Fixes: tag and no stable marking: the
mixed skb is an invariant violation, not a regression, and this is a
net-next hardening in the BUG_ON() removal series.

Signed-off-by: Shao Mingyin <shao.mingyin@zte.com.cn>
---
 net/core/skbuff.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

Tested on 7.3.0-next-20261030 (QEMU/TCG guest).  QEMU has no devmem
(net_iov) backend, so a TEST-ONLY hook (applied to both kernels, not
part of this patch) fabricates an skb whose frag_list child carries an
unreadable frag, then calls the real __pskb_pull_tail() on it at
late_initcall time: skb_copy_bits() recurses into the child, sees the
unreadable frag and fails.  The hook then calls skb_condense() on the
same skb, so the failed pull must not shrink truesize.

  unpatched                             patched
  kernel BUG at                         __pskb_pull_tail() returns NULL,
  net/core/skbuff.c:2903                skb_condense() keeps truesize 960,
                                        boot continues, no BUG

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 48f676411724..8a200252ef93 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -2900,8 +2900,11 @@ void *__pskb_pull_tail(struct sk_buff *skb, int delta)
 			return NULL;
 	}
 
-	BUG_ON(skb_copy_bits(skb, skb_headlen(skb),
-			     skb_tail_pointer(skb), delta));
+	if (unlikely(skb_copy_bits(skb, skb_headlen(skb),
+				   skb_tail_pointer(skb), delta))) {
+		DEBUG_NET_WARN_ON_ONCE(1);
+		return NULL;
+	}
 
 	/* Optimization: no fragments, no reasons to preestimate
 	 * size of pulled pages. Superb.
@@ -7218,7 +7221,8 @@ void skb_condense(struct sk_buff *skb)
 			return;
 
 		/* Nice, we can free page frag(s) right now */
-		__pskb_pull_tail(skb, skb->data_len);
+		if (!__pskb_pull_tail(skb, skb->data_len))
+			return;	/* frags are still there, keep truesize */
 	}
 	/* At this point, skb->truesize might be over estimated,
 	 * because skb had a fragment, and fragments do not tell
--

Re: [PATCH] net: skbuff: don't BUG() when skb_copy_bits() fails in __pskb_pull_tail

[Willem de Bruijn]

shao.mingyin@ wrote:
> From: Shao Mingyin <shao.mingyin@zte.com.cn>
> 
> __pskb_pull_tail() BUG()s if skb_copy_bits() returns an error.
> skb_copy_bits() fails when the lengths do not add up, or when a
> frag_list child carries unreadable frags (devmem net_iov, commit
> 65249feb6b3d ("net: add support for skbs with unreadable frags")).  The
> top-level check above does not cover frag_list children, and
> skb_gro_receive() links them without propagating skb->unreadable, so a
> mixed skb trips the BUG().
> 
> Nothing has been committed at that point, so return NULL the way the
> rest of the function already handles failures, with a
> DEBUG_NET_WARN_ON_ONCE() for debug kernels.  skb_condense(), the one
> net/core caller that ignores the return, now checks it and keeps the
> original truesize when the pull fails, as the frags are still there.
> 
> The other direct callers that ignore the return do not build a
> frag_list child: xen-netfront drops the skb when the failed pull leaves
> nr_frags at MAX_SKB_FRAGS, and niu, mlx5 and ftmac100 pull at most what
> their own skb holds.  A length mismatch there is a driver bug, which
> the debug warning reports.
> 
> This is the separate change that commit 279877d86c82 ("net: skbuff:
> don't BUG() when skb_copy_bits() fails in copy helpers") left for the
> callers; the audit is above.  No Fixes: tag and no stable marking: the
> mixed skb is an invariant violation, not a regression, and this is a
> net-next hardening in the BUG_ON() removal series.

In the series, Josef reported

  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.

> Signed-off-by: Shao Mingyin <shao.mingyin@zte.com.cn>
> ---
>  net/core/skbuff.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> Tested on 7.3.0-next-20261030 (QEMU/TCG guest).  QEMU has no devmem
> (net_iov) backend, so a TEST-ONLY hook (applied to both kernels, not
> part of this patch) fabricates an skb whose frag_list child carries an
> unreadable frag, then calls the real __pskb_pull_tail() on it at
> late_initcall time: skb_copy_bits() recurses into the child, sees the
> unreadable frag and fails.  The hook then calls skb_condense() on the
> same skb, so the failed pull must not shrink truesize.
> 
>   unpatched                             patched
>   kernel BUG at                         __pskb_pull_tail() returns NULL,
>   net/core/skbuff.c:2903                skb_condense() keeps truesize 960,
>                                         boot continues, no BUG
> 
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 48f676411724..8a200252ef93 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -2900,8 +2900,11 @@ void *__pskb_pull_tail(struct sk_buff *skb, int delta)
>  			return NULL;
>  	}
>  
> -	BUG_ON(skb_copy_bits(skb, skb_headlen(skb),
> -			     skb_tail_pointer(skb), delta));
> +	if (unlikely(skb_copy_bits(skb, skb_headlen(skb),
> +				   skb_tail_pointer(skb), delta))) {
> +		DEBUG_NET_WARN_ON_ONCE(1);
> +		return NULL;
> +	}
>  
>  	/* Optimization: no fragments, no reasons to preestimate
>  	 * size of pulled pages. Superb.
> @@ -7218,7 +7221,8 @@ void skb_condense(struct sk_buff *skb)
>  			return;
>  
>  		/* Nice, we can free page frag(s) right now */
> -		__pskb_pull_tail(skb, skb->data_len);
> +		if (!__pskb_pull_tail(skb, skb->data_len))
> +			return;	/* frags are still there, keep truesize */
>  	}
>  	/* At this point, skb->truesize might be over estimated,
>  	 * because skb had a fragment, and fragments do not tell
> --