Re: [PATCH bpf-next v5 08/11] bpf: Reject register-pair returns when the subprog BTF is unreliable

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] push_callback_call() incorrectly passes helper arguments for BTF validation, tainting callback subprograms as unreliable.
--

--- Patch [8]: [PATCH bpf-next v5 08/11] bpf: Reject register-pair returns when the subprog BTF is unreliable ---
commit c1362d6d7ccc6806c96ffe9fc78804a2b8b6f1b9
Author: Yonghong Song <[email protected]>

bpf: Reject register-pair returns when the subprog BTF is unreliable

This patch rejects return values larger than 8 bytes as soon as the
subprogram's BTF prototype is marked unreliable. It adds
subprog_ret_pair_unreliable() and uses it in check_func_call() to
prevent tracking R2 based on a discarded signature.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index b23be0fa95af4..0ffb3bed1649e 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c

[ ... ]

> @@ -9527,6 +9544,12 @@ static int check_func_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>  		return 0;
>  	}
>  
> +	if (subprog_ret_pair_unreliable(env, subprog)) {
> +		verbose(env, "Func#%d ('%s') returns >8 bytes, which requires reliable BTF\n",
> +			subprog, subprog_name(env, subprog));
> +		return -EINVAL;
> +	}

[Severity: High]
This is a pre-existing issue, but will this new check incorrectly reject
valid BPF programs because of how push_callback_call() marks subprograms
as unreliable?

Looking at push_callback_call():

    caller = state->frame[state->curframe];
    err = btf_check_subprog_call(env, subprog, caller->regs);

It passes the caller's registers (which contain the helper arguments, e.g.,
the PTR_TO_FUNC for bpf_loop) instead of the actual arguments the callback
will receive. Because these arguments don't match the callback's prototype,
btf_check_subprog_call() fails, sets the unreliable flag to true on the
subprogram, and returns -EINVAL.

push_callback_call() ignores this error, leaving the callback permanently
tainted as unreliable.

If a callback subprogram returns >8 bytes, won't it be incorrectly rejected
here if it is also called directly elsewhere in the program, since the
unreliability flag was set mistakenly?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8