On Mon, Feb 6, 2023 at 10:45 AM Kees Cook keescook@chromium.org wrote:
On Mon, Feb 06, 2023 at 09:52:17AM -0800, Stanislav Fomichev wrote:
On Sat, Feb 4, 2023 at 10:32 AM Kees Cook keescook@chromium.org 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 ast@kernel.org Cc: Daniel Borkmann daniel@iogearbox.net Cc: Andrii Nakryiko andrii@kernel.org Cc: Martin KaFai Lau martin.lau@linux.dev Cc: Song Liu song@kernel.org Cc: Yonghong Song yhs@fb.com Cc: John Fastabend john.fastabend@gmail.com Cc: KP Singh kpsingh@kernel.org Cc: Stanislav Fomichev sdf@google.com Cc: Hao Luo haoluo@google.com Cc: Jiri Olsa jolsa@kernel.org Cc: Mykola Lysenko mykolal@fb.com Cc: Shuah Khan shuah@kernel.org Cc: Haowen Bai baihaowen@meizu.com Cc: bpf@vger.kernel.org Cc: linux-kselftest@vger.kernel.org Signed-off-by: Kees Cook keescook@chromium.org
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.
I guess I still don't understand why we need the change in map_ptr_kern.c?
Re-reading the description:
This includes fixing the selftest which was incorrectly using a variable length struct as a header, identified earlier[1].
It's my understanding that it's the intended use-case. Users are expected to use this struct as a header; at least we've been using it that way :-)
For me, both return the same: sizeof(struct { __u32 prefix; __u8 data[0]; }) sizeof(struct { __u32 prefix; __u8 data[]; })
So let's do s/data[0]/data[]/ in the UAPI only? What's wrong with using this struct as a header?
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