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

[Yonghong Song <yonghong.song@linux.dev>]

On 10/9/26 9:08 AM, sashiko-bot@kernel.org wrote:
> 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?

We probably will be okay since for bpf program, the return value will be
narrowed based on return type.

>
> [ ... ]
>> @@ -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?

This patch just allows 16-byte aligned parameters with possible holes. The
rest of change should be the same as the original code. Here you mentioned
a 'push rax' before calling orig_call. These should be okay, 16-byte aligned
parameters may have holes but it should not damage the 'push rax' or calling
orig_call. I will see whether we can add some tests for this.