Re: [PATCH bpf-next v6 09/17] bpf: Track r11 registers in const_fold and liveness

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The stack arguments feature is entirely blocked because BPF_REG_PARAMS is unconditionally rejected during initial instruction sanity checks.
- [Medium] State tracking corruption due to early returns skipping register invalidation for BPF_REG_PARAMS
- [Medium] Out-of-bounds access in verifier register checks when using BPF_REG_PARAMS in unsupported instructions
--

--- Patch [9]: [PATCH bpf-next v6 09/17] bpf: Track r11 registers in const_fold and liveness ---
commit 9af64f45f038d2b8f7990714d3dc13da2d1fbc25
Author: Yonghong Song <[email protected]>

bpf: Track r11 registers in const_fold and liveness

This commit adds early returns in the register tracking arrays for
const_fold.c and liveness.c when instructions use BPF_REG_PARAMS (r11).
It aims to prevent out-of-bounds accesses and invalidates the destination
register for BPF_LDX instructions to avoid leaving stale states.

> diff --git a/kernel/bpf/const_fold.c b/kernel/bpf/const_fold.c
> index db73c4740b1e1..41e5a406432c4 100644
> --- a/kernel/bpf/const_fold.c
> +++ b/kernel/bpf/const_fold.c
> @@ -51,13 +51,23 @@ static void const_reg_xfer(struct bpf_verifier_env *env, struct const_arg_info *
>  			   struct bpf_insn *insn, struct bpf_insn *insns, int idx)
>  {
>  	struct const_arg_info unknown = { .state = CONST_ARG_UNKNOWN, .val = 0 };
> -	struct const_arg_info *dst = &ci_out[insn->dst_reg];
> -	struct const_arg_info *src = &ci_out[insn->src_reg];
> +	struct const_arg_info *dst, *src;
>  	u8 class = BPF_CLASS(insn->code);
>  	u8 mode = BPF_MODE(insn->code);
>  	u8 opcode = BPF_OP(insn->code) | BPF_SRC(insn->code);
>  	int r;
>  
> +	/* Stack arguments using BPF_REG_PARAMS are outside the tracked register set. */
> +	if (insn->dst_reg >= MAX_BPF_REG)
> +		return;
> +	if (insn->src_reg >= MAX_BPF_REG) {
> +		if (class == BPF_LDX)
> +			ci_out[insn->dst_reg] = unknown;
> +		return;
> +	}

[Severity: Medium]
If an ALU or atomic instruction uses BPF_REG_PARAMS (r11) as a source or
destination register, is it possible that this early return skips clearing the
destination register state?

If an ALU instruction uses r11, it looks like the destination register's
tracking state is not invalidated, retaining its previous, potentially stale
constant state. Similarly, an atomic store (BPF_STX) to r11 could return
early without invalidating R0 for BPF_CMPXCHG.

Could this lead to dead branch elimination errors or forged pointers?

> diff --git a/kernel/bpf/liveness.c b/kernel/bpf/liveness.c
> index 332e6e003f270..87022da94f3a0 100644
> --- a/kernel/bpf/liveness.c
> +++ b/kernel/bpf/liveness.c
> @@ -1068,11 +1068,21 @@ static void arg_track_xfer(struct bpf_verifier_env *env, struct bpf_insn *insn,
>  	int depth = instance->depth;
>  	u8 class = BPF_CLASS(insn->code);
>  	u8 code = BPF_OP(insn->code);
> -	struct arg_track *dst = &at_out[insn->dst_reg];
> -	struct arg_track *src = &at_out[insn->src_reg];
> +	struct arg_track *dst, *src;
>  	struct arg_track none = { .frame = ARG_NONE };
>  	int r;
>  
> +	/* Stack arguments using BPF_REG_PARAMS are outside the tracked register set. */
> +	if (insn->dst_reg >= MAX_BPF_REG)
> +		return;
> +	if (insn->src_reg >= MAX_BPF_REG) {
> +		if (class == BPF_LDX)
> +			at_out[insn->dst_reg] = none;
> +		return;
> +	}

[Severity: Medium]
Looking at this similar early return, could it skip updating the liveness
tracking for ALU and atomic instructions that use BPF_REG_PARAMS?

While reviewing this feature, I also noticed a couple of related issues in
kernel/bpf/verifier.c:

[Severity: Medium]
In kernel/bpf/verifier.c:__check_reg_arg():

    reg = &regs[regno];

When an ALU, JMP, or ATOMIC instruction uses BPF_REG_PARAMS (r11) as a register
operand, it appears this might access the regs array with an index of 11.
Since the verifier's regs array is sized exactly MAX_BPF_REG (11 elements,
indices 0-10), could this lead to an out-of-bounds access?


[Severity: High]
In kernel/bpf/verifier.c:check_and_resolve_insns():

    if (insn->dst_reg >= MAX_BPF_REG) {
        verbose(env, "R%d is invalid\n", insn->dst_reg);
        return -EINVAL;
    }

Is BPF_REG_PARAMS unintentionally being rejected during the initial sanity
checks here?

Since BPF_REG_PARAMS is defined as MAX_BPF_REG (11), any instruction
referencing r11 seems to be immediately rejected with "R11 is invalid" before
core verification begins. Should this bounds check be updated to accommodate
the new register?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=9