Received-SPF: pass (google.com: domain of linux-kernel+bounces-147745-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Tue, 16 Apr 2024 19:14:40 -0400
From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
To: =?UTF-8?B?TWFjaWVqIMW7ZW5jenlrb3dza2k=?= <maze@google.com>, 
 Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Cc: =?UTF-8?B?TGVuYSBXYW5nICjnjovlqJwp?= <Lena.Wang@mediatek.com>, 
 "steffen.klassert@secunet.com" <steffen.klassert@secunet.com>, 
 "kuba@kernel.org" <kuba@kernel.org>, 
 =?UTF-8?B?U2hpbWluZyBDaGVuZyAo5oiQ6K+X5piOKQ==?= <Shiming.Cheng@mediatek.com>, 
 "pabeni@redhat.com" <pabeni@redhat.com>, 
 "edumazet@google.com" <edumazet@google.com>, 
 "matthias.bgg@gmail.com" <matthias.bgg@gmail.com>, 
 "davem@davemloft.net" <davem@davemloft.net>, 
 "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, 
 "netdev@vger.kernel.org" <netdev@vger.kernel.org>, 
 bpf <bpf@vger.kernel.org>
Message-ID: <661f066060ab4_7a39f2945d@willemb.c.googlers.com.notmuch>
In-Reply-To: <CANP3RGd+Zd-bx6S-NzeGch_crRK2w0-u6xwSVn71M581uCp9cQ@mail.gmail.com>
References: <20240415150103.23316-1-shiming.cheng@mediatek.com>
 <661d93b4e3ec3_3010129482@willemb.c.googlers.com.notmuch>
 <65e3e88a53d466cf5bad04e5c7bc3f1648b82fd7.camel@mediatek.com>
 <CANP3RGdkxT4TjeSvv1ftXOdFQd5Z4qLK1DbzwATq_t_Dk+V8ig@mail.gmail.com>
 <661eb25eeb09e_6672129490@willemb.c.googlers.com.notmuch>
 <CANP3RGdrRDERiPFVQ1nZYVtopErjqOQ72qQ_+ijGQiL7bTtcLQ@mail.gmail.com>
 <CANP3RGd+Zd-bx6S-NzeGch_crRK2w0-u6xwSVn71M581uCp9cQ@mail.gmail.com>
Subject: Re: [PATCH net] udp: fix segmentation crash for GRO packet without
 fraglist
Precedence: bulk
Mime-Version: 1.0
Content-Type: text/plain;
 charset=utf-8
Content-Transfer-Encoding: quoted-printable

> > > > Personally, I think bpf_skb_pull_data() should have automatically=

> > > > (ie. in kernel code) reduced how much it pulls so that it would p=
ull
> > > > headers only,
> > >
> > > That would be a helper that parses headers to discover header lengt=
h.
> >
> > Does it actually need to?  Presumably the bpf pull function could
> > notice that it is
> > a packet flagged as being of type X (UDP GSO FRAGLIST) and reduce the=
 pull
> > accordingly so that it doesn't pull anything from the non-linear
> > fraglist portion???
> >
> > I know only the generic overview of what udp gso is, not any details,=
 so I am
> > assuming here that there's some sort of guarantee to how these packet=
s
> > are structured...  But I imagine there must be or we wouldn't be hitt=
ing these
> > issues deeper in the stack?
> =

> Perhaps for a packet of this type we're already guaranteed the headers
> are in the linear portion,
> and the pull should simply be ignored?
> =

> >
> > > Parsing is better left to the BPF program.

I do prefer adding sanity checks to the BPF helpers, over having to
add then in the net hot path only to protect against dangerous BPF
programs.

In this case, it would be detecting this GSO type and failing the
operation if exceeding skb_headlen().
> > >
> > > > and not packet content.
> > > > (This is assuming the rest of the code isn't ready to deal with a=
 longer pull,
> > > > which I think is the case atm.  Pulling too much, and then crashi=
ng or forcing
> > > > the stack to drop packets because of them being malformed seems w=
rong...)
> > > >
> > > > In general it would be nice if there was a way to just say pull a=
ll headers...
> > > > (or possibly all L2/L3/L4 headers)
> > > > You in general need to pull stuff *before* you've even looked at =
the packet,
> > > > so that you can look at the packet,
> > > > so it's relatively hard/annoying to pull the correct length from =
bpf
> > > > code itself.
> > > >
> > > > > > > BPF needs to modify a proper length to do pull data. Howeve=
r kernel
> > > > > > > should also improve the flow to avoid crash from a bpf func=
tion
> > > > > > call.
> > > > > > > As there is no split flow and app may not decode the merged=
 UDP
> > > > > > packet,
> > > > > > > we should drop the packet without fraglist in skb_segment_l=
ist
> > > > > > here.
> > > > > > >
> > > > > > > Fixes: 3a1296a38d0c ("net: Support GRO/GSO fraglist chainin=
g.")
> > > > > > > Signed-off-by: Shiming Cheng <shiming.cheng@mediatek.com>
> > > > > > > Signed-off-by: Lena Wang <lena.wang@mediatek.com>
> > > > > > > ---
> > > > > > >  net/core/skbuff.c | 3 +++
> > > > > > >  1 file changed, 3 insertions(+)
> > > > > > >
> > > > > > > diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> > > > > > > index b99127712e67..f68f2679b086 100644
> > > > > > > --- a/net/core/skbuff.c
> > > > > > > +++ b/net/core/skbuff.c
> > > > > > > @@ -4504,6 +4504,9 @@ struct sk_buff *skb_segment_list(stru=
ct
> > > > > > sk_buff *skb,
> > > > > > >  if (err)
> > > > > > >  goto err_linearize;
> > > > > > >
> > > > > > > +if (!list_skb)
> > > > > > > +goto err_linearize;
> > > > > > > +
> > >
> > > This would catch the case where the entire data frag_list is
> > > linearized, but not a pskb_may_pull that only pulls in part of the
> > > list.
> > >
> > > Even with BPF being privileged, the kernel should not crash if BPF
> > > pulls a FRAGLIST GSO skb.
> > >
> > > But the check needs to be refined a bit. For a UDP GSO packet, I
> > > think gso_size is still valid, so if the head_skb length does not
> > > match gso_size, it has been messed with and should be dropped.
> > >
> > > For a GSO_BY_FRAGS skb, there is no single gso_size, and this pull
> > > may be entirely undetectable as long as frag_list !=3D NULL?
> > >
> > >
> > > > > > >  skb_shinfo(skb)->frag_list =3D NULL;
> > > > > >
> > > > > > In absense of plugging the issue in BPF, dropping here is the=
 best
> > > > > > we can do indeed, I think.
> =

> --
> Maciej =C5=BBenczykowski, Kernel Networking Developer @ Google