From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
To: "Lena Wang (王娜)" <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>,
"Shiming Cheng (成诗明)" <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>
Subject: Re: [PATCH net] udp: fix segmentation crash for GRO packet without fraglist
Date: Wed, 24 Apr 2024 10:28:38 -0400 [thread overview]
Message-ID: <66291716bcaed_1a760729446@willemb.c.googlers.com.notmuch> (raw)
In-Reply-To: <274c7e9837e5bbe468d19aba7718cc1cf0f9a6eb.camel@mediatek.com>
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@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 = 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?
> skb_shinfo(skb)->frag_list = NULL;
>
> while (list_skb) {
> --
> 2.18.0
next prev parent reply other threads:[~2024-04-24 14:28 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-15 15:01 [PATCH net] udp: fix segmentation crash for GRO packet without fraglist shiming.cheng
2024-04-15 20:53 ` Willem de Bruijn
2024-04-16 2:14 ` Lena Wang (王娜)
2024-04-16 2:53 ` Maciej Żenczykowski
2024-04-16 17:16 ` Willem de Bruijn
2024-04-16 17:51 ` Maciej Żenczykowski
2024-04-16 17:57 ` Maciej Żenczykowski
2024-04-16 23:14 ` Willem de Bruijn
2024-04-17 7:19 ` Lena Wang (王娜)
2024-04-17 19:48 ` Willem de Bruijn
2024-04-18 2:52 ` Lena Wang (王娜)
2024-04-18 4:15 ` Maciej Żenczykowski
2024-04-19 8:36 ` Lena Wang (王娜)
2024-04-19 14:17 ` Willem de Bruijn
2024-04-19 17:29 ` Maciej Żenczykowski
2024-04-19 17:41 ` Willem de Bruijn
2024-04-23 14:47 ` Lena Wang (王娜)
2024-04-23 18:35 ` Willem de Bruijn
2024-04-24 12:22 ` Lena Wang (王娜)
2024-04-24 14:28 ` Willem de Bruijn [this message]
2024-04-25 4:32 ` Lena Wang (王娜)
2024-04-25 14:07 ` Willem de Bruijn
2024-04-26 9:52 ` Lena Wang (王娜)
2024-04-26 21:08 ` Daniel Borkmann
2024-04-27 13:28 ` Willem de Bruijn
2024-04-28 7:48 ` Lena Wang (王娜)
2024-04-28 13:19 ` Willem de Bruijn
2024-04-29 10:15 ` Daniel Borkmann
2024-04-29 11:45 ` Lena Wang (王娜)
2024-04-29 15:11 ` Daniel Borkmann
2024-04-29 21:14 ` Willem de Bruijn
2024-04-26 0:16 ` Maciej Żenczykowski
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=66291716bcaed_1a760729446@willemb.c.googlers.com.notmuch \
--to=willemdebruijn.kernel@gmail.com \
--cc=Lena.Wang@mediatek.com \
--cc=Shiming.Cheng@mediatek.com \
--cc=bpf@vger.kernel.org \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=matthias.bgg@gmail.com \
--cc=maze@google.com \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=steffen.klassert@secunet.com \
--cc=yan@cloudflare.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).