Re: [PATCH bpf-next v4 5/6] bpf: Add dynptr data slices

[Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx>]

On Mon, May 09, 2022 at 03:42:56PM -0700, Joanne Koong wrote:
>  	} else if (is_acquire_function(func_id, meta.map_ptr)) {
> -		int id = acquire_reference_state(env, insn_idx);
> +		int id = 0;
> +
> +		if (is_dynptr_ref_function(func_id)) {
> +			int 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 (id) {
> +						verbose(env, "verifier internal error: more than one dynptr arg in a dynptr ref func\n");
> +						return -EFAULT;
> +					}
> +					id = stack_slot_get_id(env, &regs[BPF_REG_1 + i]);

I'm afraid this approach doesn't work.
Consider:
  struct bpf_dynptr ptr;
  u32 *data1, *data2;

  bpf_dynptr_alloc(8, 0, &ptr);
  data1 = bpf_dynptr_data(&ptr, 0, 8);
  data2 = bpf_dynptr_data(&ptr, 8, 8);
  if (data1)
     *data2 = 0; /* this will succeed, but shouldn't */

The same 'id' is being reused for data1 and data2 to make sure
that bpf_dynptr_put(&ptr); will clear data1/data2,
but data1 and data2 will look the same in mark_ptr_or_null_reg().

> +				}
> +			}
> +			if (!id) {
> +				verbose(env, "verifier internal error: no dynptr args to a dynptr ref func\n");
> +				return -EFAULT;
> +			}
> +		} else {
> +			id = acquire_reference_state(env, insn_idx);
> +			if (id < 0)
> +				return id;
> +		}
>  
> -		if (id < 0)
> -			return id;
>  		/* For mark_ptr_or_null_reg() */
>  		regs[BPF_REG_0].id = id;
>  		/* For release_reference() */
> @@ -9810,7 +9864,8 @@ static void mark_ptr_or_null_regs(struct bpf_verifier_state *vstate, u32 regno,
>  	u32 id = regs[regno].id;
>  	int i;
>  
> -	if (ref_obj_id && ref_obj_id == id && is_null)
> +	if (ref_obj_id && ref_obj_id == id && is_null &&
> +	    !is_ref_obj_id_dynptr(state, id))

This bit is avoiding doing release of dynptr's id,
because id is shared between dynptr and slice's id.

In this patch I'm not sure what is the purpose of bpf_dynptr_data()
being an acquire function. data1 and data2 are not acquiring.
They're not incrementing refcnt of dynptr.

I think normal logic of check_helper_call() that does:
        if (type_may_be_null(regs[BPF_REG_0].type))
                regs[BPF_REG_0].id = ++env->id_gen;

should be preserved.
It will give different id-s to data1 and data2 and the problem
described earlier will not exist.

The transfer of ref_obj_id from dynptr into data1 and data2 needs to happen,
but this part:
        u32 ref_obj_id = regs[regno].ref_obj_id;
        u32 id = regs[regno].id;
        int i;

        if (ref_obj_id && ref_obj_id == id && is_null)
                /* regs[regno] is in the " == NULL" branch.
                 * No one could have freed the reference state before
                 * doing the NULL check.
                 */
                WARN_ON_ONCE(release_reference_state(state, id));

should be left alone.
bpf_dynptr_put(&ptr); will release dynptr and will clear data1 and data2.
if (!data1)
   will not release dynptr, because data1->id != data1->ref_obj_id.

In other words bpf_dynptr_data() should behave like is_ptr_cast_function().
It should trasnfer ref_obj_id to R0, but should give new R0->id.
See big comment in bpf_verifier.h next to ref_obj_id.