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/09 01:14PM, Anton Protopopov wrote:
> 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.

(If it's actually reproducible.)

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