Re: [PATCH v1 bpf-next 13/18] bpf: Check the return values of fmod_ret programs on found targets

[Anton Protopopov <a.s.protopopov@gmail.com>]

On 26/11/06 09:17PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found no new issues
> in this patch, but noticed 1 related pre-existing issue it does not
> introduce:
> - [Critical] bpf: use-after-free in check_helper_call() [1]
> --
> 
> commit f5e3bd12bcf921a0d31fb8a40c17694ea46d4300
> Author: Anton Protopopov <a.s.protopopov@gmail.com>
> 
> bpf: Check the return values of fmod_ret programs on found targets
> 
> This commit parses the "bpf_fmodret_retval:" BTF decl tag on fmod_ret
> target functions and verifies that attached fmod_ret programs only return
> 0 or one of the specified error values, while disallowing tail calls and
> freplace extensions on such programs.
> 
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 1b58a1796a2e4..71fbb30e4d768 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -13338,6 +13338,14 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
> >  	}
> >  
> >  	if (func_id == BPF_FUNC_tail_call) {
> > +		/*
> > +		 * The program which a tail call jumps to returns for this one,
> > +		 * and a program array does not check the values it may return.
> > +		 */
> > +		if (env->prog->aux->fmodret_retvals) {
> > +			verbose(env, "tail calls are not allowed in fmod_ret programs with restricted return values\n");
> > +			return -EINVAL;
> > +		}
> >  		if (env->cur_state->curframe) {
> >  			struct bpf_verifier_state *branch;
> >  
> 
> [Severity: Critical]
> This problem wasn't introduced by this patch, but in check_helper_call(),
> can passing an RCU-protected or iterator-yielded pointer to a pointer-cast
> helper such as bpf_skc_to_tcp_sock() drop its lifetime constraints on
> regs[BPF_REG_0] [1]?
> 
> kernel/bpf/verifier.c:check_helper_call() {
> 	...
> 	if (is_ptr_cast_function(func_id) &&
> 	    find_reference_state(env->cur_state, meta.ref_obj.id)) {
> 		...
> 		regs[BPF_REG_0].type &= ~PTR_MAYBE_NULL;
> 		regs[BPF_REG_0].id = meta.ref_obj.id;
> 	}
> 	...
> }
> 
> RET_PTR_TO_BTF_ID initializes regs[BPF_REG_0] without propagating MEM_RCU or
> parent_id from the input argument, and the cast block above only runs when
> find_reference_state() finds an acquired reference.  Because RCU-protected
> and iterator-yielded pointers are tracked via MEM_RCU or parent_id rather
> than a reference state, regs[BPF_REG_0] is left as an unconstrained
> PTR_TO_BTF_ID.  When the iterator advances or bpf_rcu_read_unlock() is
> called, the parent pointer is invalidated while the cast pointer remains
> trusted, allowing a use-after-free.

Not related to this patchset.

The bug itself looks real, I think it requires a follow up,
will try to not forget to send it.

> [1] https://sashiko.dev/#/bug/linux-97cfbde4-a454-41bd-bdcf-b13a51d2adaa
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20261106205523.1765622-1-a.s.protopopov@gmail.com?part=13