On Tue, Jul 12, 2022 at 3:44 PM <sdf@xxxxxxxxxx> wrote:
>
> On 07/12, Joanne Koong wrote:
> > This patch does two things:
>
> > 1. For matching against the arg type, the match should be against the
> > base type of the arg type, since the arg type can have different
> > bpf_type_flags set on it.
>
> Does this need a fixes tag? Something around the following maybe:
>
> Fixes: d639b9d13a39 ("bpf: Introduce composable reg, ret and arg types.")
>
> ?
I will add that tag. Thanks!
>
> > 2. Uses switch casing to improve readability + efficiency.
>
> > Signed-off-by: Joanne Koong <joannelkoong@xxxxxxxxx>
> > ---
> > kernel/bpf/verifier.c | 66 +++++++++++++++++++++++++------------------
> > 1 file changed, 38 insertions(+), 28 deletions(-)
>
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 328cfab3af60..26e7e787c20a 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -5533,17 +5533,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_size(enum bpf_arg_type type)
> > -{
> > - return type == ARG_CONST_ALLOC_SIZE_OR_ZERO;
> > -}
> > -
> > -static bool arg_type_is_int_ptr(enum bpf_arg_type type)
> > -{
> > - return type == ARG_PTR_TO_INT ||
> > - type == ARG_PTR_TO_LONG;
> > -}
> > -
> > static bool arg_type_is_release(enum bpf_arg_type type)
> > {
> > return type & OBJ_RELEASE;
> > @@ -5929,7 +5918,8 @@ static int check_func_arg(struct bpf_verifier_env
> > *env, u32 arg,
> > meta->ref_obj_id = reg->ref_obj_id;
> > }
>
> > - if (arg_type == ARG_CONST_MAP_PTR) {
> > + switch (base_type(arg_type)) {
> > + case ARG_CONST_MAP_PTR:
> > /* bpf_map_xxx(map_ptr) call: remember that map_ptr */
> > if (meta->map_ptr) {
> > /* Use map_uid (which is unique id of inner map) to reject:
> > @@ -5954,7 +5944,8 @@ static int check_func_arg(struct bpf_verifier_env
> > *env, u32 arg,
> > }
> > meta->map_ptr = reg->map_ptr;
> > meta->map_uid = reg->map_uid;
> > - } else if (arg_type == ARG_PTR_TO_MAP_KEY) {
> > + break;
> > + case ARG_PTR_TO_MAP_KEY:
> > /* bpf_map_xxx(..., map_ptr, ..., key) call:
> > * check that [key, key + map->key_size) are within
> > * stack limits and initialized
> > @@ -5971,7 +5962,8 @@ 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 (base_type(arg_type) == ARG_PTR_TO_MAP_VALUE) {
> > + break;
> > + case ARG_PTR_TO_MAP_VALUE:
> > if (type_may_be_null(arg_type) && register_is_null(reg))
> > return 0;
>
> > @@ -5987,14 +5979,16 @@ static int check_func_arg(struct bpf_verifier_env
> > *env, u32 arg,
> > err = check_helper_mem_access(env, regno,
> > meta->map_ptr->value_size, false,
> > meta);
> > - } else if (arg_type == ARG_PTR_TO_PERCPU_BTF_ID) {
> > + break;
> > + case ARG_PTR_TO_PERCPU_BTF_ID:
> > if (!reg->btf_id) {
> > verbose(env, "Helper has invalid btf_id in R%d\n", regno);
> > return -EACCES;
> > }
> > meta->ret_btf = reg->btf;
> > meta->ret_btf_id = reg->btf_id;
> > - } else if (arg_type == ARG_PTR_TO_SPIN_LOCK) {
> > + break;
> > + case ARG_PTR_TO_SPIN_LOCK:
> > if (meta->func_id == BPF_FUNC_spin_lock) {
> > if (process_spin_lock(env, regno, true))
> > return -EACCES;
> > @@ -6005,12 +5999,15 @@ static int check_func_arg(struct bpf_verifier_env
> > *env, u32 arg,
> > verbose(env, "verifier internal error\n");
> > return -EFAULT;
> > }
> > - } else if (arg_type == ARG_PTR_TO_TIMER) {
> > + break;
> > + case ARG_PTR_TO_TIMER:
> > if (process_timer_func(env, regno, meta))
> > return -EACCES;
> > - } else if (arg_type == ARG_PTR_TO_FUNC) {
> > + break;
> > + case ARG_PTR_TO_FUNC:
> > meta->subprogno = reg->subprogno;
> > - } else if (base_type(arg_type) == ARG_PTR_TO_MEM) {
> > + break;
> > + case ARG_PTR_TO_MEM:
> > /* The access to this pointer is only checked when we hit the
> > * next is_mem_size argument below.
> > */
> > @@ -6020,11 +6017,14 @@ static int check_func_arg(struct bpf_verifier_env
> > *env, u32 arg,
> > fn->arg_size[arg], false,
> > meta);
> > }
> > - } else if (arg_type_is_mem_size(arg_type)) {
> > - bool zero_size_allowed = (arg_type == ARG_CONST_SIZE_OR_ZERO);
> > -
> > - err = check_mem_size_reg(env, reg, regno, zero_size_allowed, meta);
> > - } else if (arg_type_is_dynptr(arg_type)) {
> > + break;
> > + case ARG_CONST_SIZE:
> > + err = check_mem_size_reg(env, reg, regno, false, meta);
> > + break;
> > + case ARG_CONST_SIZE_OR_ZERO:
> > + err = check_mem_size_reg(env, reg, regno, true, meta);
> > + break;
> > + case ARG_PTR_TO_DYNPTR:
> > if (arg_type & MEM_UNINIT) {
> > if (!is_dynptr_reg_valid_uninit(env, reg)) {
> > verbose(env, "Dynptr has to be an uninitialized dynptr\n");
> > @@ -6058,21 +6058,28 @@ static int check_func_arg(struct bpf_verifier_env
> > *env, u32 arg,
> > err_extra, arg + 1);
> > return -EINVAL;
> > }
> > - } else if (arg_type_is_alloc_size(arg_type)) {
> > + break;
> > + case ARG_CONST_ALLOC_SIZE_OR_ZERO:
> > if (!tnum_is_const(reg->var_off)) {
> > verbose(env, "R%d is not a known constant'\n",
> > regno);
> > return -EACCES;
> > }
> > meta->mem_size = reg->var_off.value;
> > - } else if (arg_type_is_int_ptr(arg_type)) {
> > + break;
> > + case ARG_PTR_TO_INT:
> > + case ARG_PTR_TO_LONG:
> > + {
> > int size = int_ptr_type_to_size(arg_type);
>
> > err = check_helper_mem_access(env, regno, size, false, meta);
> > if (err)
> > return err;
> > err = check_ptr_alignment(env, reg, 0, size, true);
> > - } else if (arg_type == ARG_PTR_TO_CONST_STR) {
> > + break;
> > + }
> > + case ARG_PTR_TO_CONST_STR:
> > + {
> > struct bpf_map *map = reg->map_ptr;
> > int map_off;
> > u64 map_addr;
> > @@ -6111,9 +6118,12 @@ static int check_func_arg(struct bpf_verifier_env
> > *env, u32 arg,
> > verbose(env, "string is not zero-terminated\n");
> > return -EINVAL;
> > }
> > - } else if (arg_type == ARG_PTR_TO_KPTR) {
> > + break;
> > + }
> > + case ARG_PTR_TO_KPTR:
> > if (process_kptr_func(env, regno, meta))
> > return -EACCES;
> > + break;
> > }
>
> > return err;
> > --
> > 2.30.2
>
