linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] bpf: Replace bpf_lpm_trie_key 0-length array with flexible array
@ 2023-02-04 18:32 Kees Cook
  2023-02-06 17:52 ` Stanislav Fomichev
  0 siblings, 1 reply; 7+ messages in thread
From: Kees Cook @ 2023-02-04 18:32 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: Kees Cook, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau,
	Song Liu, Yonghong Song, John Fastabend, KP Singh,
	Stanislav Fomichev, Hao Luo, Jiri Olsa, Mykola Lysenko,
	Shuah Khan, Haowen Bai, bpf, linux-kselftest, linux-kernel,
	linux-hardening

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 */
 };
 
 struct bpf_cgroup_storage_key {
diff --git a/tools/testing/selftests/bpf/progs/map_ptr_kern.c b/tools/testing/selftests/bpf/progs/map_ptr_kern.c
index db388f593d0a..543012deb349 100644
--- a/tools/testing/selftests/bpf/progs/map_ptr_kern.c
+++ b/tools/testing/selftests/bpf/progs/map_ptr_kern.c
@@ -311,7 +311,7 @@ struct lpm_trie {
 } __attribute__((preserve_access_index));
 
 struct lpm_key {
-	struct bpf_lpm_trie_key trie_key;
+	__u32 prefixlen;
 	__u32 data;
 };
 
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] bpf: Replace bpf_lpm_trie_key 0-length array with flexible array
  2023-02-04 18:32 [PATCH] bpf: Replace bpf_lpm_trie_key 0-length array with flexible array Kees Cook
@ 2023-02-06 17:52 ` Stanislav Fomichev
  2023-02-06 18:45   ` Kees Cook
  0 siblings, 1 reply; 7+ messages in thread
From: Stanislav Fomichev @ 2023-02-06 17:52 UTC (permalink / raw)
  To: Kees Cook
  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, linux-kselftest, linux-kernel, linux-hardening

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 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..

>
>  struct bpf_cgroup_storage_key {
> diff --git a/tools/testing/selftests/bpf/progs/map_ptr_kern.c b/tools/testing/selftests/bpf/progs/map_ptr_kern.c
> index db388f593d0a..543012deb349 100644
> --- a/tools/testing/selftests/bpf/progs/map_ptr_kern.c
> +++ b/tools/testing/selftests/bpf/progs/map_ptr_kern.c
> @@ -311,7 +311,7 @@ struct lpm_trie {
>  } __attribute__((preserve_access_index));
>
>  struct lpm_key {
> -       struct bpf_lpm_trie_key trie_key;
> +       __u32 prefixlen;
>         __u32 data;
>  };
> --
> 2.34.1
>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] bpf: Replace bpf_lpm_trie_key 0-length array with flexible array
  2023-02-06 17:52 ` Stanislav Fomichev
@ 2023-02-06 18:45   ` Kees Cook
  2023-02-06 19:17     ` Stanislav Fomichev
  0 siblings, 1 reply; 7+ messages in thread
From: Kees Cook @ 2023-02-06 18:45 UTC (permalink / raw)
  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, linux-kselftest, linux-kernel, linux-hardening

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.

> 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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] bpf: Replace bpf_lpm_trie_key 0-length array with flexible array
  2023-02-06 18:45   ` Kees Cook
@ 2023-02-06 19:17     ` Stanislav Fomichev
  2023-02-09 16:36       ` Kees Cook
  0 siblings, 1 reply; 7+ messages in thread
From: Stanislav Fomichev @ 2023-02-06 19:17 UTC (permalink / raw)
  To: Kees Cook
  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, linux-kselftest, linux-kernel, linux-hardening

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] bpf: Replace bpf_lpm_trie_key 0-length array with flexible array
  2023-02-06 19:17     ` Stanislav Fomichev
@ 2023-02-09 16:36       ` Kees Cook
  2023-02-09 16:55         ` Alexei Starovoitov
  0 siblings, 1 reply; 7+ messages in thread
From: Kees Cook @ 2023-02-09 16:36 UTC (permalink / raw)
  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, linux-kselftest, linux-kernel, linux-hardening

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] bpf: Replace bpf_lpm_trie_key 0-length array with flexible array
  2023-02-09 16:36       ` Kees Cook
@ 2023-02-09 16:55         ` Alexei Starovoitov
  2023-02-09 17:48           ` Kees Cook
  0 siblings, 1 reply; 7+ messages in thread
From: Alexei Starovoitov @ 2023-02-09 16:55 UTC (permalink / raw)
  To: Kees Cook
  Cc: Stanislav Fomichev, 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, open list:KERNEL SELFTEST FRAMEWORK,
	LKML, linux-hardening

On Thu, Feb 9, 2023 at 8:36 AM Kees Cook <keescook@chromium.org> wrote:
>
> 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?

Plenty of bpf progs use it:
https://github.com/cilium/cilium/blob/master/bpf/lib/common.h#L352

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] bpf: Replace bpf_lpm_trie_key 0-length array with flexible array
  2023-02-09 16:55         ` Alexei Starovoitov
@ 2023-02-09 17:48           ` Kees Cook
  0 siblings, 0 replies; 7+ messages in thread
From: Kees Cook @ 2023-02-09 17:48 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: Stanislav Fomichev, 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, open list:KERNEL SELFTEST FRAMEWORK,
	LKML, linux-hardening

On Thu, Feb 09, 2023 at 08:55:23AM -0800, Alexei Starovoitov wrote:
> On Thu, Feb 9, 2023 at 8:36 AM Kees Cook <keescook@chromium.org> wrote:
> > This approach is, perhaps, the best way to go? Besides the selftest,
> > what things in userspace consumes struct bpf_lpm_trie_key?
> 
> Plenty of bpf progs use it:
> https://github.com/cilium/cilium/blob/master/bpf/lib/common.h#L352

Thanks for the pointer! Yeah, it seems the "data" member is not
directly used, but is expected to be there for static initializers and
offsetof() use. For example:

cilium:
        struct egress_gw_policy_key in_key = {
                .lpm_key = { 32 + 24, {} },
                .saddr   = CLIENT_IP,
                .daddr   = EXTERNAL_SVC_IP & 0Xffffff,
        };

systemd:
	ipv6_map_fd = bpf_map_new(
			BPF_MAP_TYPE_LPM_TRIE,
			offsetof(struct bpf_lpm_trie_key, data) + sizeof(uint32_t)*4,
			sizeof(uint64_t),
			...

All the others searches in Debian I could find were just copies of UAPI
headers:
https://codesearch.debian.net/search?q=struct+bpf_lpm_trie_key&literal=1&perpkg=1

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2023-02-09 17:48 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-04 18:32 [PATCH] bpf: Replace bpf_lpm_trie_key 0-length array with flexible array Kees Cook
2023-02-06 17:52 ` Stanislav Fomichev
2023-02-06 18:45   ` Kees Cook
2023-02-06 19:17     ` Stanislav Fomichev
2023-02-09 16:36       ` Kees Cook
2023-02-09 16:55         ` Alexei Starovoitov
2023-02-09 17:48           ` Kees Cook

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).