| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <752468b66d2f5766ea16381a0c5d7b82ab77c5c4.camel@mediatek.com>
Date: Fri, 26 Apr 2024 09:52:37 +0000
From: Lena Wang (王娜) <Lena.Wang@...iatek.com>
To: "maze@...gle.com" <maze@...gle.com>, "willemdebruijn.kernel@...il.com"
<willemdebruijn.kernel@...il.com>
CC: "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 Thu, 2024-04-25 at 10:07 -0400, Willem de Bruijn wrote:
>
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>
> > > > 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?
>
> Since this is an error path it should reach kfree_skb rather than
> consume_skb.
>
So we could keep the check after the skb_unclone path, right?
> >
> > 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)) {
> > +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.
>
> Drop the "though, right?"
> >
> > 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.
>
> This paragraph is now duplicative. Drop.
OK, updated as below:
From 59d561adc13d52e3c225c6b8276f6a53324f7d56 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. 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.
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
> >
> > 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
>
>
Powered by blists - more mailing lists