linux-hardening - Re: [PATCH] bpf: Replace bpf_lpm_trie_key 0-length array with flexible array

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <63e5210b.630a0220.c17be.27ff@mx.google.com>
Date:   Thu, 9 Feb 2023 08:36:27 -0800
From:   Kees Cook <keescook@...omium.org>
To:     Stanislav Fomichev <sdf@...gle.com>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Martin KaFai Lau <martin.lau@...ux.dev>,
        Song Liu <song@...nel.org>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>, Hao Luo <haoluo@...gle.com>,
        Jiri Olsa <jolsa@...nel.org>, Mykola Lysenko <mykolal@...com>,
        Shuah Khan <shuah@...nel.org>,
        Haowen Bai <baihaowen@...zu.com>, bpf@...r.kernel.org,
        linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-hardening@...r.kernel.org
Subject: Re: [PATCH] bpf: Replace bpf_lpm_trie_key 0-length array with
 flexible array

On Mon, Feb 06, 2023 at 11:17:06AM -0800, Stanislav Fomichev wrote:
> 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?

For the whole struct, yup, the above sizeof()s are correct. However:

sizeof(foo->data) == 0             // when data[0]
sizeof(foo->data) == compile error // when data[]

The [0]-array GNU extension doesn't have consistent behavior, so it's
being removed from the kernel in favor of the proper C99 [] flexible
arrays, so we can enable -fstrict-flex-arrays=3 to remove all the
ambiguities with array bounds:
https://docs.kernel.org/process/deprecated.html#zero-length-and-one-element-arrays
https://people.kernel.org/kees/bounded-flexible-arrays-in-c

As a header, this kind of overlap isn't well supported. Clang already
warns, and GCC is going to be removing support for overlapping composite
structs with a flex array in the middle (and also warns under -pedantic):
https://godbolt.org/z/vWzqs41h6

I talk about dealing with these specific cases in my recent write-up
on array bounds checking -- see "Overlapping composite structure members"
in the people.kernel.org post above.

> > 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;
> > };

This approach is, perhaps, the best way to go? Besides the selftest,
what things in userspace consumes struct bpf_lpm_trie_key?

-- 
Kees Cook