Re: [PATCH bpf-next 3/5] bpf, x86: Place trampoline arguments by the x86-64 calling convention

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] bpf, x86: stack arguments offset mismatch and misalignment with BPF_TRAMP_F_TAIL_CALL_CTX
- [Medium] bpf: info leak of uninitialized kernel stack memory due to missing garbage cleanup
--

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

bpf, x86: Place trampoline arguments by the x86-64 calling convention

This commit updates the BPF trampoline to place on-stack arguments
according to the x86-64 calling convention. It introduces a backfill
mechanism using bpf_jit_place_args() to handle arguments like __int128
that may span stack slots and create alignment holes.

> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index 083fcd6cf15b7..0fc7715e569b5 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
[ ... ]
> @@ -3472,8 +3479,35 @@ static void save_args(const struct btf_func_model *m, u8 **prog,
>  		 *
>  		 * the arg1-5,arg7 will be passed by regs, and arg6 will
>  		 * by stack.
> +		 *
> +		 * An argument aligned to 16 bytes, an __int128 or a struct
> +		 * holding one, also starts at an even stack slot, which can
> +		 * leave a hole before it. bpf_jit_place_args() worked out
> +		 * where each eightbyte is.
>  		 */
> -		if (nr_regs + arg_regs > 6) {
> +		for (j = 0; j < arg_regs; j++, slot++, bargs_off -= 8) {
> +			u8 pos = pos_of_slot[slot];
> +			u32 src;
> +
> +			if (!x86_arg_on_stack(pos)) {
[ ... ]
> +			/* copy function arguments from origin stack frame
> +			 * into current stack frame.
[ ... ]
> +			 *
> +			 * The area for the origin call mirrors the incoming
> +			 * one, hole and all; only the bpf program takes the
> +			 * arguments packed.
>  			 */
> -			for (j = 0; j < arg_regs; j++) {
> -				emit_ldx(prog, BPF_DW, BPF_REG_0, BPF_REG_FP,
> -					 nr_stack_slots * 8 + stack_args_off);
> -				if (arena_arg)
> -					emit_arena_arg_conv(prog, BPF_REG_0, nullable,
> -							    (u32)arena_base);
> -				emit_stx(prog, BPF_DW, BPF_REG_FP, BPF_REG_0,
> -					 -stack_size);
> -
> -				if (!nr_stack_slots)
> -					first_off = stack_size;
> -				stack_size -= 8;
> -				nr_stack_slots++;
> -			}
> +			off = x86_arg_stack_off(pos);
> +			doff = for_call_origin ? stack_size - off : bargs_off;
> +			emit_ldx(prog, BPF_DW, BPF_REG_0, BPF_REG_FP,
> +				 stack_args_off + off);
> +			if (arena_arg)
> +				emit_arena_arg_conv(prog, BPF_REG_0, nullable,
> +						    (u32)arena_base);
> +			emit_stx(prog, BPF_DW, BPF_REG_FP, BPF_REG_0, -doff);
> +
> +			if (!nr_stack_slots)
> +				first_off = doff;
> +			nr_stack_slots++;
>  		}
>  	}
>  
>  	clean_stack_garbage(m, prog, nr_stack_slots, first_off);
>  }

[Severity: Medium]
Does this code leak uninitialized kernel stack memory?

The x86-64 ABI allocates 8-byte stack slots for arguments, but compilers
frequently use 32-bit instructions to place 32-bit arguments, leaving the upper
4 bytes of the slot uninitialized.

Since clean_stack_garbage() explicitly returns early when nr_stack_slots != 1,
copying the full 64 bits with emit_ldx(..., BPF_DW, ...) captures this
uninitialized kernel memory into the BPF context. Can this leak kernel stack
memory to privileged BPF users if there are multiple stack arguments?

[ ... ]
> @@ -4011,8 +4014,8 @@ static int __arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *rw_im
>  	}
>  
>  	if (flags & BPF_TRAMP_F_CALL_ORIG) {
> -		restore_regs(m, &prog, regs_off);
> -		save_args(m, &prog, arg_stack_off, true, flags, 0);
> +		restore_regs(&prog, pos_of_slot, nr_regs, regs_off);
> +		save_args(m, &prog, pos_of_slot, arg_stack_off, true, flags, 0);
>  
>  		if (flags & BPF_TRAMP_F_TAIL_CALL_CTX) {
>  			/* Before calling the original function, load the

[Severity: High]
Are stack arguments misaligned here if BPF_TRAMP_F_TAIL_CALL_CTX is active?

When __arch_prepare_bpf_trampoline() executes, it checks for
BPF_TRAMP_F_TAIL_CALL_CTX and emits a push rax instruction (pushing 8 bytes)
after sub rsp, stack_size, which permanently shifts RSP down by 8 bytes.

However, the updated save_args() calculates the stack argument offsets based on
the original stack_size (doff = stack_size - off) without accounting for this
extra 8-byte shift.

Because the push rax is unpopped before call orig_call, the original function
will read its first stack argument from [RSP + 8]. Due to the 8-byte shift,
this points to the tail call counter pointer instead of the actual first stack
argument, corrupting all on-stack arguments.

Additionally, doesn't this 8-byte shift break the required 16-byte stack
alignment on function entry for x86-64?

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