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

[Jiri Olsa <olsajiri@gmail.com>]

On Wed, Jul 20, 2022 at 07:48:20PM -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: 34d4ef5775f7("bpf: Add dynptr data slices");
> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>

LGTM

Acked-by: Jiri Olsa <jolsa@kernel.org>

jirka

> ---
>  kernel/bpf/verifier.c | 30 +++++++++++++++++-------------
>  1 file changed, 17 insertions(+), 13 deletions(-)
> 
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index c59c3df0fea6..00f9b5a77734 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -7341,6 +7341,22 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
>  			}
>  		}
>  		break;
> +	case BPF_FUNC_dynptr_data:
> +		/* Find the id of the dynptr we're tracking the reference of.
> +		 * We must do this before we reset caller saved regs.
> +		 *
> +		 * Please note as well that meta.ref_obj_id after the check_func_arg() calls doesn't
> +		 * already contain the dynptr ref obj id, since dynptrs are stored on the stack.
> +		 */
> +		for (i = 0; i < MAX_BPF_FUNC_REG_ARGS; i++) {
> +			if (arg_type_is_dynptr(fn->arg_type[i])) {
> +				if (meta.ref_obj_id) {
> +					verbose(env, "verifier internal error: multiple refcounted args in func\n");
> +					return -EFAULT;
> +				}
> +				meta.ref_obj_id = stack_slot_get_id(env, &regs[BPF_REG_1 + i]);
> +			}
> +		}
>  	}
>  
>  	if (err)
> @@ -7470,20 +7486,8 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
>  		/* For release_reference() */
>  		regs[BPF_REG_0].ref_obj_id = id;
>  	} else if (func_id == BPF_FUNC_dynptr_data) {
> -		int dynptr_id = 0, i;
> -
> -		/* Find the id of the dynptr we're acquiring a reference to */
> -		for (i = 0; i < MAX_BPF_FUNC_REG_ARGS; i++) {
> -			if (arg_type_is_dynptr(fn->arg_type[i])) {
> -				if (dynptr_id) {
> -					verbose(env, "verifier internal error: multiple dynptr args in func\n");
> -					return -EFAULT;
> -				}
> -				dynptr_id = stack_slot_get_id(env, &regs[BPF_REG_1 + i]);
> -			}
> -		}
>  		/* For release_reference() */
> -		regs[BPF_REG_0].ref_obj_id = dynptr_id;
> +		regs[BPF_REG_0].ref_obj_id = meta.ref_obj_id;
>  	}
>  
>  	do_refine_retval_range(regs, fn->ret_type, func_id, &meta);
> -- 
> 2.30.2
>