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

On 4/16/26 9:50 PM, [email protected] wrote:
> 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 <[email protected]>
>
> 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?

Good point. Will fix.

>
>> +
>> +	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?

Same, will fix.

>
>> +
>> +	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.

Yes, this patch should be after "bpf: Support stack arguments for bpf functions
" patch.