Re: [PATCH bpf-next v2 1/2] bpf: Fix ref_obj_id for dynptr data slices in verifier

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Jul 22, 2022 at 10:58:06AM -0700, Joanne Koong wrote:
> When a data slice is obtained from a dynptr (through the bpf_dynptr_data API),
> the ref obj id of the dynptr must be found and then associated with the data
> slice.
> 
> The ref obj id of the dynptr must be found *before* the caller saved regs are
> reset. Without this fix, the ref obj id tracking is not correct for
> dynptrs that are at an offset from the frame pointer.
> 
> Please also note that the data slice's ref obj id must be assigned after the
> ret types are parsed, since RET_PTR_TO_ALLOC_MEM-type return regs get
> zero-marked.
> 
> Fixes: 34d4ef5775f776ec4b0d53a02d588bf3195cada6 ("bpf: Add dynptr data slices");
> Signed-off-by: Joanne Koong <joannelkoong@xxxxxxxxx>
> ---
>  kernel/bpf/verifier.c | 62 ++++++++++++++++++++-----------------------
>  1 file changed, 29 insertions(+), 33 deletions(-)
> 
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index c59c3df0fea6..29987b2ea26f 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -5830,7 +5830,8 @@ static u32 stack_slot_get_id(struct bpf_verifier_env *env, struct bpf_reg_state
>  
>  static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
>  			  struct bpf_call_arg_meta *meta,
> -			  const struct bpf_func_proto *fn)
> +			  const struct bpf_func_proto *fn,
> +			  int func_id)
>  {
>  	u32 regno = BPF_REG_1 + arg;
>  	struct bpf_reg_state *regs = cur_regs(env), *reg = &regs[regno];
> @@ -6040,23 +6041,33 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
>  			}
>  
>  			meta->uninit_dynptr_regno = regno;
> -		} else if (!is_dynptr_reg_valid_init(env, reg, arg_type)) {
> -			const char *err_extra = "";
> +		} else {
> +			if (!is_dynptr_reg_valid_init(env, reg, arg_type)) {
> +				const char *err_extra = "";
>  
> -			switch (arg_type & DYNPTR_TYPE_FLAG_MASK) {
> -			case DYNPTR_TYPE_LOCAL:
> -				err_extra = "local ";
> -				break;
> -			case DYNPTR_TYPE_RINGBUF:
> -				err_extra = "ringbuf ";
> -				break;
> -			default:
> -				break;
> -			}
> +				switch (arg_type & DYNPTR_TYPE_FLAG_MASK) {
> +				case DYNPTR_TYPE_LOCAL:
> +					err_extra = "local ";
> +					break;
> +				case DYNPTR_TYPE_RINGBUF:
> +					err_extra = "ringbuf ";
> +					break;
> +				default:
> +					break;
> +				}
>  
> -			verbose(env, "Expected an initialized %sdynptr as arg #%d\n",
> -				err_extra, arg + 1);
> -			return -EINVAL;
> +				verbose(env, "Expected an initialized %sdynptr as arg #%d\n",
> +					err_extra, arg + 1);
> +				return -EINVAL;
> +			}
> +			if (func_id == BPF_FUNC_dynptr_data) {
> +				if (meta->ref_obj_id) {
> +					verbose(env, "verifier internal error: multiple refcounted args in BPF_FUNC_dynptr_data");
If 'func_id == BPF_FUNC_dynptr_data' is not checked first,
this verbose (or the earlier one in the 'if (reg->ref_obj_id) {...}')
may be hit for the bpf_dynptr_write helper?

Overall lgtm.

Acked-by: Martin KaFai Lau <kafai@xxxxxx>


> +					return -EFAULT;
> +				}
> +				/* Find the id of the dynptr we're tracking the reference of */
> +				meta->ref_obj_id = stack_slot_get_id(env, reg);
> +			}
>  		}
>  		break;
>  	case ARG_CONST_ALLOC_SIZE_OR_ZERO:
> @@ -7227,7 +7238,7 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
>  	meta.func_id = func_id;
>  	/* check args */
>  	for (i = 0; i < MAX_BPF_FUNC_REG_ARGS; i++) {
> -		err = check_func_arg(env, i, &meta, fn);
> +		err = check_func_arg(env, i, &meta, fn, func_id);
>  		if (err)
>  			return err;
>  	}
> @@ -7457,7 +7468,7 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
>  	if (type_may_be_null(regs[BPF_REG_0].type))
>  		regs[BPF_REG_0].id = ++env->id_gen;
>  
> -	if (is_ptr_cast_function(func_id)) {
> +	if (is_ptr_cast_function(func_id) || func_id == BPF_FUNC_dynptr_data) {
>  		/* For release_reference() */
>  		regs[BPF_REG_0].ref_obj_id = meta.ref_obj_id;
>  	} else if (is_acquire_function(func_id, meta.map_ptr)) {



[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux