Re: [PATCH bpf-next v3 1/2] bpf, verifier: fold reg->var_off into PTR_TO_FLOW_KEYS bounds check

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The patch introduces undeclared identifiers (`argno_t`, `argno`, and `reg_arg_name()`), which will cause a fatal compilation error.
--

commit c76d52d074593085f56551ce421dd163632b1f9d
Author: Nuoqi Gui <gnq25@mails.tsinghua.edu.cn>

bpf, verifier: fold reg->var_off into PTR_TO_FLOW_KEYS bounds check

This patch modifies check_flow_keys_access() to fold the constant
reg->var_off.value into the offset before bounds checking, mirroring the
behavior of other pointer types. It also rejects non-constant variable
offsets to prevent out-of-bounds memory accesses.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 7fb88e1cd7c4d..28b83a91720c5 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -4997,9 +4997,21 @@ static int check_ctx_access(struct bpf_verifier_env *env, int insn_idx, u32 regn
>  	return err;
>  }
>  
> -static int check_flow_keys_access(struct bpf_verifier_env *env, int off,
> -				  int size)
> +static int check_flow_keys_access(struct bpf_verifier_env *env,
> +				  struct bpf_reg_state *reg, argno_t argno,
> +				  int off, int size)
>  {

[Severity: Low]
Is argno_t a valid type here? I couldn't find a definition for it in the
tree, so this looks like it might cause a build failure.

> +	/* Only a constant offset is allowed here; fold it into off. */
> +	if (!tnum_is_const(reg->var_off)) {
> +		char tn_buf[48];
> +
> +		tnum_strn(tn_buf, sizeof(tn_buf), reg->var_off);
> +		verbose(env, "%s invalid variable offset to flow keys: off=%d, var_off=%s\n",
> +			reg_arg_name(env, argno), off, tn_buf);

[Severity: Low]
Does the reg_arg_name() function exist? This seems to reference an undeclared
function which could also prevent the kernel from compiling.

> +		return -EACCES;
> +	}
> +	off += reg->var_off.value;
> +
>  	if (size < 0 || off < 0 ||
>  	    (u64)off + size > sizeof(struct bpf_flow_keys)) {
>  		verbose(env, "invalid access to flow keys off=%d size=%d\n",

[ ... ]

> @@ -6514,7 +6526,7 @@ static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, u32 regn
>  			return -EACCES;
>  		}
>  
> -		err = check_flow_keys_access(env, off, size);
> +		err = check_flow_keys_access(env, reg, argno, off, size);

[Severity: Low]
Is argno declared in the scope of check_mem_access()? Could this be a typo
for regno?

>  		if (!err && t == BPF_READ && value_regno >= 0)
>  			mark_reg_unknown(env, regs, value_regno);
>  	} else if (type_is_sk_pointer(reg->type)) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260606-c3-01-v3-v3-0-97c51f592f15@mails.tsinghua.edu.cn?part=1