On Mon, Dec 6, 2021 at 3:22 PM Hao Luo <haoluo@xxxxxxxxxx> wrote:
>
> We have introduced a new type to make bpf_arg composable, by
> reserving high bits of bpf_arg to represent flags of a type.
>
> One of the flags is PTR_MAYBE_NULL which indicates a pointer
> may be NULL. When applying this flag to an arg_type, it means
> the arg can take NULL pointer. This patch switches the
> qualified arg_types to use this flag. The arg_types changed
> in this patch include:
>
> 1. ARG_PTR_TO_MAP_VALUE_OR_NULL
> 2. ARG_PTR_TO_MEM_OR_NULL
> 3. ARG_PTR_TO_CTX_OR_NULL
> 4. ARG_PTR_TO_SOCKET_OR_NULL
> 5. ARG_PTR_TO_ALLOC_MEM_OR_NULL
> 6. ARG_PTR_TO_STACK_OR_NULL
>
> This patch does not eliminate the use of these arg_types, instead
> it makes them an alias to the 'ARG_XXX | PTR_MAYBE_NULL'.
>
> Signed-off-by: Hao Luo <haoluo@xxxxxxxxxx>
> ---
> include/linux/bpf.h | 15 +++++++++------
> kernel/bpf/verifier.c | 39 ++++++++++++++-------------------------
> 2 files changed, 23 insertions(+), 31 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index d8e6f8cd78a2..b0d063972091 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -331,13 +331,11 @@ enum bpf_arg_type {
> ARG_PTR_TO_MAP_KEY, /* pointer to stack used as map key */
> ARG_PTR_TO_MAP_VALUE, /* pointer to stack used as map value */
> ARG_PTR_TO_UNINIT_MAP_VALUE, /* pointer to valid memory used to store a map value */
> - ARG_PTR_TO_MAP_VALUE_OR_NULL, /* pointer to stack used as map value or NULL */
>
> /* the following constraints used to prototype bpf_memcmp() and other
> * functions that access data on eBPF program stack
> */
> ARG_PTR_TO_MEM, /* pointer to valid memory (stack, packet, map value) */
> - ARG_PTR_TO_MEM_OR_NULL, /* pointer to valid memory or NULL */
> ARG_PTR_TO_UNINIT_MEM, /* pointer to memory does not need to be initialized,
> * helper function must fill all bytes or clear
> * them in error case.
> @@ -347,26 +345,31 @@ enum bpf_arg_type {
> ARG_CONST_SIZE_OR_ZERO, /* number of bytes accessed from memory or 0 */
>
> ARG_PTR_TO_CTX, /* pointer to context */
> - ARG_PTR_TO_CTX_OR_NULL, /* pointer to context or NULL */
> ARG_ANYTHING, /* any (initialized) argument is ok */
> ARG_PTR_TO_SPIN_LOCK, /* pointer to bpf_spin_lock */
> ARG_PTR_TO_SOCK_COMMON, /* pointer to sock_common */
> ARG_PTR_TO_INT, /* pointer to int */
> ARG_PTR_TO_LONG, /* pointer to long */
> ARG_PTR_TO_SOCKET, /* pointer to bpf_sock (fullsock) */
> - ARG_PTR_TO_SOCKET_OR_NULL, /* pointer to bpf_sock (fullsock) or NULL */
> ARG_PTR_TO_BTF_ID, /* pointer to in-kernel struct */
> 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 */
> ARG_PTR_TO_BTF_ID_SOCK_COMMON, /* pointer to in-kernel sock_common or bpf-mirrored bpf_sock */
> ARG_PTR_TO_PERCPU_BTF_ID, /* pointer to in-kernel percpu type */
> ARG_PTR_TO_FUNC, /* pointer to a bpf program function */
> - ARG_PTR_TO_STACK_OR_NULL, /* pointer to stack or NULL */
> + ARG_PTR_TO_STACK, /* pointer to stack */
> ARG_PTR_TO_CONST_STR, /* pointer to a null terminated read-only string */
> ARG_PTR_TO_TIMER, /* pointer to bpf_timer */
> __BPF_ARG_TYPE_MAX,
>
> + /* Extended arg_types. */
> + ARG_PTR_TO_MAP_VALUE_OR_NULL = PTR_MAYBE_NULL | ARG_PTR_TO_MAP_VALUE,
> + ARG_PTR_TO_MEM_OR_NULL = PTR_MAYBE_NULL | ARG_PTR_TO_MEM,
> + ARG_PTR_TO_CTX_OR_NULL = PTR_MAYBE_NULL | ARG_PTR_TO_CTX,
> + ARG_PTR_TO_SOCKET_OR_NULL = PTR_MAYBE_NULL | ARG_PTR_TO_SOCKET,
> + ARG_PTR_TO_ALLOC_MEM_OR_NULL = PTR_MAYBE_NULL | ARG_PTR_TO_ALLOC_MEM,
> + ARG_PTR_TO_STACK_OR_NULL = PTR_MAYBE_NULL | ARG_PTR_TO_STACK,
> +
> /* This must be the last entry. Its purpose is to ensure the enum is
> * wide enough to hold the higher bits reserved for bpf_type_flag.
> */
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 0763cca139a7..b8fa88266af7 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -472,14 +472,9 @@ static bool arg_type_may_be_refcounted(enum bpf_arg_type type)
> return type == ARG_PTR_TO_SOCK_COMMON;
> }
>
> -static bool arg_type_may_be_null(enum bpf_arg_type type)
> +static bool type_may_be_null(u32 type)
> {
> - return type == ARG_PTR_TO_MAP_VALUE_OR_NULL ||
> - type == ARG_PTR_TO_MEM_OR_NULL ||
> - type == ARG_PTR_TO_CTX_OR_NULL ||
> - type == ARG_PTR_TO_SOCKET_OR_NULL ||
> - type == ARG_PTR_TO_ALLOC_MEM_OR_NULL ||
> - type == ARG_PTR_TO_STACK_OR_NULL;
> + return type & PTR_MAYBE_NULL;
> }
>
> /* Determine whether the function releases some resources allocated by another
> @@ -4932,9 +4927,8 @@ static int process_timer_func(struct bpf_verifier_env *env, int regno,
>
> static bool arg_type_is_mem_ptr(enum bpf_arg_type type)
> {
> - return type == ARG_PTR_TO_MEM ||
> - type == ARG_PTR_TO_MEM_OR_NULL ||
> - type == ARG_PTR_TO_UNINIT_MEM;
> + return base_type(type) == ARG_PTR_TO_MEM ||
> + base_type(type) == ARG_PTR_TO_UNINIT_MEM;
> }
>
> static bool arg_type_is_mem_size(enum bpf_arg_type type)
> @@ -5071,31 +5065,26 @@ 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,
> - [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,
> #ifdef CONFIG_NET
> [ARG_PTR_TO_BTF_ID_SOCK_COMMON] = &btf_id_sock_common_types,
> #endif
> [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,
> [ARG_PTR_TO_PERCPU_BTF_ID] = &percpu_btf_ptr_types,
> [ARG_PTR_TO_FUNC] = &func_ptr_types,
> - [ARG_PTR_TO_STACK_OR_NULL] = &stack_ptr_types,
> + [ARG_PTR_TO_STACK] = &stack_ptr_types,
> [ARG_PTR_TO_CONST_STR] = &const_str_ptr_types,
> [ARG_PTR_TO_TIMER] = &timer_types,
> };
> @@ -5109,7 +5098,7 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno,
> const struct bpf_reg_types *compatible;
> int i, j;
>
> - compatible = compatible_reg_types[arg_type];
> + compatible = compatible_reg_types[base_type(arg_type)];
> if (!compatible) {
> verbose(env, "verifier internal error: unsupported arg type %d\n", arg_type);
> return -EFAULT;
> @@ -5190,15 +5179,14 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> return -EACCES;
> }
>
> - if (arg_type == ARG_PTR_TO_MAP_VALUE ||
> - arg_type == ARG_PTR_TO_UNINIT_MAP_VALUE ||
> - arg_type == ARG_PTR_TO_MAP_VALUE_OR_NULL) {
> + if (base_type(arg_type) == ARG_PTR_TO_MAP_VALUE ||
> + base_type(arg_type) == ARG_PTR_TO_UNINIT_MAP_VALUE) {
> err = resolve_map_arg_type(env, meta, &arg_type);
> if (err)
> return err;
> }
>
> - if (register_is_null(reg) && arg_type_may_be_null(arg_type))
> + if (register_is_null(reg) && type_may_be_null(arg_type))
> /* A NULL register has a SCALAR_VALUE type, so skip
> * type checking.
> */
> @@ -5267,10 +5255,11 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> err = check_helper_mem_access(env, regno,
> meta->map_ptr->key_size, false,
> NULL);
> - } else if (arg_type == ARG_PTR_TO_MAP_VALUE ||
> - (arg_type == ARG_PTR_TO_MAP_VALUE_OR_NULL &&
> - !register_is_null(reg)) ||
> - arg_type == ARG_PTR_TO_UNINIT_MAP_VALUE) {
> + } else if (base_type(arg_type) == ARG_PTR_TO_MAP_VALUE ||
> + base_type(arg_type) == ARG_PTR_TO_UNINIT_MAP_VALUE) {
> + if (type_may_be_null(arg_type) && register_is_null(reg))
> + return err;
> +
small nit: return 0 would make it clear that we successfully checked
everything (err is going to be zero here, but you need to scroll quite
a lot up to check this, so it's a bit annoying).
> /* bpf_map_xxx(..., map_ptr, ..., value) call:
> * check [value, value + map->value_size) validity
> */
> --
> 2.34.1.400.ga245620fadb-goog
>
