Received-SPF: pass (google.com: domain of linux-kernel+bounces-157073-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Wed, 24 Apr 2024 10:28:38 -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: <66291716bcaed_1a760729446@willemb.c.googlers.com.notmuch>
In-Reply-To: <274c7e9837e5bbe468d19aba7718cc1cf0f9a6eb.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>
 <6627ff5432c3a_1759e929467@willemb.c.googlers.com.notmuch>
 <274c7e9837e5bbe468d19aba7718cc1cf0f9a6eb.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: quoted-printable

Lena Wang (=E7=8E=8B=E5=A8=9C) wrote:
> On Tue, 2024-04-23 at 14:35 -0400, Willem de Bruijn wrote:
> >  	 =

> > External email : Please do not click links or open attachments until
> > you have verified the sender or the content.
> >  > 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 =3D skb_tnl_header_len(skb);
> > >         unsigned int delta_truesize =3D 0;
> > >         unsigned int delta_len =3D 0;
> > > +       unsigned int mss =3D skb_shinfo(skb)->gso_size;
> > >         struct sk_buff *tail =3D NULL;
> > >         struct sk_buff *nskb, *tmp;
> > >         int len_diff, err;
> > > +       bool err_len =3D 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 !=3D GSO_BY_FRAGS && mss !=3D 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 =3D true;
> > > +               }
> > > +       }
> > > +
> > =

> > Why the branch? Might as well always fail immediately?
> > =

> Hi Willem,
> Thanks for your guidance.
> You are right. There is no need for another branch as fraglist
> could be freeed in kfree_skb.
> Could I git send mail wo patches as below?
> =

> From 933237400c0e2fa997470b70ff0e407996fa239c Mon Sep 17 00:00:00 2001
> From: Shiming Cheng <shiming.cheng@mediatek.com>
> Date: Wed, 24 Apr 2024 13:42:35 +0800
> Subject: [PATCH net] net: prevent BPF pull GROed skb's fraglist
> =

> A GROed skb with fraglist can't be pulled data

Please use the specific label: SKB_GSO_FRAGLIST skb

> from its fraglist as it may result a invalid
> segmentation or kernel exception.
> =

> For such structured skb we limit the BPF pull
> data length smaller than skb_headlen() and return
> error if exceeding.
> =

> Fixes: 3a1296a38d0c ("net: Support GRO/GSO fraglist chaining.")
> Signed-off-by: Shiming Cheng <shiming.cheng@mediatek.com>
> Signed-off-by: Lena Wang <lena.wang@mediatek.com>
> ---
>  net/core/filter.c | 5 +++++
>  1 file changed, 5 insertions(+)
> =

> diff --git a/net/core/filter.c b/net/core/filter.c
> index 8adf95765cdd..8ed4d5d87167 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -1662,6 +1662,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;
> +	}
>  	return skb_ensure_writable(skb, write_len);
>  }
>  =

> -- =

> 2.18.0
> =

> =

> From 2d0729b20cf810ba1b31e046952c1cd78f295ca3 Mon Sep 17 00:00:00 2001
> From: Shiming Cheng <shiming.cheng@mediatek.com>
> Date: Wed, 24 Apr 2024 14:43:45 +0800
> Subject: [PATCH net] net: drop GROed skb pulled from fraglist
> =

> A GROed skb with fraglist maybe pulled by BPF
> or other ways. It can't be trusted at all even
> if one byte is pulled and should be dropped
> on segmentation.

This paraphrases my comment. It is better to spell it out:

An SKB_GSO_FRAGLIST skb without GSO_BY_FRAGS is expected to have all
segments except the last to be gso_size long. If this does not hold,
the skb has been modified and the fraglist gso integrity is lost. Drop
the packet, as it cannot be segmented correctly by skb_segment_list.

The skb could be salvaged, though, right? By linearizing, dropping
the SKB_GSO_FRAGLIST bit and entering the normal skb_segment path
rather than the skb_segment_list path.

That choice is currently made in the protocol caller,
__udp_gso_segment. It's not trivial to add such a backup path here.
So let's add this backstop against kernel crashes.

> =

> If the gso_size does not match skb_headlen(),
> it means to be pulled part of or the entire
> fraglsit. It has been messed with and we return

fraglist

> error to free this skb.
> =

> Fixes: 3a1296a38d0c ("net: Support GRO/GSO fraglist chaining.")
> Signed-off-by: Shiming Cheng <shiming.cheng@mediatek.com>
> Signed-off-by: Lena Wang <lena.wang@mediatek.com>
> ---
>  net/core/skbuff.c | 4 ++++
>  1 file changed, 4 insertions(+)
> =

> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index b99127712e67..750fbb51b99f 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -4493,6 +4493,7 @@ struct sk_buff *skb_segment_list(struct sk_buff
> *skb,
>  	unsigned int tnl_hlen =3D skb_tnl_header_len(skb);
>  	unsigned int delta_truesize =3D 0;
>  	unsigned int delta_len =3D 0;
> +	unsigned int mss =3D skb_shinfo(skb)->gso_size;

Reverse christmas tree

>  	struct sk_buff *tail =3D NULL;
>  	struct sk_buff *nskb, *tmp;
>  	int len_diff, err;
> @@ -4504,6 +4505,9 @@ struct sk_buff *skb_segment_list(struct sk_buff
> *skb,
>  	if (err)
>  		goto err_linearize;
>  =

> +	if (mss !=3D GSO_BY_FRAGS && mss !=3D skb_headlen(skb))
> +		return ERR_PTR(-EFAULT);
> +

Do this precondition integrity check before the skb_unclone path?

>  	skb_shinfo(skb)->frag_list =3D NULL;
>  =

>  	while (list_skb) {
> -- =

> 2.18.0