Re: [PATCH net] udp: fix segmentation crash for GRO packet without fraglist

["Lena Wang (王娜)" <Lena.Wang@mediatek.com>]

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@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
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.

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
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 = 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;
@@ -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

> >         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.
> 
>