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@xxxxxxxxxxxx>
> > 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@xxxxxxxxxxxx>
> > Signed-off-by: Lena Wang <lena.wang@xxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxx>
> > 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@xxxxxxxxxxxx>
> > Signed-off-by: Lena Wang <lena.wang@xxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxx>
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@xxxxxxxxxxxx>
Signed-off-by: Lena Wang <lena.wang@xxxxxxxxxxxx>
---
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 64d55392debbc90ef2e9c33441024d612075bdd7 Mon Sep 17 00:00:00 2001
From: Shiming Cheng <shiming.cheng@xxxxxxxxxxxx>
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@xxxxxxxxxxxx>
Signed-off-by: Lena Wang <lena.wang@xxxxxxxxxxxx>
---
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
