Re: [PATCH v2 bpf-next 15/18] resolve_btfids: Tag fmod_ret candidates as targets

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

On 26/11/09 05:42PM, bot+bpf-ci@kernel.org wrote:
> > resolve_btfids: Tag fmod_ret candidates as targets
> 
> TL;DR: resolve_btfids may tag KF_IMPLICIT_ARGS kfuncs (e.g.
> bpf_wq_set_callback) as fmod_ret targets; the trampoline may then clobber the
> implicit argument, and the kfunc could dereference a garbage pointer.
> 
> > diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c
> > --- a/tools/bpf/resolve_btfids/main.c
> > +++ b/tools/bpf/resolve_btfids/main.c
> 
> [ ... ]
> 
> > @@ -1841,6 +1888,16 @@ static int btf2btf(struct object *obj)
> >  		}
> >  	}
> >  
> > +	for (size_t i = 0; i < obj->fmodret_retvals.cnt; i++) {
> > +		const struct fmodret_retval *retval = &obj->fmodret_retvals.retvals[i];
> > +
> > +		if (has_set_func(&ctx.fmodrets, retval->btf_id))
> > +			continue;
> > +		err = add_fmodret_retval_tags(&ctx, retval);
> > +		if (err)
> > +			goto out;
> > +	}
> > +
> 
> Can this loop tag a kfunc with KF_IMPLICIT_ARGS as an fmod_ret target?

Yes, looks like you're right, thanks. Will be fixed.

> 
> The kfunc loop earlier in btf2btf() has already rewritten the BTF
> prototype of such a kfunc through process_kfunc_with_implicit_args():
> 
> 	t = (struct btf_type *)btf__type_by_id(btf, kfunc->btf_id);
> 	t->type = new_proto_id;
> 
> so the BTF FUNC no longer lists the implicit arguments.  fmodret_collect()
> ran before btf2btf() and compared only the return type
> (btf_func_int_return_size()), never the argument count.  The only check
> here is has_set_func(&ctx.fmodrets, ...), which does not consult
> ctx.kfuncs, so the same btf_id still gets "bpf_fmodret" and
> "bpf_fmodret_retval:...".
> 
> bpf_wq_set_callback() looks like a concrete case.  It is built with the
> plugin, has an ftrace call site, and starts with a side-effect free
> error return:
> 
> 	if (flags)
> 		return -EINVAL;
> 	return __bpf_async_set_callback(async, callback_fn, aux->prog);
> 
> so it would get "bpf_fmodret_retval:-22".  The kernel then adds it to
> BTF_KFUNC_HOOK_FMODRET, and bpf_check_attach_target() accepts
> BPF_MODIFY_RETURN through btf_kfunc_is_modify_return().
> 
> The trampoline model is built from the 3-argument prototype.  On x86,
> __arch_prepare_bpf_trampoline() runs the fmod_ret programs and then calls:
> 
> 	restore_regs(&prog, pos_of_slot, nr_regs, regs_off)
> 
> with nr_regs = 3 before calling the original function.  rcx (aux) is
> caller-saved, so __bpf_prog_enter(), the program and __bpf_prog_exit()
> may have overwritten it.  If the program returns 0, which the verifier
> allows, bpf_wq_set_callback() dereferences a garbage aux->prog.
> 
> The same seems to apply to the other KF_IMPLICIT_ARGS kfuncs that return
> int (bpf_task_work_schedule_*, bpf_list_push_*, scx_bpf_*) whenever the
> analysis finds an error path for them.
> 
> Would it make sense to skip retvals whose btf_id is a kfunc with
> KF_IMPLICIT_ARGS in ctx.kfuncs, or any kfunc whose prototype was
> rewritten?  No later commit in the series changes resolve_btfids.
> 
> 
> ---
> AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
> See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
> 
> CI run summary: https://github.com/kernel-patches/bpf/actions/runs/42662799554