From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 8DE4CC63797 for ; Mon, 6 Feb 2023 18:45:21 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230062AbjBFSpU (ORCPT ); Mon, 6 Feb 2023 13:45:20 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:57144 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229890AbjBFSpS (ORCPT ); Mon, 6 Feb 2023 13:45:18 -0500 Received: from mail-pj1-x102a.google.com (mail-pj1-x102a.google.com [IPv6:2607:f8b0:4864:20::102a]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id AC6F54ED0 for ; Mon, 6 Feb 2023 10:45:16 -0800 (PST) Received: by mail-pj1-x102a.google.com with SMTP id d2so8836142pjd.5 for ; Mon, 06 Feb 2023 10:45:16 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=in-reply-to:content-disposition:mime-version:references:subject:cc :to:from:date:message-id:from:to:cc:subject:date:message-id:reply-to; bh=DkhDQDKT3oVXHbvXapcI5zTOXdH8Icb/ANsh1giobys=; b=R/gvzAjpkUwIW2Ypz2voQVYC/0MAAoapXn89Mk0mzs1r9FBsjEjUzP53KbAdiKtEv+ nCHbrT375Kte2H7weMvqen9+Na7Dr06oH7ZK83AZshB8v0eIGE5Zvi/cZ+H1/C6WTs6u oXgNjd6akjn1A46hqWw6X7Uzgjv4jxq/Fn6h4= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=in-reply-to:content-disposition:mime-version:references:subject:cc :to:from:date:message-id:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=DkhDQDKT3oVXHbvXapcI5zTOXdH8Icb/ANsh1giobys=; b=i+yk5Ci+0v+XRkTNVKIVbFRFGev+f5qDcEw6xa2Wzf4OjZEeSj844V59uH8tpv4vAT RZRl6tLgMZqy6kgRkTxVyiEmm1xR9UzHvF6ggkFpb3QCY32v4rVLsENac8UWlwN8QvMl KvoprUkMnd96BBb215TH5DXqA6SPpQK1jkEX1PjgBhZm14Yy2zegcK8PowRoJQhzjXaT YT45+QPfZY1x8fdXboRTGrU3IMHjMw5oO023VSQpukPseOFW8mrZhYQqw4XeqCk0nEGw IKUzpygtgThWY75cW6boQGGIw9cy8Fl2Dj7wregPr6ElYwTxTeRmFyZN+m3tvTZgAzC+ N5yw== X-Gm-Message-State: AO0yUKUzhGikLKTNmiE1RBVc+RdwhU4YA8NZYoZsdgBf7SV2PKwCyWjW PGCDjl3UMsS8NAX47P35r8eEoQ== X-Google-Smtp-Source: AK7set8KFBZcJoyADEvCdNhzMHm/y5MIdXeUfrR90b/geFbtcc/C2jtV6qhmaO/gjfdwti36unuj5w== X-Received: by 2002:a17:902:f113:b0:196:7a96:cd82 with SMTP id e19-20020a170902f11300b001967a96cd82mr16617424plb.42.1675709116186; Mon, 06 Feb 2023 10:45:16 -0800 (PST) Received: from www.outflux.net (198-0-35-241-static.hfc.comcastbusiness.net. [198.0.35.241]) by smtp.gmail.com with ESMTPSA id e21-20020a170902d39500b0019601fbb963sm7225276pld.172.2023.02.06.10.45.15 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 06 Feb 2023 10:45:15 -0800 (PST) Message-ID: <63e14abb.170a0220.ca425.b7bc@mx.google.com> X-Google-Original-Message-ID: <202302061040.@keescook> Date: Mon, 6 Feb 2023 10:45:15 -0800 From: Kees Cook To: Stanislav Fomichev Cc: Alexei Starovoitov , Daniel Borkmann , Andrii Nakryiko , Martin KaFai Lau , Song Liu , Yonghong Song , John Fastabend , KP Singh , Hao Luo , Jiri Olsa , Mykola Lysenko , Shuah Khan , Haowen Bai , bpf@vger.kernel.org, linux-kselftest@vger.kernel.org, linux-kernel@vger.kernel.org, linux-hardening@vger.kernel.org Subject: Re: [PATCH] bpf: Replace bpf_lpm_trie_key 0-length array with flexible array References: <20230204183241.never.481-kees@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Feb 06, 2023 at 09:52:17AM -0800, Stanislav Fomichev wrote: > On Sat, Feb 4, 2023 at 10:32 AM Kees Cook wrote: > > > > Replace deprecated 0-length array in struct bpf_lpm_trie_key with > > flexible array. Found with GCC 13: > > > > ../kernel/bpf/lpm_trie.c:207:51: warning: array subscript i is outside array bounds of 'const __u8[0]' {aka 'const unsigned char[]'} [-Warray-bounds=] > > 207 | *(__be16 *)&key->data[i]); > > | ^~~~~~~~~~~~~ > > ../include/uapi/linux/swab.h:102:54: note: in definition of macro '__swab16' > > 102 | #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x)) > > | ^ > > ../include/linux/byteorder/generic.h:97:21: note: in expansion of macro '__be16_to_cpu' > > 97 | #define be16_to_cpu __be16_to_cpu > > | ^~~~~~~~~~~~~ > > ../kernel/bpf/lpm_trie.c:206:28: note: in expansion of macro 'be16_to_cpu' > > 206 | u16 diff = be16_to_cpu(*(__be16 *)&node->data[i] > > ^ > > | ^~~~~~~~~~~ > > In file included from ../include/linux/bpf.h:7: > > ../include/uapi/linux/bpf.h:82:17: note: while referencing 'data' > > 82 | __u8 data[0]; /* Arbitrary size */ > > | ^~~~ > > > > This includes fixing the selftest which was incorrectly using a > > variable length struct as a header, identified earlier[1]. Avoid this > > by just explicitly including the prefixlen member instead of struct > > bpf_lpm_trie_key. > > > > [1] https://lore.kernel.org/all/202206281009.4332AA33@keescook/ > > > > Cc: Alexei Starovoitov > > Cc: Daniel Borkmann > > Cc: Andrii Nakryiko > > Cc: Martin KaFai Lau > > Cc: Song Liu > > Cc: Yonghong Song > > Cc: John Fastabend > > Cc: KP Singh > > Cc: Stanislav Fomichev > > Cc: Hao Luo > > Cc: Jiri Olsa > > Cc: Mykola Lysenko > > Cc: Shuah Khan > > Cc: Haowen Bai > > Cc: bpf@vger.kernel.org > > Cc: linux-kselftest@vger.kernel.org > > Signed-off-by: Kees Cook > > --- > > include/uapi/linux/bpf.h | 2 +- > > tools/testing/selftests/bpf/progs/map_ptr_kern.c | 2 +- > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > > index ba0f0cfb5e42..5930bc5c7e2c 100644 > > --- a/include/uapi/linux/bpf.h > > +++ b/include/uapi/linux/bpf.h > > @@ -79,7 +79,7 @@ struct bpf_insn { > > /* Key of an a BPF_MAP_TYPE_LPM_TRIE entry */ > > struct bpf_lpm_trie_key { > > __u32 prefixlen; /* up to 32 for AF_INET, 128 for AF_INET6 */ > > - __u8 data[0]; /* Arbitrary size */ > > + __u8 data[]; /* Arbitrary size */ > > }; > > That's a UAPI change, can we do it? The safest option is probably just > to remove this field if it's causing any problems (and not do the > map_ptr_kern.c change below). The problem was seen because "data" is used by the kernel (see the compiler warning above). But if it can be removed, sure, that works too, and it much nicer since the resulting structs would have fixed sizes. > The usual use-case (at least that's what we do) is to define some new > struct over it: > > struct my_key { > struct bpf_lpm_trie_key prefix; > int a, b, c; > }; > > So I really doubt that the 'data' is ever touched by any programs at all.. Horrible alternative: struct my_key { union { struct bpf_lpm_trie_key trie; struct { u8 header[sizeof(struct bpf_lpm_trie_key)]; int a, b, c; }; }; }; Perhaps better might be: struct bpf_lpm_trie_key { __u32 prefixlen; /* up to 32 for AF_INET, 128 for AF_INET6 */ }; struct bpf_lpm_trie_key_raw { struct bpf_lpm_trie_key_prefix prefix; u8 data[]; }; struct my_key { struct bpf_lpm_trie_key_prefix prefix; int a, b, c; }; Thoughts? -- Kees Cook