Re: [PATCH bpf-next v2 03/14] bpf: Allow struct_ops prog to return referenced kptr

[Eduard Zingerman <eddyz87@gmail.com>]

On Thu, 2025-01-23 at 01:57 -0800, Eduard Zingerman wrote:

[...]

> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 26305571e377..0e6a3c4daa7d 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -10707,6 +10707,8 @@ record_func_key(struct bpf_verifier_env *env, struct bpf_call_arg_meta *meta,
> >  static int check_reference_leak(struct bpf_verifier_env *env, bool exception_exit)
> >  {
> >  	struct bpf_verifier_state *state = env->cur_state;
> > +	enum bpf_prog_type type = resolve_prog_type(env->prog);
> > +	struct bpf_reg_state *reg = reg_state(env, BPF_REG_0);
> >  	bool refs_lingering = false;
> >  	int i;
> >  
> > @@ -10716,6 +10718,12 @@ static int check_reference_leak(struct bpf_verifier_env *env, bool exception_exi
> >  	for (i = 0; i < state->acquired_refs; i++) {
> >  		if (state->refs[i].type != REF_TYPE_PTR)
> >  			continue;
> > +		/* Allow struct_ops programs to return a referenced kptr back to
> > +		 * kernel. Type checks are performed later in check_return_code.
> > +		 */
> > +		if (type == BPF_PROG_TYPE_STRUCT_OPS && !exception_exit &&
> > +		    reg->ref_obj_id == state->refs[i].id)
> > +			continue;
> >  		verbose(env, "Unreleased reference id=%d alloc_insn=%d\n",
> >  			state->refs[i].id, state->refs[i].insn_idx);
> >  		refs_lingering = true;
> > @@ -16320,13 +16328,14 @@ static int check_return_code(struct bpf_verifier_env *env, int regno, const char
> >  	const char *exit_ctx = "At program exit";
> >  	struct tnum enforce_attach_type_range = tnum_unknown;
> >  	const struct bpf_prog *prog = env->prog;
> > -	struct bpf_reg_state *reg;
> > +	struct bpf_reg_state *reg = reg_state(env, regno);
> >  	struct bpf_retval_range range = retval_range(0, 1);
> >  	enum bpf_prog_type prog_type = resolve_prog_type(env->prog);
> >  	int err;
> >  	struct bpf_func_state *frame = env->cur_state->frame[0];
> >  	const bool is_subprog = frame->subprogno;
> >  	bool return_32bit = false;
> > +	const struct btf_type *reg_type, *ret_type = NULL;
> >  
> >  	/* LSM and struct_ops func-ptr's return type could be "void" */
> >  	if (!is_subprog || frame->in_exception_callback_fn) {
> > @@ -16335,10 +16344,26 @@ static int check_return_code(struct bpf_verifier_env *env, int regno, const char
> >  			if (prog->expected_attach_type == BPF_LSM_CGROUP)
> >  				/* See below, can be 0 or 0-1 depending on hook. */
> >  				break;
> > -			fallthrough;
> > +			if (!prog->aux->attach_func_proto->type)
> > +				return 0;
> > +			break;
> >  		case BPF_PROG_TYPE_STRUCT_OPS:
> >  			if (!prog->aux->attach_func_proto->type)
> >  				return 0;
> > +
> > +			if (frame->in_exception_callback_fn)
> > +				break;
> > +
> > +			/* Allow a struct_ops program to return a referenced kptr if it
> > +			 * matches the operator's return type and is in its unmodified
> > +			 * form. A scalar zero (i.e., a null pointer) is also allowed.
> > +			 */
> > +			reg_type = reg->btf ? btf_type_by_id(reg->btf, reg->btf_id) : NULL;
> > +			ret_type = btf_type_resolve_ptr(prog->aux->attach_btf,
> > +							prog->aux->attach_func_proto->type,
> > +							NULL);
> 
> This does not enforce the kernel provenance of the pointer.
> See my comment for the next patch for an example.
> 
> I think such return should only be allowed for parameters marked with
> __ref suffix. If so, pointer provenance check would just compare
> reg->ref_obj_id value with known ids of __ref arguments.

After looking at the selftests in patch #13, it appears that I
misunderstood the line:

"2) The pointer originally comes from the kernel (not locally allocated)"

And the only thing you'd like to exclude here is 'bpf_obj_new'.
In which case my comments for patches #3,4 are invalid.
Sorry for the noise.

[...]