Received-SPF: pass (google.com: domain of linux-kernel+bounces-161051-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Sat, 27 Apr 2024 09:28:13 -0400
From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
To: Daniel Borkmann <daniel@iogearbox.net>, 
 =?UTF-8?B?TGVuYSBXYW5nICjnjovlqJwp?= <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>, 
 =?UTF-8?B?U2hpbWluZyBDaGVuZyAo5oiQ6K+X5piOKQ==?= <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>
Message-ID: <662cfd6db06df_28b9852949a@willemb.c.googlers.com.notmuch>
In-Reply-To: <ae0ba22a-049a-49c1-d791-d0e953625904@iogearbox.net>
References: <20240415150103.23316-1-shiming.cheng@mediatek.com>
 <77068ef60212e71b270281b2ccd86c8c28ee6be3.camel@mediatek.com>
 <662027965bdb1_c8647294b3@willemb.c.googlers.com.notmuch>
 <11395231f8be21718f89981ffe3703da3f829742.camel@mediatek.com>
 <CANP3RGdh24xyH2V7Sa2fs9Ca=tiZNBdKu1qQ8LFHS3sY41CxmA@mail.gmail.com>
 <b24bc70ae2c50dc50089c45afbed34904f3ee189.camel@mediatek.com>
 <66227ce6c1898_116a9b294be@willemb.c.googlers.com.notmuch>
 <CANP3RGfxeKDUmGwSsZrAs88Fmzk50XxN+-MtaJZTp641aOhotA@mail.gmail.com>
 <6622acdd22168_122c5b2945@willemb.c.googlers.com.notmuch>
 <9f097bcafc5bacead23c769df4c3f63a80dcbad5.camel@mediatek.com>
 <6627ff5432c3a_1759e929467@willemb.c.googlers.com.notmuch>
 <274c7e9837e5bbe468d19aba7718cc1cf0f9a6eb.camel@mediatek.com>
 <66291716bcaed_1a760729446@willemb.c.googlers.com.notmuch>
 <c28a5c635f38a47f1be266c4328e5fbba44ff084.camel@mediatek.com>
 <662a63aeee385_1de39b294fd@willemb.c.googlers.com.notmuch>
 <752468b66d2f5766ea16381a0c5d7b82ab77c5c4.camel@mediatek.com>
 <ae0ba22a-049a-49c1-d791-d0e953625904@iogearbox.net>
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

Daniel Borkmann wrote:
> On 4/26/24 11:52 AM, Lena Wang (=E7=8E=8B=E5=A8=9C) wrote:
> [...]
> >>>  From 301da5c9d65652bac6091d4cd64b751b3338f8bb 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 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@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);
> =

> Dumb question, but should this guard be more generically part of skb_en=
sure_writable()
> internals, presumably that would be inside pskb_may_pull_reason(), or o=
nly if we ever
> see more code instances similar to this?

Good point. Most callers of skb_ensure_writable correctly pull only
headers, so wouldn't cause this problem. But it also adds coverage to
things like tc pedit.