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@xxxxxxxxx>
> ---
> 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, ®s[BPF_REG_1 + i]);
check_func_arg() is setting meta->ref_obj_id for each arg.
Can this meta.ref_obj_id assignment be done in check_func_arg()
instead of looping all args again here.
> + }
> + }
> }
>
> 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, ®s[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;
nit. This will be the same as the earlier is_ptr_cast_function().
Merge the if statements ?
> }
>
> do_refine_retval_range(regs, fn->ret_type, func_id, &meta);
> --
> 2.30.2
>
