Re: [PATCH bpf-next v6 07/10] bpf: Enable aggregate return types up to 16 bytes

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

New issues:
- [High] Attaching a BPF trampoline to a function returning a 16-byte struct silently corrupts the return value because the trampoline fails to preserve the second return register.

Pre-existing issues:
- [Medium] Kfuncs returning structs with padding or sizes that are not a multiple of 8 may leak uninitialized kernel memory to BPF programs.
--

--- Patch [7]: [PATCH bpf-next v6 07/10] bpf: Enable aggregate return types up to 16 bytes ---
commit 5c9b0fdbdfee0f07315dc190bc0c039af9bce8bd
Author: Yonghong Song <[email protected]>

bpf: Enable aggregate return types up to 16 bytes
    
This commit relaxes the verifier and BTF distillation to accept by-value
struct or union return types up to 16 bytes. These are returned in the
R0:R2 register pair.

> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -7591,7 +7591,7 @@ int btf_distill_func_proto(struct bpf_verifier_log *log,
>  		return -EINVAL;
>  	}
>  	ret = __get_type_size(btf, func->type, &t);
> -	if (ret < 0 || btf_type_is_struct(t)) {
> +	if (ret < 0 || ret > 16) {

[Severity: High]
By relaxing this check to allow up to 16-byte returns, does this silently
break fentry and freplace programs attached to such functions?

While bpf_check_attach_target() rejects programs if they explicitly use the
trampoline retval:

bpf_check_attach_target() {
    if (tgt_info->fmodel.ret_size > 8 &&
        attach_uses_trampoline_retval(prog->expected_attach_type)) {
        ...
    }
}

It appears to still allow fentry and freplace programs to attach. When a
BPF program attaches to a kernel function returning a 16-byte struct, the
target function returns its value in RAX and RDX (on x86-64).

The BPF trampoline epilogue calls __bpf_tramp_exit(), which clobbers
caller-saved registers like RDX.

If the trampoline only restores RAX from the stack before returning, won't
RDX be left with garbage, corrupting the upper 8 bytes of the original
function's return value?

>  		bpf_log(log,
>  			"The function %s return type %s is unsupported.\n",
>  			tname, btf_type_str(t));

> [ ... ]

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c

> [ ... ]

> @@ -14008,7 +14008,7 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>  		 * otherwise a pointer field would be laundered into a scalar
>  		 * and escape provenance and reference tracking.
>  		 */
> -		if (!__btf_type_is_scalar_struct(env, desc_btf, t, 0)) {
> +		if (!btf_type_is_scalar_struct(env, desc_btf, t, 0)) {

[Severity: Medium]
This isn't a bug introduced by this patch, but since the scope of by-value
returns is expanded from 8 bytes to 16 bytes, it might be worth verifying.

In check_kfunc_call(), the verifier immediately calls mark_kfunc_ret_regs()
after this check:

check_kfunc_call() {
    if (!btf_type_is_scalar_struct(env, desc_btf, t, 0)) {
        ...
        return -EINVAL;
    }
    mark_kfunc_ret_regs(env, regs, t->size);
}

If a kfunc returns a struct containing padding bytes, the C compiler might
leave those padding bytes uninitialized. Since the verifier marks the entire
return registers as fully initialized scalars, could this allow a BPF
program to read uninitialized kernel memory via the padding bytes?

>  			verbose(env,
>  				"kernel function %s returns %s %s that is not composed of scalars\n",
>  				func_name, btf_type_str(t),

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