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.
[...]
