Re: [PATCH bpf-next v6 14/17] bpf,x86: Implement JIT support for stack arguments

[sashiko-bot@kernel.org]

Sashiko AI review found 4 potential issue(s):
- [Critical] Incoming Arg 6 mapped to volatile R9 is clobbered by calls, causing verifier type confusion.
- [Critical] Sub-register BPF_LDX from incoming Arg 6 incorrectly copies the full 64-bit R9 register, breaking verifier bounds tracking.
- [High] BPF_ST instructions for outgoing Arg 6 miss size checks, causing out-of-bounds stack corruption and uninitialized arguments.
- [High] Sub-register BPF_STX to outgoing Arg 6 overwrites the entire 64-bit R9 register.
--

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

    bpf,x86: Implement JIT support for stack arguments

This commit adds x86_64 JIT support for BPF functions with more than 5
arguments. The extra arguments are passed through a stack area addressed by
register r11 (BPF_REG_PARAMS), mapping Arg 6 to R9 and Args 7+ to the stack.

> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index ea9e707e8abff..06f33fef1e8e1 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
[ ... ]
> @@ -2134,12 +2227,26 @@ static int do_jit(struct bpf_verifier_env *env, struct bpf_prog *bpf_prog, int *
>  				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;

Does this code handle 32-bit, 16-bit, or 8-bit stores to Arg 6 correctly?

The check for dst_reg == BPF_REG_PARAMS && insn->off == -8 is uniquely placed
inside the BPF_ST | BPF_MEM | BPF_DW case block. If a BPF program uses a
smaller size store like BPF_W to prepare Arg 6, the code bypasses this check
and falls through to the st: label.

At the st: label, the insn_off calculation equates to RSP - 8 relative to
RBP, emitting a memory store that corrupts unallocated stack memory. In
addition, R9 is never updated, which leaves Arg 6 uninitialized for the callee.

[ ... ]
> @@ -2149,7 +2256,17 @@ st:			if (is_imm8(insn->off))
>  		case BPF_STX | BPF_MEM | BPF_B:
>  		case BPF_STX | BPF_MEM | BPF_H:
>  		case BPF_STX | BPF_MEM | BPF_W:
>  		case BPF_STX | BPF_MEM | BPF_DW:
> -			emit_stx(&prog, BPF_SIZE(insn->code), dst_reg, src_reg, insn->off);
> +			if (dst_reg == BPF_REG_PARAMS && insn->off == -8) {
> +				/* Arg 6: store register value in r9 */
> +				EMIT_mov(X86_REG_R9, src_reg);
> +				break;
> +			}

Does this full 64-bit move break semantics for partial writes?

For smaller sizes like BPF_B or BPF_W, EMIT_mov generates a full 64-bit move
that copies all 64 bits from src_reg to R9. 

BPF semantics dictate that a sub-register store should only modify the
specified lower bytes of the target location, leaving the rest unmodified. This
full copy could overwrite data previously stored in the upper bytes of Arg 6
via other partial writes.

[ ... ]
> @@ -2248,6 +2365,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;
> +				}

Can R9 be clobbered by function calls when used for Arg 6?

The verifier treats incoming stack arguments as stable memory preserved across
function calls. However, under the x86-64 System V ABI, R9 is a caller-saved
(volatile) register.

If a BPF program reads [r11+8] after making a helper, kfunc, or subprog call,
it will read the clobbered value left in R9. The verifier will incorrectly
track this as the uncorrupted argument, which could cause type confusion.

Additionally, does EMIT_mov(dst_reg, X86_REG_R9) zero-extend sub-register
loads?

This generates a full 64-bit register-to-register move regardless of the load
size. For smaller loads like BPF_B, BPF semantics require the value to be
zero-extended. 

Since the entire 64-bit R9 register is copied without zeroing the upper 56
bits, garbage left by the caller in the upper bits will leak into the
destination register, breaking the bounds tracked by the verifier.

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