On Wed, Jul 08, 2026 at 02:01:50AM -0700, Sun Jian wrote:
[...]
> Fixes: 022ac0750883 ("bpf: use reg->var_off instead of reg->off for pointers")
Agree that above is the commit that introduced the issue.
More comments below.
[...]
> @@ -5326,14 +5326,18 @@ static int check_max_stack_depth(struct
> bpf_verifier_env *env)
> static int __check_buffer_access(struct bpf_verifier_env *env,
> const char *buf_info,
> const struct bpf_reg_state *reg,
> - argno_t argno, int off, int size)
> + argno_t argno, int off, int size,
> + u32 *access_end)
> {
> + s64 start, var_off;
> +
> if (off < 0) {
> verbose(env,
> "%s invalid %s buffer access: off=%d, size=%d\n",
> reg_arg_name(env, argno), buf_info, off, size);
> return -EACCES;
> }
> +
> if (!tnum_is_const(reg->var_off)) {
> char tn_buf[48];
>
> @@ -5344,6 +5348,29 @@ static int __check_buffer_access(struct
> bpf_verifier_env *env,
> return -EACCES;
> }
>
> + var_off = (s64)reg->var_off.value;
> + if (var_off >= BPF_MAX_VAR_OFF || var_off <= -BPF_MAX_VAR_OFF) {
> + verbose(env, "%s %s buffer offset %lld is not allowed\n",
> + reg_arg_name(env, argno), buf_info, var_off);
> + return -EACCES;
> + }
> +
> + start = var_off + off;
> + if (start < 0) {
> + verbose(env,
> + "%s invalid negative %s buffer offset: off=%d,
> var_off=%lld\n",
> + reg_arg_name(env, argno), buf_info, off, var_off);
> + return -EACCES;
> + }
I was thinking of suggest to just do a single unsigned check
var_off = reg->var_off.value;
if (var_off >= BPF_MAX_VAR_OFF) {
...
But looking at the code before 022ac0750883, what you have is closer
aligned to the previous behavior, let's stick to this.
> +
> + if (size < 0) {
> + verbose(env, "%s invalid %s buffer access: off=%lld, size=%d\n",
> + reg_arg_name(env, argno), buf_info, start, size);
> + return -EACCES;
> + }
`size` comes from bpf_size_to_bytes(bpf_size) in check_mem_access(), and
is already checked to be not negative; plus other check_* helpers does
not check for this, so I think it can be dropped.
Otherwise, LGTM:
Acked-by: Shung-Hsi Yu <[email protected]>
> +
> + *access_end = (u32)start + (u32)size;
If you want, this could use a quick one-line comments regarding the fact
that it won't overflow, or even verifier_bug_if().
> +
> return 0;
> }
>
[...]
