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

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 3005 bytes --]

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

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