Received-SPF: pass (google.com: domain of linux-kernel+bounces-155757-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Tue, 23 Apr 2024 14:35:00 -0400
From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
To: =?UTF-8?B?TGVuYSBXYW5nICjnjovlqJwp?= <Lena.Wang@mediatek.com>, 
 "maze@google.com" <maze@google.com>, 
 "willemdebruijn.kernel@gmail.com" <willemdebruijn.kernel@gmail.com>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, 
 "bpf@vger.kernel.org" <bpf@vger.kernel.org>, 
 "steffen.klassert@secunet.com" <steffen.klassert@secunet.com>, 
 "kuba@kernel.org" <kuba@kernel.org>, 
 =?UTF-8?B?U2hpbWluZyBDaGVuZyAo5oiQ6K+X5piOKQ==?= <Shiming.Cheng@mediatek.com>, 
 "pabeni@redhat.com" <pabeni@redhat.com>, 
 "edumazet@google.com" <edumazet@google.com>, 
 "netdev@vger.kernel.org" <netdev@vger.kernel.org>, 
 "matthias.bgg@gmail.com" <matthias.bgg@gmail.com>, 
 "davem@davemloft.net" <davem@davemloft.net>, 
 "yan@cloudflare.com" <yan@cloudflare.com>
Message-ID: <6627ff5432c3a_1759e929467@willemb.c.googlers.com.notmuch>
In-Reply-To: <9f097bcafc5bacead23c769df4c3f63a80dcbad5.camel@mediatek.com>
References: <20240415150103.23316-1-shiming.cheng@mediatek.com>
 <661d93b4e3ec3_3010129482@willemb.c.googlers.com.notmuch>
 <65e3e88a53d466cf5bad04e5c7bc3f1648b82fd7.camel@mediatek.com>
 <CANP3RGdkxT4TjeSvv1ftXOdFQd5Z4qLK1DbzwATq_t_Dk+V8ig@mail.gmail.com>
 <661eb25eeb09e_6672129490@willemb.c.googlers.com.notmuch>
 <CANP3RGdrRDERiPFVQ1nZYVtopErjqOQ72qQ_+ijGQiL7bTtcLQ@mail.gmail.com>
 <CANP3RGd+Zd-bx6S-NzeGch_crRK2w0-u6xwSVn71M581uCp9cQ@mail.gmail.com>
 <661f066060ab4_7a39f2945d@willemb.c.googlers.com.notmuch>
 <77068ef60212e71b270281b2ccd86c8c28ee6be3.camel@mediatek.com>
 <662027965bdb1_c8647294b3@willemb.c.googlers.com.notmuch>
 <11395231f8be21718f89981ffe3703da3f829742.camel@mediatek.com>
 <CANP3RGdh24xyH2V7Sa2fs9Ca=tiZNBdKu1qQ8LFHS3sY41CxmA@mail.gmail.com>
 <b24bc70ae2c50dc50089c45afbed34904f3ee189.camel@mediatek.com>
 <66227ce6c1898_116a9b294be@willemb.c.googlers.com.notmuch>
 <CANP3RGfxeKDUmGwSsZrAs88Fmzk50XxN+-MtaJZTp641aOhotA@mail.gmail.com>
 <6622acdd22168_122c5b2945@willemb.c.googlers.com.notmuch>
 <9f097bcafc5bacead23c769df4c3f63a80dcbad5.camel@mediatek.com>
Subject: Re: [PATCH net] udp: fix segmentation crash for GRO packet without
 fraglist
Precedence: bulk
Mime-Version: 1.0
Content-Type: text/plain;
 charset=utf-8
Content-Transfer-Encoding: 7bit

> Hi Willem,
> As the discussion, is it OK for the patch below?

Thanks for iterating on this.

I would like the opinion also of the fraglist and UDP GRO experts.
 
Yes, I think both

- protecting skb_segment_list against clearly illegal fraglist packets, and
- blocking BPF from constructing such packets

are worthwhile stable fixes. I believe they should be two separate
patches. Both probably with the same Fixes tag: 3a1296a38d0c
("net: Support GRO/GSO fraglist chaining").

> diff --git a/net/core/filter.c b/net/core/filter.c
> index 3a6110ea4009..abc6029c8eef 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -1655,6 +1655,11 @@ static DEFINE_PER_CPU(struct bpf_scratchpad,
> bpf_sp);
>  static inline int __bpf_try_make_writable(struct sk_buff *skb,
>                                           unsigned int write_len)
>  {
> +       if (skb_is_gso(skb) && (skb_shinfo(skb)->gso_type &
> +                       SKB_GSO_FRAGLIST) && (write_len >
> skb_headlen(skb))) {
> +               return -ENOMEM;
> +       }
> +

Indentation looks off, but I agree with the logic.

    if (skb_is_gso(skb) &&
        (skb_shinfo(skb)->gso_type & SKB_GSO_FRAGLIST) &&
         (write_len > skb_headlen(skb)))

>         return skb_ensure_writable(skb, write_len);
>  }
> 
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 73b1e0e53534..2e90534c1a1e 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -4036,9 +4036,11 @@ struct sk_buff *skb_segment_list(struct sk_buff
> *skb,
>         unsigned int tnl_hlen = skb_tnl_header_len(skb);
>         unsigned int delta_truesize = 0;
>         unsigned int delta_len = 0;
> +       unsigned int mss = skb_shinfo(skb)->gso_size;
>         struct sk_buff *tail = NULL;
>         struct sk_buff *nskb, *tmp;
>         int len_diff, err;
> +       bool err_len = false;
> 
>         skb_push(skb, -skb_network_offset(skb) + offset);
> 
> @@ -4047,6 +4049,14 @@ struct sk_buff *skb_segment_list(struct sk_buff
> *skb,
>         if (err)
>                 goto err_linearize;
> 
> +       if (mss != GSO_BY_FRAGS && mss != skb_headlen(skb)) {
> +               if (!list_skb) {
> +                       goto err_linearize;

The label no longer truly covers the meaning.

But that is already true since the above (second) jump was added in
commit c329b261afe7 ("net: prevent skb corruption on frag list
segmentation").

Neither needs the kfree_skb_list, as skb->next is not assigned to
until the loop. Can just return ERR_PTR(-EFAULT)?

> +               } else {
> +                       err_len = true;
> +               }
> +       }
> +

Why the branch? Might as well always fail immediately?

>         skb_shinfo(skb)->frag_list = NULL;
> 
>         while (list_skb) {
> @@ -4109,6 +4119,9 @@ struct sk_buff *skb_segment_list(struct sk_buff
> *skb,
>             __skb_linearize(skb))
>                 goto err_linearize;
> 
> +       if (err_len)
> +               goto err_linearize;
> +
>         skb_get(skb);
> 
>         return skb;
> 
> > > 
> > > > Back to the original report: the issue should already have been
> > fixed
> > > > by commit 876e8ca83667 ("net: fix NULL pointer in
> > skb_segment_list").
> > > > But that commit is in the kernel for which you report the error.
> > > >
> > > > Turns out that the crash is not in skb_segment_list, but later in
> > > > __udpv4_gso_segment_list_csum. Which unconditionally dereferences
> > > > udp_hdr(seg).
> > > >
> > > > The above fix also mentions skb pull as the culprit, but does not
> > > > include a BPF program. If this can be reached in other ways, then
> > we
> > > > do need a stronger test in skb_segment_list, as you propose.
> > > >
> > > > I don't want to narrowly check whether udp_hdr is safe.
> > Essentially,
> > > > an SKB_GSO_FRAGLIST skb layout cannot be trusted at all if even
> > one
> > > > byte would get pulled.