Re: [PATCH bpf-next v5 13/16] bpf,x86: Implement JIT support for stack arguments

[email protected] writes:

> Sashiko AI review found 4 potential issue(s):
> - [High] Data corruption of Argument 6 across function calls due to caller-saved register clobbering.
> - [High] Missing size handling for Argument 6 allows verifier bounds tracking bypass and out-of-bounds memory writes.
> - [High] The offset translation formula for outgoing arguments 7+ reverses the memory layout, corrupting sub-slot accesses.
> - [Medium] The JIT misroutes stack accesses because it rigidly assumes LDX only targets incoming arguments and STX/ST only target outgoing arguments.
> --
>
> commit 41dcf99330b6b0a3804185f2a284a76be8340ed1
> Author: Yonghong Song <[email protected]>
>
> bpf,x86: Implement JIT support for stack arguments
>
> Add x86_64 JIT support for BPF functions and kfuncs with more than
> 5 arguments. The extra arguments are passed through a stack area
> addressed by register r11 (BPF_REG_PARAMS) in BPF bytecode,
> which the JIT translates to native code.
>
>> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
>> index 32864dbc2c4e8..25b4357de023b 100644
>> --- a/arch/x86/net/bpf_jit_comp.c
>> +++ b/arch/x86/net/bpf_jit_comp.c
>
> [ ... ]
>
>> @@ -1664,16 +1692,45 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image, u8 *rw_image
>>  	stack_depth = bpf_prog->aux->stack_depth;
>> +	stack_arg_depth = bpf_prog->aux->stack_arg_depth;
>> +	incoming_stack_arg_depth = bpf_prog->aux->incoming_stack_arg_depth;
>> +	outgoing_stack_arg_depth = stack_arg_depth - incoming_stack_arg_depth;
>>  	priv_stack_ptr = bpf_prog->aux->priv_stack_ptr;
>>  	if (priv_stack_ptr) {
>>  		priv_frame_ptr = priv_stack_ptr + PRIV_STACK_GUARD_SZ + round_up(stack_depth, 8);
>>  		stack_depth = 0;
>>  	}
>>  
>> +	/*
>> +	 * Follow x86-64 calling convention for both BPF-to-BPF and
>> +	 * kfunc calls:
>> +	 *   - Arg 6 is passed in R9 register
>> +	 *   - Args 7+ are passed on the stack at [rsp]
>
> Can the use of the R9 register cause data corruption across function calls?
>
> The x86-64 C calling convention defines R9 as a caller-saved register.
> However, the BPF verifier models argument 6 as stack memory, which implies
> its value should be preserved across calls.
>
> If a BPF program reads incoming argument 6 after making a helper or kfunc
> call, will it receive a clobbered value instead of the original argument?
> Similarly, could a helper call scheduled by LLVM between the setup of
> outgoing argument 6 and the target call corrupt the R9 register?
>
> [ ... ]

This is correct analysis, I just realized that sashiko has already found
this!

>
>> +	/*
>> +	 * Base offset from rbp for translating BPF outgoing args 7+
>> +	 * to native offsets. BPF uses negative offsets from r11
>> +	 * (r11-8 for arg6, r11-16 for arg7, ...) while x86 uses
>> +	 * positive offsets from rsp ([rsp+0] for arg7, [rsp+8] for
>> +	 * arg8, ...). Arg 6 goes to R9 directly.
>> +	 *
>> +	 * The translation reverses direction:
>> +	 *   native_off = outgoing_arg_base - outgoing_rsp - bpf_off - 16
>> +	 *
>> +	 * Note that tail_call_reachable is guaranteed to be false when
>> +	 * stack args exist, so tcc pushes need not be accounted for.
>> +	 */
>
> Does this formula correctly handle multi-byte sub-slot accesses?
>
> Since the formula subtracts the BPF offset, the memory layout within the
> 8-byte slots appears mathematically reversed. While this correctly maps the
> 8-byte slot bases (e.g., -16 maps to rsp + 0), it seems to point to the
> wrong memory for partial writes.
>
> For example, if a program writes to the upper 32 bits of Arg 7 at BPF offset
> -12, the formula yields BASE - (-12) - 16 = BASE - 4. Will this map the
> access out-of-bounds below the stack pointer instead of to [rsp + 4]?
>
> [ ... ]
>
>> @@ -2129,12 +2222,26 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image, u8 *rw_image
>>  				EMIT1(0xC7);
>>  			goto st;
>>  		case BPF_ST | BPF_MEM | BPF_DW:
>> +			if (dst_reg == BPF_REG_PARAMS && insn->off == -8) {
>> +				/* Arg 6: store immediate in r9 register */
>> +				emit_mov_imm64(&prog, X86_REG_R9, imm32 >> 31, (u32)imm32);
>> +				break;
>> +			}
>>  			EMIT2(add_1mod(0x48, dst_reg), 0xC7);
>>  
>> -st:			if (is_imm8(insn->off))
>> -				EMIT2(add_1reg(0x40, dst_reg), insn->off);
>> +st:			insn_off = insn->off;
>> +			if (dst_reg == BPF_REG_PARAMS) {
>> +				/* Args 7+: reverse BPF negative offsets to
>> +				 * x86 positive rsp offsets.
>> +				 * BPF off=-16 → [rsp+0], off=-24 → [rsp+8], ...
>> +				 */
>> +				insn_off = outgoing_arg_base - outgoing_rsp - insn_off - 16;
>> +				dst_reg = BPF_REG_FP;
>> +			}
>
> Are the size constraints missing for Argument 6 here?
>
> The check for Arg 6 (insn->off == -8) is only implemented under the BPF_DW
> case. For sizes B, H, and W, execution will fall through to the st: label.
> This would incorrectly translate the access as a normal stack operation
> using the formula above, writing out-of-bounds below the stack pointer at
> [rsp - 8] instead of populating R9.
>
> Additionally, for BPF_LDX, a 1-byte load emits a full 64-bit move to R9.
> Since the upper 56 bits of R9 are undefined for 1-byte arguments, could
> they contain garbage data? This would bypass the verifier's strict value
> bounds tracking, which assumes the loaded value is strictly within [0, 255].
>
> [ ... ]
>
>> @@ -2243,6 +2360,19 @@ st:			if (is_imm8(insn->off))
>>  		case BPF_LDX | BPF_PROBE_MEMSX | BPF_H:
>>  		case BPF_LDX | BPF_PROBE_MEMSX | BPF_W:
>>  			insn_off = insn->off;
>> +			if (src_reg == BPF_REG_PARAMS) {
>> +				if (insn_off == 8) {
>> +					/* Incoming arg 6: read from r9 */
>> +					EMIT_mov(dst_reg, X86_REG_R9);
>> +					break;
>> +				}
>> +				src_reg = BPF_REG_FP;
>> +				/*
>> +				 * Incoming args 7+: native_off == bpf_off
>> +				 * (r11+16 → [rbp+16], r11+24 → [rbp+24], ...)
>> +				 * No offset adjustment needed.
>> +				 */
>> +			}
>
> Does this logic misroute stack accesses by assuming LDX always targets
> incoming arguments and STX/ST always target outgoing arguments?
>
> If a program writes an outgoing argument ([r11 - 16]) and then reads it back
> via LDX, the JIT appears to apply the incoming argument translation mapping
> the read to [rbp - 16] (the local stack) instead of [rsp] (the outgoing
> area). This would return uninitialized data.
>
> Conversely, if a program modifies an incoming argument ([r11 + 16]) using
> STX, the JIT would apply the outgoing translation formula, yielding
> BASE - 16 - 16 = BASE - 32, which writes out-of-bounds below the stack
> pointer.
>
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=13