* Re: [PATCH bpf-next v5 03/16] bpf: Refactor to handle memory and size together
[not found] ` <20260417044914.84018C19425@smtp.kernel.org>
@ 2026-04-18 16:40 ` Yonghong Song
0 siblings, 0 replies; only message in thread
From: Yonghong Song @ 2026-04-18 16:40 UTC (permalink / raw)
To: sashiko; +Cc: bpf
On 4/16/26 9:49 PM, sashiko-bot@kernel.org wrote:
> Sashiko AI review found 1 potential issue(s):
> - [High] Incomplete refactoring for stack arguments breaks verifier precision tracking, marking incorrect registers.
> --
>
> commit 7bebb6c179745d67fba0c807428d095b66738135
> Author: Yonghong Song <yonghong.song@linux.dev>
>
> bpf: Refactor to handle memory and size together
>
> This commit refactors memory and size register handling in BPF helper
> verification, passing bpf_reg_state directly to prepare for stack arguments.
>
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index 2bedaa193d54d..7a7024d94cf00 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
> [ ... ]
>> @@ -7020,15 +7020,17 @@ static int check_helper_mem_access(struct bpf_verifier_env *env, int regno,
>> /* verify arguments to helpers or kfuncs consisting of a pointer and an access
>> * size.
>> *
>> - * @regno is the register containing the access size. regno-1 is the register
>> - * containing the pointer.
>> + * @mem_regno is the register containing the pointer, mem_regno+1 is the register
>> + * containing the access size.
>> */
>> static int check_mem_size_reg(struct bpf_verifier_env *env,
>> - struct bpf_reg_state *reg, u32 regno,
>> + struct bpf_reg_state *mem_reg,
>> + struct bpf_reg_state *size_reg, u32 mem_regno,
>> enum bpf_access_type access_type,
>> bool zero_size_allowed,
>> struct bpf_call_arg_meta *meta)
>> {
>> + int size_regno = mem_regno + 1;
>> int err;
> Since this refactoring is intended to support stack arguments, does this
> hardcoded size_regno = mem_regno + 1 break verifier precision tracking?
We are fine here. In function get_kfunc_ptr_arg_type(), we have
if (argno + 1 < nargs &&
(is_kfunc_arg_mem_size(meta->btf, &args[argno + 1], ®s[regno + 1]) ||
is_kfunc_arg_const_mem_size(meta->btf, &args[argno + 1], ®s[regno + 1])))
arg_mem_size = true;
This ensures mem_size is at most at R5.
>
> If the memory pointer is passed in R5, mem_regno is 5, making size_regno 6.
>
> This would cause mark_chain_precision() to incorrectly backtrack the
> callee-saved register BPF_REG_6 instead of the stack slot containing the size
> argument.
>
> Furthermore, if mem_regno is later modified to represent a 0-based argument
> index, size_regno will evaluate to the wrong register entirely.
>
> [ ... ]
>> - err = check_helper_mem_access(env, regno - 1, reg->umax_value,
>> + err = check_helper_mem_access(env, mem_reg, mem_regno, size_reg->umax_value,
>> access_type, zero_size_allowed, meta);
>> if (!err)
>> - err = mark_chain_precision(env, regno);
>> + err = mark_chain_precision(env, size_regno);
> Does marking the wrong register leave the actual size argument imprecise,
> potentially corrupting verifier state pruning and allowing out-of-bounds
> memory accesses to bypass verification?
>
^ permalink raw reply [flat|nested] only message in thread