Re: [PATCH bpf v4 1/2] bpf: Reject negative const offsets for buffer pointers

[sun jian]

On Wed, Jul 8, 2026 at 9:13 PM Shung-Hsi Yu <shung-hsi.yu@xxxxxxxx> wrote:
>
> 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 <shung-hsi.yu@xxxxxxxx>
>
> > +
> > +     *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;
> >  }
> >
> [...]

Thanks for taking another look, and thanks for the Ack.

I agree that the size check is redundant given the current call path. I’ll leave
v4 as-is for now to avoid another respin unless maintainers prefer dropping it
or adding a short comment around the access_end calculation.

Best Regards
Sun Jian