Re: [PATCH bpf-next v5 06/16] bpf: Limit the scope of BPF_REG_PARAMS usage

[sashiko-bot@kernel.org]

Sashiko AI review found 2 potential issue(s):
- [Critical] Early returns in dataflow tracking passes skip necessary state updates for the destination register during BPF_LDX using BPF_REG_PARAMS.
- [Critical] Permitting BPF_REG_PARAMS causes severe out-of-bounds memory accesses to the `regs` array during the main verifier loop (`do_check_main`).
--

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

bpf: Limit the scope of BPF_REG_PARAMS usage

This commit restricts the usage of BPF_REG_PARAMS (r11) to specific memory
load and store instruction forms. It also adds early returns in dataflow
tracking passes to prevent out-of-bounds accesses for stack arguments.

> diff --git a/kernel/bpf/const_fold.c b/kernel/bpf/const_fold.c
> index db73c4740b1e1..09db7fdb370fd 100644
> --- a/kernel/bpf/const_fold.c
> +++ b/kernel/bpf/const_fold.c

[ ... ]

> @@ -51,13 +51,18 @@ 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 || insn->src_reg >= MAX_BPF_REG)
> +		return;

If we return early here during a BPF_LDX instruction where src_reg is
BPF_REG_PARAMS (11), doesn't this skip necessary state updates for dst_reg
(e.g., registers 0 through 9)? 

Since dst_reg is not marked as unknown, won't it retain its stale tracking
state? Could bpf_prune_dead_branches() rely on this stale constant to
incorrectly rewrite conditional jumps, allowing verifier safety checks to be
bypassed?

> +
> +	dst = &ci_out[insn->dst_reg];
> +	src = &ci_out[insn->src_reg];

[ ... ]

> diff --git a/kernel/bpf/liveness.c b/kernel/bpf/liveness.c
> index 332e6e003f270..0c7eef3385513 100644
> --- a/kernel/bpf/liveness.c
> +++ b/kernel/bpf/liveness.c
> @@ -1068,11 +1068,16 @@ 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 || insn->src_reg >= MAX_BPF_REG)
> +		return;

Similarly, does this early return skip clearing the arg_track state for the
destination register when loading from a stack argument?

> +
> +	dst = &at_out[insn->dst_reg];
> +	src = &at_out[insn->src_reg];

[ ... ]

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index ff0c55d803117..f25a56cfabaca 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -18487,13 +18487,22 @@ static int check_and_resolve_insns(struct bpf_verifier_env *env)
>  		return err;
>  
>  	for (i = 0; i < insn_cnt; i++, insn++) {
> +		u8 class = BPF_CLASS(insn->code);
> +		u8 mode = BPF_MODE(insn->code);
> +
>  		if (insn->dst_reg >= MAX_BPF_REG) {
> -			verbose(env, "R%d is invalid\n", insn->dst_reg);
> -			return -EINVAL;
> +			if (insn->dst_reg != BPF_REG_PARAMS ||
> +			    !((class == BPF_ST  || class == BPF_STX) && mode == BPF_MEM)) {
> +				verbose(env, "R%d is invalid\n", insn->dst_reg);
> +				return -EINVAL;
> +			}
>  		}

By permitting BPF_REG_PARAMS (which equals MAX_BPF_REG) to pass this validation
check, won't this cause out-of-bounds memory accesses to the regs array
(e.g., struct bpf_reg_state regs[MAX_BPF_REG]) during the main verifier loop?

When do_check_main() processes these instructions, functions like
check_reg_arg() will attempt to access regs[11], which overlays verifier
internal state.

I note this regression is fixed later in the patch series in commit
6ec4a3fcb5e83 ("bpf: Support stack arguments for bpf functions"), but it
appears to introduce an issue in this intermediate patch.

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