Re: [PATCH net-next v2 11/15] net: skbuff: refuse to segment a nested frag_list

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Josef Bacik wrote:
> skb_segment() walks head_skb's frag_list, but for each member it only
> looks at the linear part and frags[], never at the member's own
> frag_list.  head_skb->len still counts those bytes, so with a nested
> frag_list the walk runs out of members while it still owes data, sets
> list_skb to NULL, and dereferences it on a later pass, before any of
> the layout checks get a chance to catch it.  When the member is taken
> whole with skb_clone(), the pskb_trim() that follows also drops its
> nested data.
> 
> Nothing in the tree is known to build such an skb; GRO flushes instead
> of chaining an skb that already has a frag_list.  Check each member as
> the walk takes it, in the GSO_BY_FRAGS, clone and frag paths, and fail
> the segmentation with -EINVAL if there's no member left or it has its
> own frag_list, with a DEBUG_NET_WARN_ON_ONCE() for debug kernels, like
> the other frag_list layout checks.  Without SG the payload is copied
> with helpers that do recurse into nested frag_lists, so that path is
> left alone.
> 
> Suggested-by: Willem de Bruijn <willemb@google.com>
> Assisted-by: LLM
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  net/core/skbuff.c | 22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 56cec58bccb7..c5e28a52114b 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -5087,6 +5087,15 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
>  		int size;
>  
>  		if (unlikely(gso_by_frags)) {
> +			/* The walk only goes one level deep: running out of
> +			 * members, or finding one with its own frag_list, means
> +			 * a nested frag_list that head_skb->len counts.
> +			 */
> +			if (unlikely(!list_skb || skb_has_frag_list(list_skb))) {
> +				DEBUG_NET_WARN_ON_ONCE(1);
> +				err = -EINVAL;
> +				goto err;
> +			}

This patch became quite a bit more complex because of my request.
More than I anticipated.

Are you sure this first block is needed?


>  			len = list_skb->len;
>  		} else {
>  			len = head_skb->len - offset;
> @@ -5096,9 +5105,11 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
>  
>  		hsize = skb_headlen(head_skb) - offset;
>  
> -		if (hsize <= 0 && i >= nfrags && skb_headlen(list_skb) &&
> +		if (hsize <= 0 && i >= nfrags && list_skb &&

this test for list_skb != NULL is new. Previously this code already
tested skb_headlen(list_skb) without it. Likely not needed for
anything new in this patch?

> +		    skb_headlen(list_skb) &&
>  		    (skb_headlen(list_skb) == len || sg)) {
> -			if (unlikely(skb_headlen(list_skb) > len)) {
> +			if (unlikely(skb_headlen(list_skb) > len ||
> +				     skb_has_frag_list(list_skb))) {
>  				DEBUG_NET_WARN_ON_ONCE(1);
>  				err = -EINVAL;
>  				goto err;
> @@ -5216,6 +5227,13 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
>  
>  		while (pos < offset + len) {
>  			if (i >= nfrags) {
> +				if (unlikely(!list_skb ||
> +					     skb_has_frag_list(list_skb))) {
> +					DEBUG_NET_WARN_ON_ONCE(1);
> +					err = -EINVAL;
> +					goto err;
> +				}
> +
>  				if (skb_orphan_frags(list_skb, GFP_ATOMIC) ||
>  				    skb_zerocopy_clone(nskb, list_skb,
>  						       GFP_ATOMIC))
> 
> -- 
> 2.55.0
>