Re: [PATCH bpf-next v3 11/15] bpf, arm64: Move kfunc arguments into the arm64 calling convention

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The ARM64 BPF trampoline incorrectly saves and restores 16-byte aligned arguments, violating AAPCS64 alignment rules.
--

commit cd40aa31f4bf1158416008f7fcb9c0b7717dda3f
Author: Yonghong Song <yonghong.song@linux.dev>

bpf, arm64: Move kfunc arguments into the arm64 calling convention

Do the proper move from the BPF calling convention to the arm64 calling
convention to satisfy the native requirement. AAPCS64 only ever moves an
argument to a higher slot, so the moves need one scratch register to carry
an eightbyte from one stack slot to another, and never the one a
convention moving an argument down would need.

> diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
> index 3aa3ea0bc30bb..bbde7c0836ae5 100644
> --- a/arch/arm64/net/bpf_jit_comp.c
> +++ b/arch/arm64/net/bpf_jit_comp.c

[ ... ]

> @@ -2534,7 +2598,13 @@ struct arg_aux {
>  static int calc_arg_aux(const struct btf_func_model *m,
>  			 struct arg_aux *a)
>  {
> -	int stack_slots, nregs, slots, i;
> +	int stack_slots, nregs, slots, i, total;
> +
> +	/* arm64 supports up to MAX_BPF_FUNC_ARGS argument slots */
> +	for (i = 0, total = 0; i < m->nr_args; i++)
> +		total += (m->arg_size[i] + 7) / 8;
> +	if (total > MAX_BPF_FUNC_ARGS)
> +		return -ENOTSUPP;
>  
>  	/* verifier ensures m->nr_args <= MAX_BPF_FUNC_ARGS */
>  	for (i = 0, nregs = 0; i < m->nr_args; i++) {

[Severity: High]
This isn't a bug introduced by this patch, but since the series introduces
support for by-value 16-byte arguments, this previously dormant ABI violation
in the BPF trampoline appears to now be actively reachable.

Does calc_arg_aux() fail to account for skipped registers due to alignment
requirements?

If a user attaches a BPF program to a kernel function (or struct_ops
callback) that takes a 16-byte aligned argument (e.g., __int128 or
by-value struct) preceded by an odd number of register-sized arguments,
the native AAPCS64 caller places the 16-byte argument in an even/odd
register pair (like x2 and x3, skipping x1).

However, the nregs calculation here simply increments by slots without
checking for skipped odd registers:

arch/arm64/net/bpf_jit_comp.c:calc_arg_aux() {
    ...
    for (i = 0, nregs = 0; i < m->nr_args; i++) {
        slots = (m->arg_size[i] + 7) / 8;
        if (nregs + slots <= 8) /* passed through register ? */
            nregs += slots;
    ...
}

This strictly sequential register handling seems to propagate to both
save_args() and restore_args() in the trampoline, where arguments are
read and written sequentially without skipping odd registers:

arch/arm64/net/bpf_jit_comp.c:save_args() {
    ...
    slots = (m->arg_size[i] + 7) / 8;
    while (slots-- > 0) {
        ...
        emit(A64_STR64I(reg, A64_SP, bargs_off), ctx);
        reg++;
        bargs_off += 8;
    }
    ...
}

arch/arm64/net/bpf_jit_comp.c:restore_args() {
    ...
    for (reg = 0; reg < nregs; reg++) {
        emit(A64_LDR64I(reg, A64_SP, bargs_off), ctx);
        bargs_off += 8;
    }
    ...
}

Could this cause data corruption for arguments passed to BPF programs
attached via fentry, fexit, or struct_ops when 16-byte arguments are
involved?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911154914.2004336-1-yonghong.song@linux.dev?part=11