| Message ID | 20200921121227.255763-12-lmb@cloudflare.com |
|---|---|
| State | Accepted |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | Make check_func_arg type checks table driven | expand |
On Mon, Sep 21, 2020 at 01:12:27PM +0100, Lorenz Bauer wrote: > +struct bpf_reg_types { > + const enum bpf_reg_type types[10]; > +}; any idea on how to make it more robust? > + > +static const struct bpf_reg_types *compatible_reg_types[] = { > + [ARG_PTR_TO_MAP_KEY] = &map_key_value_types, > + [ARG_PTR_TO_MAP_VALUE] = &map_key_value_types, > + [ARG_PTR_TO_UNINIT_MAP_VALUE] = &map_key_value_types, > + [ARG_PTR_TO_MAP_VALUE_OR_NULL] = &map_key_value_types, > + [ARG_CONST_SIZE] = &scalar_types, > + [ARG_CONST_SIZE_OR_ZERO] = &scalar_types, > + [ARG_CONST_ALLOC_SIZE_OR_ZERO] = &scalar_types, > + [ARG_CONST_MAP_PTR] = &const_map_ptr_types, > + [ARG_PTR_TO_CTX] = &context_types, > + [ARG_PTR_TO_CTX_OR_NULL] = &context_types, > + [ARG_PTR_TO_SOCK_COMMON] = &sock_types, > + [ARG_PTR_TO_SOCKET] = &fullsock_types, > + [ARG_PTR_TO_SOCKET_OR_NULL] = &fullsock_types, > + [ARG_PTR_TO_BTF_ID] = &btf_ptr_types, > + [ARG_PTR_TO_SPIN_LOCK] = &spin_lock_types, > + [ARG_PTR_TO_MEM] = &mem_types, > + [ARG_PTR_TO_MEM_OR_NULL] = &mem_types, > + [ARG_PTR_TO_UNINIT_MEM] = &mem_types, > + [ARG_PTR_TO_ALLOC_MEM] = &alloc_mem_types, > + [ARG_PTR_TO_ALLOC_MEM_OR_NULL] = &alloc_mem_types, > + [ARG_PTR_TO_INT] = &int_ptr_types, > + [ARG_PTR_TO_LONG] = &int_ptr_types, > + [__BPF_ARG_TYPE_MAX] = NULL, I don't understand what this extra value is for. I tried: diff --git a/include/linux/bpf.h b/include/linux/bpf.h index fc5c901c7542..87b0d5dcc1ff 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -292,7 +292,6 @@ enum bpf_arg_type { ARG_PTR_TO_ALLOC_MEM, /* pointer to dynamically allocated memory */ ARG_PTR_TO_ALLOC_MEM_OR_NULL, /* pointer to dynamically allocated memory or NULL */ ARG_CONST_ALLOC_SIZE_OR_ZERO, /* number of allocated bytes requested */ - __BPF_ARG_TYPE_MAX, }; /* type of values returned from helper functions */ diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 15ab889b0a3f..83faa67858b6 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -4025,7 +4025,6 @@ static const struct bpf_reg_types *compatible_reg_types[] = { [ARG_PTR_TO_ALLOC_MEM_OR_NULL] = &alloc_mem_types, [ARG_PTR_TO_INT] = &int_ptr_types, [ARG_PTR_TO_LONG] = &int_ptr_types, - [__BPF_ARG_TYPE_MAX] = NULL, }; and everything is fine as I think it should be. > + compatible = compatible_reg_types[arg_type]; > + if (!compatible) { > + verbose(env, "verifier internal error: unsupported arg type %d\n", arg_type); > return -EFAULT; > } This check will trigger the same way when somebody adds new ARG_* and doesn't add to the table. > > + err = check_reg_type(env, regno, compatible); > + if (err) > + return err; > + > if (type == PTR_TO_BTF_ID) { > const u32 *btf_id = fn->arg_btf_id[arg]; > > @@ -4174,10 +4213,6 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg, > } > > return err; > -err_type: > - verbose(env, "R%d type=%s expected=%s\n", regno, > - reg_type_str[type], reg_type_str[expected_type]); > - return -EACCES; I'm not a fan of table driven checks. I think one explicit switch statement would have been easier to read, but I guess we can convert back to it later if table becomes too limiting. The improvement in the verifier output is important and justifies this approach. Applied to bpf-next. Thanks!
On Mon, 21 Sep 2020 at 23:23, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Mon, Sep 21, 2020 at 01:12:27PM +0100, Lorenz Bauer wrote: > > +struct bpf_reg_types { > > + const enum bpf_reg_type types[10]; > > +}; > > any idea on how to make it more robust? I kind of copied this from the bpf_iter context. I prototyped using an enum bpf_reg_type * and then terminating the array with NOT_INIT. Writing this out is more involved, and might need some macro magic to make it palatable. The current approach is a lot simpler, and I figured that the compiler will error out if we ever exceed the 10 items. > > > + > > +static const struct bpf_reg_types *compatible_reg_types[] = { > > + [ARG_PTR_TO_MAP_KEY] = &map_key_value_types, > > + [ARG_PTR_TO_MAP_VALUE] = &map_key_value_types, > > + [ARG_PTR_TO_UNINIT_MAP_VALUE] = &map_key_value_types, > > + [ARG_PTR_TO_MAP_VALUE_OR_NULL] = &map_key_value_types, > > + [ARG_CONST_SIZE] = &scalar_types, > > + [ARG_CONST_SIZE_OR_ZERO] = &scalar_types, > > + [ARG_CONST_ALLOC_SIZE_OR_ZERO] = &scalar_types, > > + [ARG_CONST_MAP_PTR] = &const_map_ptr_types, > > + [ARG_PTR_TO_CTX] = &context_types, > > + [ARG_PTR_TO_CTX_OR_NULL] = &context_types, > > + [ARG_PTR_TO_SOCK_COMMON] = &sock_types, > > + [ARG_PTR_TO_SOCKET] = &fullsock_types, > > + [ARG_PTR_TO_SOCKET_OR_NULL] = &fullsock_types, > > + [ARG_PTR_TO_BTF_ID] = &btf_ptr_types, > > + [ARG_PTR_TO_SPIN_LOCK] = &spin_lock_types, > > + [ARG_PTR_TO_MEM] = &mem_types, > > + [ARG_PTR_TO_MEM_OR_NULL] = &mem_types, > > + [ARG_PTR_TO_UNINIT_MEM] = &mem_types, > > + [ARG_PTR_TO_ALLOC_MEM] = &alloc_mem_types, > > + [ARG_PTR_TO_ALLOC_MEM_OR_NULL] = &alloc_mem_types, > > + [ARG_PTR_TO_INT] = &int_ptr_types, > > + [ARG_PTR_TO_LONG] = &int_ptr_types, > > + [__BPF_ARG_TYPE_MAX] = NULL, > > I don't understand what this extra value is for. > I tried: > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > index fc5c901c7542..87b0d5dcc1ff 100644 > --- a/include/linux/bpf.h > +++ b/include/linux/bpf.h > @@ -292,7 +292,6 @@ enum bpf_arg_type { > ARG_PTR_TO_ALLOC_MEM, /* pointer to dynamically allocated memory */ > ARG_PTR_TO_ALLOC_MEM_OR_NULL, /* pointer to dynamically allocated memory or NULL */ > ARG_CONST_ALLOC_SIZE_OR_ZERO, /* number of allocated bytes requested */ > - __BPF_ARG_TYPE_MAX, > }; > > /* type of values returned from helper functions */ > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index 15ab889b0a3f..83faa67858b6 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -4025,7 +4025,6 @@ static const struct bpf_reg_types *compatible_reg_types[] = { > [ARG_PTR_TO_ALLOC_MEM_OR_NULL] = &alloc_mem_types, > [ARG_PTR_TO_INT] = &int_ptr_types, > [ARG_PTR_TO_LONG] = &int_ptr_types, > - [__BPF_ARG_TYPE_MAX] = NULL, > }; > > and everything is fine as I think it should be. > > > + compatible = compatible_reg_types[arg_type]; > > + if (!compatible) { > > + verbose(env, "verifier internal error: unsupported arg type %d\n", arg_type); > > return -EFAULT; > > } > > This check will trigger the same way when somebody adds new ARG_* and doesn't add to the table. I think in that case that value of compatible will be undefined, since it points past the end of compatible_reg_types. Hence the __BPF_ARG_TYPE_MAX to ensure that the array has a NULL slot for new arg types. > > > > > + err = check_reg_type(env, regno, compatible); > > + if (err) > > + return err; > > + > > if (type == PTR_TO_BTF_ID) { > > const u32 *btf_id = fn->arg_btf_id[arg]; > > > > @@ -4174,10 +4213,6 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg, > > } > > > > return err; > > -err_type: > > - verbose(env, "R%d type=%s expected=%s\n", regno, > > - reg_type_str[type], reg_type_str[expected_type]); > > - return -EACCES; > > I'm not a fan of table driven checks. I think one explicit switch statement > would have been easier to read, but I guess we can convert back to it later if > table becomes too limiting. The improvement in the verifier output is important > and justifies this approach. > > Applied to bpf-next. Thanks! Thank you! -- Lorenz Bauer | Systems Engineer 6th Floor, County Hall/The Riverside Building, SE1 7PB, UK www.cloudflare.com
On Tue, Sep 22, 2020 at 09:20:27AM +0100, Lorenz Bauer wrote: > On Mon, 21 Sep 2020 at 23:23, Alexei Starovoitov > <alexei.starovoitov@gmail.com> wrote: > > > > On Mon, Sep 21, 2020 at 01:12:27PM +0100, Lorenz Bauer wrote: > > > +struct bpf_reg_types { > > > + const enum bpf_reg_type types[10]; > > > +}; > > > > any idea on how to make it more robust? > > I kind of copied this from the bpf_iter context. I prototyped using an > enum bpf_reg_type * and then terminating the array with NOT_INIT. > Writing this out is more involved, and might need some macro magic to > make it palatable. The current approach is a lot simpler, and I > figured that the compiler will error out if we ever exceed the 10 > items. The compiler will be silent if number of types is exactly 10, but at run-time the loop will access out of bounds. > > > > > + > > > +static const struct bpf_reg_types *compatible_reg_types[] = { > > > + [ARG_PTR_TO_MAP_KEY] = &map_key_value_types, > > > + [ARG_PTR_TO_MAP_VALUE] = &map_key_value_types, > > > + [ARG_PTR_TO_UNINIT_MAP_VALUE] = &map_key_value_types, > > > + [ARG_PTR_TO_MAP_VALUE_OR_NULL] = &map_key_value_types, > > > + [ARG_CONST_SIZE] = &scalar_types, > > > + [ARG_CONST_SIZE_OR_ZERO] = &scalar_types, > > > + [ARG_CONST_ALLOC_SIZE_OR_ZERO] = &scalar_types, > > > + [ARG_CONST_MAP_PTR] = &const_map_ptr_types, > > > + [ARG_PTR_TO_CTX] = &context_types, > > > + [ARG_PTR_TO_CTX_OR_NULL] = &context_types, > > > + [ARG_PTR_TO_SOCK_COMMON] = &sock_types, > > > + [ARG_PTR_TO_SOCKET] = &fullsock_types, > > > + [ARG_PTR_TO_SOCKET_OR_NULL] = &fullsock_types, > > > + [ARG_PTR_TO_BTF_ID] = &btf_ptr_types, > > > + [ARG_PTR_TO_SPIN_LOCK] = &spin_lock_types, > > > + [ARG_PTR_TO_MEM] = &mem_types, > > > + [ARG_PTR_TO_MEM_OR_NULL] = &mem_types, > > > + [ARG_PTR_TO_UNINIT_MEM] = &mem_types, > > > + [ARG_PTR_TO_ALLOC_MEM] = &alloc_mem_types, > > > + [ARG_PTR_TO_ALLOC_MEM_OR_NULL] = &alloc_mem_types, > > > + [ARG_PTR_TO_INT] = &int_ptr_types, > > > + [ARG_PTR_TO_LONG] = &int_ptr_types, > > > + [__BPF_ARG_TYPE_MAX] = NULL, > > > > I don't understand what this extra value is for. > > I tried: > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > > index fc5c901c7542..87b0d5dcc1ff 100644 > > --- a/include/linux/bpf.h > > +++ b/include/linux/bpf.h > > @@ -292,7 +292,6 @@ enum bpf_arg_type { > > ARG_PTR_TO_ALLOC_MEM, /* pointer to dynamically allocated memory */ > > ARG_PTR_TO_ALLOC_MEM_OR_NULL, /* pointer to dynamically allocated memory or NULL */ > > ARG_CONST_ALLOC_SIZE_OR_ZERO, /* number of allocated bytes requested */ > > - __BPF_ARG_TYPE_MAX, > > }; > > > > /* type of values returned from helper functions */ > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > index 15ab889b0a3f..83faa67858b6 100644 > > --- a/kernel/bpf/verifier.c > > +++ b/kernel/bpf/verifier.c > > @@ -4025,7 +4025,6 @@ static const struct bpf_reg_types *compatible_reg_types[] = { > > [ARG_PTR_TO_ALLOC_MEM_OR_NULL] = &alloc_mem_types, > > [ARG_PTR_TO_INT] = &int_ptr_types, > > [ARG_PTR_TO_LONG] = &int_ptr_types, > > - [__BPF_ARG_TYPE_MAX] = NULL, > > }; > > > > and everything is fine as I think it should be. > > > > > + compatible = compatible_reg_types[arg_type]; > > > + if (!compatible) { > > > + verbose(env, "verifier internal error: unsupported arg type %d\n", arg_type); > > > return -EFAULT; > > > } > > > > This check will trigger the same way when somebody adds new ARG_* and doesn't add to the table. > > I think in that case that value of compatible will be undefined, since > it points past the end of compatible_reg_types. Hence the > __BPF_ARG_TYPE_MAX to ensure that the array has a NULL slot for new > arg types. I still don't see a point. If anyone adds one more ARG_ to the end (or anywhere else) the compatible_reg_types array will be zero inited in that place by the compiler. Just like it does already for ARG_ANYTHING and ARG_DONTCARE. Am I still missing something? If not please follow up with removal of __BPF_ARG_TYPE_MAX.
On Tue, 22 Sep 2020 at 21:07, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Tue, Sep 22, 2020 at 09:20:27AM +0100, Lorenz Bauer wrote: > > On Mon, 21 Sep 2020 at 23:23, Alexei Starovoitov > > <alexei.starovoitov@gmail.com> wrote: > > > > > > On Mon, Sep 21, 2020 at 01:12:27PM +0100, Lorenz Bauer wrote: > > > > +struct bpf_reg_types { > > > > + const enum bpf_reg_type types[10]; > > > > +}; > > > > > > any idea on how to make it more robust? > > > > I kind of copied this from the bpf_iter context. I prototyped using an > > enum bpf_reg_type * and then terminating the array with NOT_INIT. > > Writing this out is more involved, and might need some macro magic to > > make it palatable. The current approach is a lot simpler, and I > > figured that the compiler will error out if we ever exceed the 10 > > items. > > The compiler will be silent if number of types is exactly 10, > but at run-time the loop will access out of bounds. Which loop do you refer to? The one in check_reg_type shouldn't go out of bounds due to ARRAY_SIZE: for (i = 0; i < ARRAY_SIZE(compatible->types); i++) { expected = compatible->types[i]; if (expected == NOT_INIT) break; > > > > > > > > + > > > > +static const struct bpf_reg_types *compatible_reg_types[] = { > > > > + [ARG_PTR_TO_MAP_KEY] = &map_key_value_types, > > > > + [ARG_PTR_TO_MAP_VALUE] = &map_key_value_types, > > > > + [ARG_PTR_TO_UNINIT_MAP_VALUE] = &map_key_value_types, > > > > + [ARG_PTR_TO_MAP_VALUE_OR_NULL] = &map_key_value_types, > > > > + [ARG_CONST_SIZE] = &scalar_types, > > > > + [ARG_CONST_SIZE_OR_ZERO] = &scalar_types, > > > > + [ARG_CONST_ALLOC_SIZE_OR_ZERO] = &scalar_types, > > > > + [ARG_CONST_MAP_PTR] = &const_map_ptr_types, > > > > + [ARG_PTR_TO_CTX] = &context_types, > > > > + [ARG_PTR_TO_CTX_OR_NULL] = &context_types, > > > > + [ARG_PTR_TO_SOCK_COMMON] = &sock_types, > > > > + [ARG_PTR_TO_SOCKET] = &fullsock_types, > > > > + [ARG_PTR_TO_SOCKET_OR_NULL] = &fullsock_types, > > > > + [ARG_PTR_TO_BTF_ID] = &btf_ptr_types, > > > > + [ARG_PTR_TO_SPIN_LOCK] = &spin_lock_types, > > > > + [ARG_PTR_TO_MEM] = &mem_types, > > > > + [ARG_PTR_TO_MEM_OR_NULL] = &mem_types, > > > > + [ARG_PTR_TO_UNINIT_MEM] = &mem_types, > > > > + [ARG_PTR_TO_ALLOC_MEM] = &alloc_mem_types, > > > > + [ARG_PTR_TO_ALLOC_MEM_OR_NULL] = &alloc_mem_types, > > > > + [ARG_PTR_TO_INT] = &int_ptr_types, > > > > + [ARG_PTR_TO_LONG] = &int_ptr_types, > > > > + [__BPF_ARG_TYPE_MAX] = NULL, > > > > > > I don't understand what this extra value is for. > > > I tried: > > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > > > index fc5c901c7542..87b0d5dcc1ff 100644 > > > --- a/include/linux/bpf.h > > > +++ b/include/linux/bpf.h > > > @@ -292,7 +292,6 @@ enum bpf_arg_type { > > > ARG_PTR_TO_ALLOC_MEM, /* pointer to dynamically allocated memory */ > > > ARG_PTR_TO_ALLOC_MEM_OR_NULL, /* pointer to dynamically allocated memory or NULL */ > > > ARG_CONST_ALLOC_SIZE_OR_ZERO, /* number of allocated bytes requested */ > > > - __BPF_ARG_TYPE_MAX, > > > }; > > > > > > /* type of values returned from helper functions */ > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > > index 15ab889b0a3f..83faa67858b6 100644 > > > --- a/kernel/bpf/verifier.c > > > +++ b/kernel/bpf/verifier.c > > > @@ -4025,7 +4025,6 @@ static const struct bpf_reg_types *compatible_reg_types[] = { > > > [ARG_PTR_TO_ALLOC_MEM_OR_NULL] = &alloc_mem_types, > > > [ARG_PTR_TO_INT] = &int_ptr_types, > > > [ARG_PTR_TO_LONG] = &int_ptr_types, > > > - [__BPF_ARG_TYPE_MAX] = NULL, > > > }; > > > > > > and everything is fine as I think it should be. > > > > > > > + compatible = compatible_reg_types[arg_type]; > > > > + if (!compatible) { > > > > + verbose(env, "verifier internal error: unsupported arg type %d\n", arg_type); > > > > return -EFAULT; > > > > } > > > > > > This check will trigger the same way when somebody adds new ARG_* and doesn't add to the table. > > > > I think in that case that value of compatible will be undefined, since > > it points past the end of compatible_reg_types. Hence the > > __BPF_ARG_TYPE_MAX to ensure that the array has a NULL slot for new > > arg types. > > I still don't see a point. > If anyone adds one more ARG_ to the end (or anywhere else) > the compatible_reg_types array will be zero inited in that place by the compiler. > Just like it does already for ARG_ANYTHING and ARG_DONTCARE. I looked up designated initializers when I wrote this, since I wasn't super familiar with them: https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html#Designated-Inits Note that the length of the array is the highest value specified plus one. So ARG_ANYTHING and ARG_DONTCARE are OK since there is a higher enum value present in the initializer. If someone adds a new item to enum bpf_arg_type I assume they would add it to the end. In that case the highest value of the initializer doesn't change, and then indexing into compatible_reg_types with the new enum value would be out of bounds. Adding __BPF_ARG_TYPE_MAX fixes that. It's very possible I misunderstood how this whole contraption works, happy to send a patch.
On Wed, Sep 23, 2020 at 2:45 AM Lorenz Bauer <lmb@cloudflare.com> wrote: > > On Tue, 22 Sep 2020 at 21:07, Alexei Starovoitov > <alexei.starovoitov@gmail.com> wrote: > > > > On Tue, Sep 22, 2020 at 09:20:27AM +0100, Lorenz Bauer wrote: > > > On Mon, 21 Sep 2020 at 23:23, Alexei Starovoitov > > > <alexei.starovoitov@gmail.com> wrote: > > > > > > > > On Mon, Sep 21, 2020 at 01:12:27PM +0100, Lorenz Bauer wrote: > > > > > +struct bpf_reg_types { > > > > > + const enum bpf_reg_type types[10]; > > > > > +}; > > > > > > > > any idea on how to make it more robust? > > > > > > I kind of copied this from the bpf_iter context. I prototyped using an > > > enum bpf_reg_type * and then terminating the array with NOT_INIT. > > > Writing this out is more involved, and might need some macro magic to > > > make it palatable. The current approach is a lot simpler, and I > > > figured that the compiler will error out if we ever exceed the 10 > > > items. > > > > The compiler will be silent if number of types is exactly 10, > > but at run-time the loop will access out of bounds. > > Which loop do you refer to? > > The one in check_reg_type shouldn't go out of bounds due to ARRAY_SIZE: > > for (i = 0; i < ARRAY_SIZE(compatible->types); i++) { ahh. right. it will always be 10 here. got it. > expected = compatible->types[i]; > if (expected == NOT_INIT) > break; > > > > > > > > > > > > + > > > > > +static const struct bpf_reg_types *compatible_reg_types[] = { > > > > > + [ARG_PTR_TO_MAP_KEY] = &map_key_value_types, > > > > > + [ARG_PTR_TO_MAP_VALUE] = &map_key_value_types, > > > > > + [ARG_PTR_TO_UNINIT_MAP_VALUE] = &map_key_value_types, > > > > > + [ARG_PTR_TO_MAP_VALUE_OR_NULL] = &map_key_value_types, > > > > > + [ARG_CONST_SIZE] = &scalar_types, > > > > > + [ARG_CONST_SIZE_OR_ZERO] = &scalar_types, > > > > > + [ARG_CONST_ALLOC_SIZE_OR_ZERO] = &scalar_types, > > > > > + [ARG_CONST_MAP_PTR] = &const_map_ptr_types, > > > > > + [ARG_PTR_TO_CTX] = &context_types, > > > > > + [ARG_PTR_TO_CTX_OR_NULL] = &context_types, > > > > > + [ARG_PTR_TO_SOCK_COMMON] = &sock_types, > > > > > + [ARG_PTR_TO_SOCKET] = &fullsock_types, > > > > > + [ARG_PTR_TO_SOCKET_OR_NULL] = &fullsock_types, > > > > > + [ARG_PTR_TO_BTF_ID] = &btf_ptr_types, > > > > > + [ARG_PTR_TO_SPIN_LOCK] = &spin_lock_types, > > > > > + [ARG_PTR_TO_MEM] = &mem_types, > > > > > + [ARG_PTR_TO_MEM_OR_NULL] = &mem_types, > > > > > + [ARG_PTR_TO_UNINIT_MEM] = &mem_types, > > > > > + [ARG_PTR_TO_ALLOC_MEM] = &alloc_mem_types, > > > > > + [ARG_PTR_TO_ALLOC_MEM_OR_NULL] = &alloc_mem_types, > > > > > + [ARG_PTR_TO_INT] = &int_ptr_types, > > > > > + [ARG_PTR_TO_LONG] = &int_ptr_types, > > > > > + [__BPF_ARG_TYPE_MAX] = NULL, > > > > > > > > I don't understand what this extra value is for. > > > > I tried: > > > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > > > > index fc5c901c7542..87b0d5dcc1ff 100644 > > > > --- a/include/linux/bpf.h > > > > +++ b/include/linux/bpf.h > > > > @@ -292,7 +292,6 @@ enum bpf_arg_type { > > > > ARG_PTR_TO_ALLOC_MEM, /* pointer to dynamically allocated memory */ > > > > ARG_PTR_TO_ALLOC_MEM_OR_NULL, /* pointer to dynamically allocated memory or NULL */ > > > > ARG_CONST_ALLOC_SIZE_OR_ZERO, /* number of allocated bytes requested */ > > > > - __BPF_ARG_TYPE_MAX, > > > > }; > > > > > > > > /* type of values returned from helper functions */ > > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > > > index 15ab889b0a3f..83faa67858b6 100644 > > > > --- a/kernel/bpf/verifier.c > > > > +++ b/kernel/bpf/verifier.c > > > > @@ -4025,7 +4025,6 @@ static const struct bpf_reg_types *compatible_reg_types[] = { > > > > [ARG_PTR_TO_ALLOC_MEM_OR_NULL] = &alloc_mem_types, > > > > [ARG_PTR_TO_INT] = &int_ptr_types, > > > > [ARG_PTR_TO_LONG] = &int_ptr_types, > > > > - [__BPF_ARG_TYPE_MAX] = NULL, > > > > }; > > > > > > > > and everything is fine as I think it should be. > > > > > > > > > + compatible = compatible_reg_types[arg_type]; > > > > > + if (!compatible) { > > > > > + verbose(env, "verifier internal error: unsupported arg type %d\n", arg_type); > > > > > return -EFAULT; > > > > > } > > > > > > > > This check will trigger the same way when somebody adds new ARG_* and doesn't add to the table. > > > > > > I think in that case that value of compatible will be undefined, since > > > it points past the end of compatible_reg_types. Hence the > > > __BPF_ARG_TYPE_MAX to ensure that the array has a NULL slot for new > > > arg types. > > > > I still don't see a point. > > If anyone adds one more ARG_ to the end (or anywhere else) > > the compatible_reg_types array will be zero inited in that place by the compiler. > > Just like it does already for ARG_ANYTHING and ARG_DONTCARE. > > I looked up designated initializers when I wrote this, since I wasn't > super familiar with them: > https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html#Designated-Inits > > Note that the length of the array is the highest value specified plus one. > > So ARG_ANYTHING and ARG_DONTCARE are OK since there is a higher enum > value present in the initializer. If someone adds a new item to enum > bpf_arg_type I assume they would add it to the end. In that case the > highest value of the initializer doesn't change, and then indexing > into compatible_reg_types with the new enum value would be out of > bounds. Adding __BPF_ARG_TYPE_MAX fixes that. I see. Could you do this instead then: -static const struct bpf_reg_types *compatible_reg_types[] = { +static const struct bpf_reg_types *compatible_reg_types[__BPF_ARG_TYPE_MAX] = { [ARG_PTR_TO_MAP_KEY] = &map_key_value_types, [ARG_PTR_TO_MAP_VALUE] = &map_key_value_types, [ARG_PTR_TO_UNINIT_MAP_VALUE] = &map_key_value_types, @@ -4025,7 +4025,6 @@ static const struct bpf_reg_types *compatible_reg_types[] = { [ARG_PTR_TO_ALLOC_MEM_OR_NULL] = &alloc_mem_types, [ARG_PTR_TO_INT] = &int_ptr_types, [ARG_PTR_TO_LONG] = &int_ptr_types, - [__BPF_ARG_TYPE_MAX] = NULL, }; That way is more obvious. That =NULL initializer just for the last element and not for ARG_ANYTHING/DONTCARE bothered me enough to start this whole discussion.
diff --git a/include/linux/bpf.h b/include/linux/bpf.h index 732e2f144b6d..4ca817a9195c 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -292,6 +292,7 @@ enum bpf_arg_type { ARG_PTR_TO_ALLOC_MEM, /* pointer to dynamically allocated memory */ ARG_PTR_TO_ALLOC_MEM_OR_NULL, /* pointer to dynamically allocated memory or NULL */ ARG_CONST_ALLOC_SIZE_OR_ZERO, /* number of allocated bytes requested */ + __BPF_ARG_TYPE_MAX, }; /* type of values returned from helper functions */ diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index d64ac79982ad..b5c00d08f9c0 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -3856,12 +3856,6 @@ static bool arg_type_is_mem_size(enum bpf_arg_type type) type == ARG_CONST_SIZE_OR_ZERO; } -static bool arg_type_is_alloc_mem_ptr(enum bpf_arg_type type) -{ - return type == ARG_PTR_TO_ALLOC_MEM || - type == ARG_PTR_TO_ALLOC_MEM_OR_NULL; -} - static bool arg_type_is_alloc_size(enum bpf_arg_type type) { return type == ARG_CONST_ALLOC_SIZE_OR_ZERO; @@ -3910,14 +3904,115 @@ static int resolve_map_arg_type(struct bpf_verifier_env *env, return 0; } +struct bpf_reg_types { + const enum bpf_reg_type types[10]; +}; + +static const struct bpf_reg_types map_key_value_types = { + .types = { + PTR_TO_STACK, + PTR_TO_PACKET, + PTR_TO_PACKET_META, + PTR_TO_MAP_VALUE, + }, +}; + +static const struct bpf_reg_types sock_types = { + .types = { + PTR_TO_SOCK_COMMON, + PTR_TO_SOCKET, + PTR_TO_TCP_SOCK, + PTR_TO_XDP_SOCK, + }, +}; + +static const struct bpf_reg_types mem_types = { + .types = { + PTR_TO_STACK, + PTR_TO_PACKET, + PTR_TO_PACKET_META, + PTR_TO_MAP_VALUE, + PTR_TO_MEM, + PTR_TO_RDONLY_BUF, + PTR_TO_RDWR_BUF, + }, +}; + +static const struct bpf_reg_types int_ptr_types = { + .types = { + PTR_TO_STACK, + PTR_TO_PACKET, + PTR_TO_PACKET_META, + PTR_TO_MAP_VALUE, + }, +}; + +static const struct bpf_reg_types fullsock_types = { .types = { PTR_TO_SOCKET } }; +static const struct bpf_reg_types scalar_types = { .types = { SCALAR_VALUE } }; +static const struct bpf_reg_types context_types = { .types = { PTR_TO_CTX } }; +static const struct bpf_reg_types alloc_mem_types = { .types = { PTR_TO_MEM } }; +static const struct bpf_reg_types const_map_ptr_types = { .types = { CONST_PTR_TO_MAP } }; +static const struct bpf_reg_types btf_ptr_types = { .types = { PTR_TO_BTF_ID } }; +static const struct bpf_reg_types spin_lock_types = { .types = { PTR_TO_MAP_VALUE } }; + +static const struct bpf_reg_types *compatible_reg_types[] = { + [ARG_PTR_TO_MAP_KEY] = &map_key_value_types, + [ARG_PTR_TO_MAP_VALUE] = &map_key_value_types, + [ARG_PTR_TO_UNINIT_MAP_VALUE] = &map_key_value_types, + [ARG_PTR_TO_MAP_VALUE_OR_NULL] = &map_key_value_types, + [ARG_CONST_SIZE] = &scalar_types, + [ARG_CONST_SIZE_OR_ZERO] = &scalar_types, + [ARG_CONST_ALLOC_SIZE_OR_ZERO] = &scalar_types, + [ARG_CONST_MAP_PTR] = &const_map_ptr_types, + [ARG_PTR_TO_CTX] = &context_types, + [ARG_PTR_TO_CTX_OR_NULL] = &context_types, + [ARG_PTR_TO_SOCK_COMMON] = &sock_types, + [ARG_PTR_TO_SOCKET] = &fullsock_types, + [ARG_PTR_TO_SOCKET_OR_NULL] = &fullsock_types, + [ARG_PTR_TO_BTF_ID] = &btf_ptr_types, + [ARG_PTR_TO_SPIN_LOCK] = &spin_lock_types, + [ARG_PTR_TO_MEM] = &mem_types, + [ARG_PTR_TO_MEM_OR_NULL] = &mem_types, + [ARG_PTR_TO_UNINIT_MEM] = &mem_types, + [ARG_PTR_TO_ALLOC_MEM] = &alloc_mem_types, + [ARG_PTR_TO_ALLOC_MEM_OR_NULL] = &alloc_mem_types, + [ARG_PTR_TO_INT] = &int_ptr_types, + [ARG_PTR_TO_LONG] = &int_ptr_types, + [__BPF_ARG_TYPE_MAX] = NULL, +}; + +static int check_reg_type(struct bpf_verifier_env *env, u32 regno, + const struct bpf_reg_types *compatible) +{ + struct bpf_reg_state *regs = cur_regs(env), *reg = ®s[regno]; + enum bpf_reg_type expected, type = reg->type; + int i, j; + + for (i = 0; i < ARRAY_SIZE(compatible->types); i++) { + expected = compatible->types[i]; + if (expected == NOT_INIT) + break; + + if (type == expected) + return 0; + } + + verbose(env, "R%d type=%s expected=", regno, reg_type_str[type]); + for (j = 0; j + 1 < i; j++) + verbose(env, "%s, ", reg_type_str[compatible->types[j]]); + verbose(env, "%s\n", reg_type_str[compatible->types[j]]); + return -EACCES; +} + static int check_func_arg(struct bpf_verifier_env *env, u32 arg, struct bpf_call_arg_meta *meta, const struct bpf_func_proto *fn) { u32 regno = BPF_REG_1 + arg; struct bpf_reg_state *regs = cur_regs(env), *reg = ®s[regno]; - enum bpf_reg_type expected_type, type = reg->type; enum bpf_arg_type arg_type = fn->arg_type[arg]; + const struct bpf_reg_types *compatible; + enum bpf_reg_type type = reg->type; int err = 0; if (arg_type == ARG_DONTCARE) @@ -3956,72 +4051,16 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg, */ goto skip_type_check; - if (arg_type == ARG_PTR_TO_MAP_KEY || - arg_type == ARG_PTR_TO_MAP_VALUE || - arg_type == ARG_PTR_TO_UNINIT_MAP_VALUE || - arg_type == ARG_PTR_TO_MAP_VALUE_OR_NULL) { - expected_type = PTR_TO_STACK; - if (!type_is_pkt_pointer(type) && - type != PTR_TO_MAP_VALUE && - type != expected_type) - goto err_type; - } else if (arg_type == ARG_CONST_SIZE || - arg_type == ARG_CONST_SIZE_OR_ZERO || - arg_type == ARG_CONST_ALLOC_SIZE_OR_ZERO) { - expected_type = SCALAR_VALUE; - if (type != expected_type) - goto err_type; - } else if (arg_type == ARG_CONST_MAP_PTR) { - expected_type = CONST_PTR_TO_MAP; - if (type != expected_type) - goto err_type; - } else if (arg_type == ARG_PTR_TO_CTX || - arg_type == ARG_PTR_TO_CTX_OR_NULL) { - expected_type = PTR_TO_CTX; - if (type != expected_type) - goto err_type; - } else if (arg_type == ARG_PTR_TO_SOCK_COMMON) { - expected_type = PTR_TO_SOCK_COMMON; - /* Any sk pointer can be ARG_PTR_TO_SOCK_COMMON */ - if (!type_is_sk_pointer(type)) - goto err_type; - } else if (arg_type == ARG_PTR_TO_SOCKET || - arg_type == ARG_PTR_TO_SOCKET_OR_NULL) { - expected_type = PTR_TO_SOCKET; - if (type != expected_type) - goto err_type; - } else if (arg_type == ARG_PTR_TO_BTF_ID) { - expected_type = PTR_TO_BTF_ID; - if (type != expected_type) - goto err_type; - } else if (arg_type == ARG_PTR_TO_SPIN_LOCK) { - expected_type = PTR_TO_MAP_VALUE; - if (type != expected_type) - goto err_type; - } else if (arg_type_is_mem_ptr(arg_type)) { - expected_type = PTR_TO_STACK; - if (!type_is_pkt_pointer(type) && - type != PTR_TO_MAP_VALUE && - type != PTR_TO_MEM && - type != PTR_TO_RDONLY_BUF && - type != PTR_TO_RDWR_BUF && - type != expected_type) - goto err_type; - } else if (arg_type_is_alloc_mem_ptr(arg_type)) { - expected_type = PTR_TO_MEM; - if (type != expected_type) - goto err_type; - } else if (arg_type_is_int_ptr(arg_type)) { - expected_type = PTR_TO_STACK; - if (!type_is_pkt_pointer(type) && - type != PTR_TO_MAP_VALUE && - type != expected_type) - goto err_type; - } else { - verbose(env, "unsupported arg_type %d\n", arg_type); + compatible = compatible_reg_types[arg_type]; + if (!compatible) { + verbose(env, "verifier internal error: unsupported arg type %d\n", arg_type); return -EFAULT; } + err = check_reg_type(env, regno, compatible); + if (err) + return err; + if (type == PTR_TO_BTF_ID) { const u32 *btf_id = fn->arg_btf_id[arg]; @@ -4174,10 +4213,6 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg, } return err; -err_type: - verbose(env, "R%d type=%s expected=%s\n", regno, - reg_type_str[type], reg_type_str[expected_type]); - return -EACCES; } static bool may_update_sockmap(struct bpf_verifier_env *env, int func_id)
The mapping between bpf_arg_type and bpf_reg_type is encoded in a big hairy if statement that is hard to follow. The debug output also leaves to be desired: if a reg_type doesn't match we only print one of the options, instead printing all the valid ones. Convert the if statement into a table which is then used to drive type checking. If none of the reg_types match we print all options, e.g.: R2 type=rdonly_buf expected=fp, pkt, pkt_meta, map_value Signed-off-by: Lorenz Bauer <lmb@cloudflare.com> Acked-by: Martin KaFai Lau <kafai@fb.com> --- include/linux/bpf.h | 1 + kernel/bpf/verifier.c | 183 +++++++++++++++++++++++++----------------- 2 files changed, 110 insertions(+), 74 deletions(-)