Message-ID: <CANP3RGeOmrawPUmWeo5VLnzUt3cwDE6pDp3D9d8_xqwicghXpA@mail.gmail.com>
Date: Thu, 25 Apr 2024 17:16:08 -0700
From: Maciej Żenczykowski <maze@...gle.com>
To: Lena Wang (王娜) <Lena.Wang@...iatek.com>
Cc: "willemdebruijn.kernel@...il.com" <willemdebruijn.kernel@...il.com>, 
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "bpf@...r.kernel.org" <bpf@...r.kernel.org>, 
	"steffen.klassert@...unet.com" <steffen.klassert@...unet.com>, "kuba@...nel.org" <kuba@...nel.org>, 
	Shiming Cheng (成诗明) <Shiming.Cheng@...iatek.com>, 
	"pabeni@...hat.com" <pabeni@...hat.com>, "edumazet@...gle.com" <edumazet@...gle.com>, 
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>, "matthias.bgg@...il.com" <matthias.bgg@...il.com>, 
	"davem@...emloft.net" <davem@...emloft.net>, "yan@...udflare.com" <yan@...udflare.com>
Subject: Re: [PATCH net] udp: fix segmentation crash for GRO packet without fraglist

On Wed, Apr 24, 2024 at 9:33 PM Lena Wang (王娜) <Lena.Wang@...iatek.com> wrote:
>
> On Wed, 2024-04-24 at 10:28 -0400, Willem de Bruijn wrote:
> >
> > External email : Please do not click links or open attachments until
> > you have verified the sender or the content.
> >
> > Lena Wang (王娜) 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 = 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?
> > > >
> > > 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@...iatek.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@...iatek.com>
> > > Signed-off-by: Lena Wang <lena.wang@...iatek.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@...iatek.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@...iatek.com>
> > > Signed-off-by: Lena Wang <lena.wang@...iatek.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 = skb_tnl_header_len(skb);
> > >  unsigned int delta_truesize = 0;
> > >  unsigned int delta_len = 0;
> > > +unsigned int mss = skb_shinfo(skb)->gso_size;
> >
> > Reverse christmas tree
> >
> > >  struct sk_buff *tail = 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 != GSO_BY_FRAGS && mss != skb_headlen(skb))
> > > +return ERR_PTR(-EFAULT);
> > > +
> >
> > Do this precondition integrity check before the skb_unclone path?
>
> After return error, the skb will enter into kfree_skb, not consume_skb.
> It may meet same crash problem which has been resolved by skb_unclone.
>
> Or kfree_skb could well handle the cloned skb's release?
>
> Other changes are updated as below:
>
> From 301da5c9d65652bac6091d4cd64b751b3338f8bb Mon Sep 17 00:00:00 2001
> From: Shiming Cheng <shiming.cheng@...iatek.com>
> Date: Wed, 24 Apr 2024 13:42:35 +0800
> Subject: [PATCH net] net: prevent BPF pulling SKB_GSO_FRAGLIST skb
>
> A SKB_GSO_FRAGLIST skb can't be pulled data
> from its fraglist as it may result an invalid
> segmentation or kernel exception.
>
> For such structured skb we limit the BPF pulling
> 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@...iatek.com>
> Signed-off-by: Lena Wang <lena.wang@...iatek.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)) {

at least call:
  skb_ensure_writable(skb, skb_headlen(skb));
here before you return the error to make sure the already linear
header portion is writable.
(the skb could be a clone with a r/o header afaict)

> +               return -ENOMEM;
> +       }
>         return skb_ensure_writable(skb, write_len);
>  }
>
> --
> 2.18.0
>
>
> From 64d55392debbc90ef2e9c33441024d612075bdd7 Mon Sep 17 00:00:00 2001
> From: Shiming Cheng <shiming.cheng@...iatek.com>
> Date: Wed, 24 Apr 2024 14:43:45 +0800
> Subject: [PATCH net] net: drop pulled SKB_GSO_FRAGLIST skb
>
> A 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 part of or the entire fraglist has been pulled.
> It has been messed with and we should return error to
> free this skb.
>
> Fixes: 3a1296a38d0c ("net: Support GRO/GSO fraglist chaining.")
> Signed-off-by: Shiming Cheng <shiming.cheng@...iatek.com>
> Signed-off-by: Lena Wang <lena.wang@...iatek.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..4777f5fea6c3 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -4491,6 +4491,7 @@ struct sk_buff *skb_segment_list(struct sk_buff
> *skb,
>  {
>         struct sk_buff *list_skb = skb_shinfo(skb)->frag_list;
>         unsigned int tnl_hlen = skb_tnl_header_len(skb);
> +       unsigned int mss = skb_shinfo(skb)->gso_size;
>         unsigned int delta_truesize = 0;
>         unsigned int delta_len = 0;
>         struct sk_buff *tail = NULL;
> @@ -4504,6 +4505,9 @@ struct sk_buff *skb_segment_list(struct sk_buff
> *skb,
>         if (err)
>                 goto err_linearize;
>
> +       if (mss != GSO_BY_FRAGS && mss != skb_headlen(skb))
> +               return ERR_PTR(-EFAULT);
> +
>         skb_shinfo(skb)->frag_list = NULL;
>
>         while (list_skb) {
> --
> 2.18.0

--
Maciej Żenczykowski, Kernel Networking Developer @ Google

Powered by blists - more mailing lists