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

[Willem de Bruijn <willemdebruijn.kernel@xxxxxxxxx>]

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@xxxxxxxxxx>
> Assisted-by: LLM
> Signed-off-by: Josef Bacik <josef@xxxxxxxxxxxxxx>
> ---
>  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
>