Re: [PATCH] bpf, x86: Sign-extend narrow signed kfunc returns

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

Pre-existing issues:
- [High] The sign-extension fix for kfunc returns is implemented for x86-64 but omitted for other architectures (ARM64, PowerPC64, x86-32) where identical verifier/runtime divergence exists.
--

--- Patch [1]: [PATCH] bpf, x86: Sign-extend narrow signed kfunc returns ---
commit 9b3fa67779a40b052a3a12d689e1b9cac8ac02a9
Author: Jérémy Jean <[email protected]>

bpf, x86: Sign-extend narrow signed kfunc returns

This patch fixes an issue on x86-64 where narrow signed kfunc return values
are not properly sign-extended to 64 bits. This divergence between verifier
assumptions and runtime execution can lead to state corruption and unbalanced
locks. The JIT is updated to use the kfunc's BTF model to sign-extend these
returns into R0 after the native call.

> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index 2853e87797a78..ec8a7694adb62 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c

[ ... ]

> @@ -2665,6 +2682,11 @@ st:			insn_off = insn->off;
>  			ip += x86_call_depth_emit_accounting(&prog, func, ip);
>  			if (emit_call(&prog, func, ip))
>  				return -EINVAL;
> +			if (src_reg == BPF_PSEUDO_KFUNC_CALL) {
> +				err = emit_kfunc_return(bpf_prog, insn, &prog);
> +				if (err)
> +					return err;
> +			}

[Severity: High]
This isn't a bug introduced by this patch, but it appears this same issue
is present in other architectures where the native calling convention leaves
the upper 32 bits undefined for 32-bit returns.

For example, in the ARM64 JIT:

arch/arm64/net/bpf_jit_comp.c:build_insn() {
    ...
    if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL) {
        ret = emit_kfunc_arena_args(ctx, insn);
        if (ret < 0)
            return ret;
    }
    emit_call(func_addr, ctx);
    ...
}

The ARM64 JIT lacks post-call return value sign-extension, leaving the upper
32 bits of X0 undefined per the AAPCS64.

Similarly, in PowerPC64:

arch/powerpc/net/bpf_jit_comp64.c:bpf_jit_build_body() {
    ...
    ret = bpf_jit_emit_func_call_rel(image, fimage, ctx, func_addr);
    if (ret)
        return ret;
    /* move return value from r3 to BPF_REG_0 */
    EMIT(PPC_RAW_MR(bpf_to_ppc(BPF_REG_0), _R3));
    ...
}

And on x86-32, the upper 32 bits of BPF_REG_0 are left uninitialized
on the stack for 32-bit and narrower returns, leaving stale data from
before the call:

arch/x86/net/bpf_jit_comp32.c:emit_kfunc_call() {
    ...
    if (fm->ret_size > sizeof(u32))
        /* mov dword ptr [ebp+off],edx */
        EMIT3(0x89, add_2reg(0x40, IA32_EBP, IA32_EDX),
              STACK_VAR(bpf2ia32[BPF_REG_0][1]));
    ...
}

Since a BPF program calling a kfunc that returns a narrow signed type
(like bpf_res_spin_lock) would also experience the verifier/runtime divergence
on these architectures, does this sign-extension fix need to be applied
there as well to prevent state corruption?

>  			if (priv_frame_ptr)
>  				pop_r9(&prog);
>  			break;

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