Received: by 2002:a05:6500:1b45:b0:1f5:f2ab:c469 with SMTP id cz5csp1060059lqb; Wed, 17 Apr 2024 21:16:06 -0700 (PDT) X-Forwarded-Encrypted: i=3; AJvYcCWWhLAPTcVeJCspxyPkdQeZLWz8P65AbeqbJDjhjLYYIjtopv9h59MDwhIct0zNlOOy/UKMFtz9DOioYMHvs6T6yA29eLrLUvJ6pnH06Q== X-Google-Smtp-Source: AGHT+IExJKrF1/7ph/DYA47EKn6z1Pupgo2utS+Xa+e1vPj6GE7CYCLJOsMZvaaYUVMrHY5ve/O0 X-Received: by 2002:a05:622a:1910:b0:436:8890:6aca with SMTP id w16-20020a05622a191000b0043688906acamr1994223qtc.31.1713413766345; Wed, 17 Apr 2024 21:16:06 -0700 (PDT) ARC-Seal: i=2; a=rsa-sha256; t=1713413766; cv=pass; d=google.com; s=arc-20160816; b=WTudOh7LylBmHzkijiMxQZWlwivGnzvpa5V6IA34jJ7ULyOxWI7LMPpzzb8uKaIfx7 uNHPHie+VbP2zVTR7l3R1m0u1uJpv9Zb3FtypNd8HjLv2Ombm+tygui4dwvMi5SluP1J r0CU69zIA6H+hjK4S2eCtM9pmCu/sD/XffR388TNKqLO3YCwOyXw6+VPBC0y1Y6C0oZ+ QyL3H9ASCjkUQ9x+lZlL+EKp67xDG6JxYwHuW+u193tIM/3LOmpu1EADWsj0qU1u7rtX 5/3+usXu2xRCBLOJgZ94va+E+SiyWiTnOeItLB8z6x1AnOlj7kDrWbZ1BW0Q+HmDRqbP YK3w== ARC-Message-Signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:list-unsubscribe:list-subscribe :list-id:precedence:dkim-signature; bh=6Ex9vkWAdcavtfrxF59Az8ojAz4XUwKMAJT3nUTpt4g=; fh=aiMrnY3L7hpQcZ02H0RqNVBrfyaeUA5crNg2OtA+cY0=; b=C14VO5Nfd/dNOPI2SOU9LnI8Fb+w3CY4Y2GD+GFRetXHoHBGrK1UKn/U6UZC7xLxUy sHFIaA4zqT5flFCiWmR/7rU98g1Ab1r5SJ+BAAh1ysUWFvn8ZkNU1rZGFX2nrmU6WiED YnpJy1LXCIwTpQ55fA4WtANXVZ+dFLebRK61qF2z0P9EKd6OUzdYvUCpBatr+Z+GCNED xLrnPydfq3dKFzmueH1a1gpEPwief8KyIWM4peRbQdivjScksJHJk20ktXzoK0eP+69M mzfKE5gINZmTe9aQQKwGoVmfmyUwEkKM/P+tNNpJccsm4GeY6rXrWaqPjCDoUhei120r etVw==; dara=google.com ARC-Authentication-Results: i=2; mx.google.com; dkim=pass header.i=@google.com header.s=20230601 header.b=o16NOjeI; arc=pass (i=1 spf=pass spfdomain=google.com dkim=pass dkdomain=google.com dmarc=pass fromdomain=google.com); spf=pass (google.com: domain of linux-kernel+bounces-149495-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) smtp.mailfrom="linux-kernel+bounces-149495-linux.lists.archive=gmail.com@vger.kernel.org"; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Return-Path: Received: from ny.mirrors.kernel.org (ny.mirrors.kernel.org. [147.75.199.223]) by mx.google.com with ESMTPS id c4-20020a05620a11a400b0078d676fef0asi682978qkk.31.2024.04.17.21.16.06 for (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 17 Apr 2024 21:16:06 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel+bounces-149495-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223; Authentication-Results: mx.google.com; dkim=pass header.i=@google.com header.s=20230601 header.b=o16NOjeI; arc=pass (i=1 spf=pass spfdomain=google.com dkim=pass dkdomain=google.com dmarc=pass fromdomain=google.com); spf=pass (google.com: domain of linux-kernel+bounces-149495-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) smtp.mailfrom="linux-kernel+bounces-149495-linux.lists.archive=gmail.com@vger.kernel.org"; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ny.mirrors.kernel.org (Postfix) with ESMTPS id E4C7E1C20DB2 for ; Thu, 18 Apr 2024 04:16:05 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 3885554BC3; Thu, 18 Apr 2024 04:15:58 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="o16NOjeI" Received: from mail-ed1-f49.google.com (mail-ed1-f49.google.com [209.85.208.49]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 0AFEF54794 for ; Thu, 18 Apr 2024 04:15:54 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.208.49 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1713413757; cv=none; b=o32neyRVbQf1o5to2Xy0kNNZUFNsQg/Zg0QKj+r9ZkasgRPsjpVQeU+/G2LovbIWYnFk6wwVlHhpi/rkD3GGuL15wJYODfukjgE4NPvG5XzcyEQok+n+ximXadGILmTav5dlsvrbMr+8CByZgaY+NvwQJ8xr0Yt9qyEblfLjVAg= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1713413757; c=relaxed/simple; bh=rIjhTSCAnsnjiu64hl14QjOMSBjuVyUzL8jWgHEufos=; h=MIME-Version:References:In-Reply-To:From:Date:Message-ID:Subject: To:Cc:Content-Type; b=lgFffqx47sBldftaWZ7jRL27Gz6iGR626P8+SSNqJrAuCcDxAQFwCJvmQkVZoNe6a7kIyCE/Bejh6JIwXGWdtmsb8HccZ9X8eOuGSgDD2hCjVWyZ8TUr5cwtN5TCpxc+115Hxyf9+D52aDQgFEZilpS7XhDQUsUY6kAO/2A+8Y4= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com; spf=pass smtp.mailfrom=google.com; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b=o16NOjeI; arc=none smtp.client-ip=209.85.208.49 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=google.com Received: by mail-ed1-f49.google.com with SMTP id 4fb4d7f45d1cf-56e2e851794so4659a12.0 for ; Wed, 17 Apr 2024 21:15:54 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1713413753; x=1714018553; darn=vger.kernel.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=6Ex9vkWAdcavtfrxF59Az8ojAz4XUwKMAJT3nUTpt4g=; b=o16NOjeI1QWj9dQkYZLX2YQuSzrHvu/k3nkubK07UFXv0FWnfxz1Ngw0p3yC54CjQX tXYqsBqU9Nv3M7s6QebiX9Y4XAOzSI/QyFhtGTw/rflH2sIEGYYo2/8lm7YpVjzcTAYz 8KCCjeFpCxN6gZfBCDKu7xCPKHGal7ExYSegPOi7RzTPzrqcmmDTL32WFy89HyquDjgZ iNZpNhE8oRtH5eUzin2ub9DUGfZ7kPZPFyelPLqy1Zjr8dgPiSjx48+AfXYXAmNAZowi neI+dinR/n1yljPKvdpWOLOGHs4XRk76QNebpJ+tUbo8HrNlW64Yyp5ZWn4pdGWD/JyH xA0g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1713413753; x=1714018553; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=6Ex9vkWAdcavtfrxF59Az8ojAz4XUwKMAJT3nUTpt4g=; b=nhyv+8/YdQfiqcm1UPcaOiQnuq+1entvzFsQeKFFvkylltSjn+AeHbNwuEmIJ30sBV RsEI1k7EwkGOV9eEFcmLEvx627TeIcy1pJBkzh0Z3rcYyNBBODDmuBHLSfpPK+SHsrFj 1t7M+7w9vTqDPqOn2POccJSI0Y80BvNWDnH7Iqp++1cjlL87TORQnk2y0qAplaH8qym0 MRGIVh0Yd7HlWfdwvkYClcJ6GQdgZHUsEpd2/lbfLlERez8gNrvyswsnlaB4NH9h3Mys rRTF7jFwU9n5r4OkNpkDYTFq3RLU0RWJnE1sqSss6c2txa5qlvOUW8CvdGN7EOvl3yl/ kVcA== X-Forwarded-Encrypted: i=1; AJvYcCWlWFmG9LeH1ldA44xZWnKqByUZ8zFQlHBMkcoNsqtTVQWEmWxS7PeZbJcBx93mYG17ZpEQWA+wRfpzz7jA1VaE6UJDsPBuuheYwl/z X-Gm-Message-State: AOJu0YzZhwbe2McFsdtHhMmmbOcIdoaBNrC9eOuUUFdt3/+MNCP6uit5 dZU3sWORjHQ+hoUldIxuuFh3J7+yul0jRyIu1P1gAZ/4T6d2KJNDCX4y9udaHZShLvHmPqJh2Sh 8DanyqZFpBrvkbxEKX1tvxKoTKnGa0kgKBIygZq5UO5B5tsLI0H25 X-Received: by 2002:a05:6402:50a:b0:571:b9c7:2804 with SMTP id m10-20020a056402050a00b00571b9c72804mr45961edv.5.1713413753150; Wed, 17 Apr 2024 21:15:53 -0700 (PDT) Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 References: <20240415150103.23316-1-shiming.cheng@mediatek.com> <661d93b4e3ec3_3010129482@willemb.c.googlers.com.notmuch> <65e3e88a53d466cf5bad04e5c7bc3f1648b82fd7.camel@mediatek.com> <661eb25eeb09e_6672129490@willemb.c.googlers.com.notmuch> <661f066060ab4_7a39f2945d@willemb.c.googlers.com.notmuch> <77068ef60212e71b270281b2ccd86c8c28ee6be3.camel@mediatek.com> <662027965bdb1_c8647294b3@willemb.c.googlers.com.notmuch> <11395231f8be21718f89981ffe3703da3f829742.camel@mediatek.com> In-Reply-To: <11395231f8be21718f89981ffe3703da3f829742.camel@mediatek.com> From: =?UTF-8?Q?Maciej_=C5=BBenczykowski?= Date: Wed, 17 Apr 2024 21:15:38 -0700 Message-ID: Subject: Re: [PATCH net] udp: fix segmentation crash for GRO packet without fraglist To: =?UTF-8?B?TGVuYSBXYW5nICjnjovlqJwp?= Cc: "willemdebruijn.kernel@gmail.com" , "linux-kernel@vger.kernel.org" , "bpf@vger.kernel.org" , "steffen.klassert@secunet.com" , "kuba@kernel.org" , =?UTF-8?B?U2hpbWluZyBDaGVuZyAo5oiQ6K+X5piOKQ==?= , "pabeni@redhat.com" , "edumazet@google.com" , "netdev@vger.kernel.org" , "matthias.bgg@gmail.com" , "davem@davemloft.net" Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Wed, Apr 17, 2024 at 7:53=E2=80=AFPM Lena Wang (=E7=8E=8B=E5=A8=9C) wrote: > > On Wed, 2024-04-17 at 15:48 -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 (=E7=8E=8B=E5=A8=9C) wrote: > > > On Tue, 2024-04-16 at 19:14 -0400, Willem de Bruijn wrote: > > > > > > > > External email : Please do not click links or open attachments > > until > > > > you have verified the sender or the content. > > > > > > > > Personally, I think bpf_skb_pull_data() should have > > > > automatically > > > > > > > > (ie. in kernel code) reduced how much it pulls so that it > > > > would pull > > > > > > > > headers only, > > > > > > > > > > > > > > That would be a helper that parses headers to discover > > header > > > > length. > > > > > > > > > > > > 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 > > > > packets > > > > > > are structured... But I imagine there must be or we wouldn't > > be > > > > hitting 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. > > > > > > > Is it OK to ignore or decrease pull length for udp gro fraglist > > packet? > > > It could save the normal packet and sent to user correctly. > > > > > > In common/net/core/filter.c > > > 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_UDP |SKB_GSO_UDP_L4)) { > > > > The issue is not with SKB_GSO_UDP_L4, but with SKB_GSO_FRAGLIST. > > > Current in kernel just UDP uses SKB_GSO_FRAGLIST to do GRO. In > udp_offload.c udp4_gro_complete gso_type adds "SKB_GSO_FRAGLIST| > SKB_GSO_UDP_L4". Here checking these two flags is to limit the packet > as "UDP + need GSO + fraglist". > > We could remove SKB_GSO_UDP_L4 check for more packet that may addrive > skb_segment_list. > > > > +return 0; > > > > Failing for any pull is a bit excessive. And would kill a sane > > workaround of pulling only as many bytes as needed. > > > > > + or if (write_len > skb_headlen(skb)) > > > +write_len =3D skb_headlen(skb); > > > > Truncating requests would be a surprising change of behavior > > for this function. > > > > Failing for a pull > skb_headlen is arguably reasonable, as > > the alternative is that we let it go through but have to drop > > the now malformed packets on segmentation. > > > > > Is it OK as below? > > In common/net/core/filter.c > 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 0; please limit write_len to skb_headlen() instead of just returning 0 > + } > return skb_ensure_writable(skb, write_len); > } > > > > +} > > > return skb_ensure_writable(skb, write_len); > > > } > > > > > > > > > > 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 > > > > crashing or forcing > > > > > > > > the stack to drop packets because of them being malformed > > > > seems wrong...) > > > > > > > > > > > > > > > > In general it would be nice if there was a way to just > > say > > > > pull all 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. > > > > However kernel > > > > > > > > > > > should also improve the flow to avoid crash from a > > bpf > > > > function > > > > > > > > > > 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_list > > > > > > > > > > here. > > > > > > > > > > > > > > > > > > > > > > Fixes: 3a1296a38d0c ("net: Support GRO/GSO fraglist > > > > chaining.") > > > > > > > > > > > Signed-off-by: Shiming Cheng < > > > > shiming.cheng@mediatek.com> > > > > > > > > > > > Signed-off-by: Lena Wang > > > > > > > > > > > --- > > > > > > > > > > > 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(struct > > > > > > > > > > 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. > > > > > > > > > > Is it OK as below? Is it OK to add log to record the error for easy > > > checking issue. > > > > > > In net/core/skbuff.c skb_segment_list > > > +unsigned int mss =3D skb_shinfo(head_skb)->gso_size; > > > +bool err_len =3D false; > > > > > > +if ( mss !=3D GSO_BY_FRAGS && mss !=3D skb_headlen(head_skb)) { > > > +pr_err("skb is dropped due to messed data. gso size:%d, > > > +hdrlen:%d", mss, skb_headlen(head_skb) > > > > Such logs should always be rate limited. But no need to log cases > > where we well understood how we get there. > > > > I would stick with one approach: either in the BPF func or in > > segmentation, not both. And then I find BPF preferable, as explained > > before. > > > OK, we try make a patch in BPF func. > > > > +if (!list_skb) > > > +goto err_linearize; > > > +else > > > +err_len =3D true; > > > +} > > > > > > ... > > > +if (err_len) { > > > +goto err_linearize; > > > +} > > > > > > skb_get(skb); > > > ... -- Maciej =C5=BBenczykowski, Kernel Networking Developer @ Google