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

[Eduard Zingerman <eddyz87@gmail.com>]

On Thu, 2026-07-09 at 14:47 +0800, Shung-Hsi Yu wrote:
> On Wed, Jul 08, 2026 at 10:11:01PM +0800, sun jian wrote:
> [...]
> > > > @@ -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.
> 
> Actually looking again at 022ac0750883, moving the `off < 0` after
> tnum_is_const() and bringing back the `off += reg->off` removed from
> check_mem_access() is perhaps the more faithful restoration of the
> original behavior. 
> 
> Though reg->off no longer exists, we have to use reg->var_off.value
> instead. IIUC any register of pointer type should already have its
> var_off bounded to +-BPF_MAX_VAR_OFF by adjust_ptr_min_max_vals() in
> theory, and thus shouldn't overflow `int off`.
> 
> See the diff below.
> 
> [...]
> 
> > 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.
> 
> Agree and make sense. Let's see what @Eduard thinks.

I don't understand what this patch is attempting to fix.
If you run the selftests from patch #2 against current bpf-next both
would be rejected. If you extend these test cases to exercise a truly
negative offset, that would be rejected as well.

And this does not rely on UB.
Consider the current code:

	env->prog->aux->max_tp_access = max(reg->var_off.value + off + size,
					    env->prog->aux->max_tp_access);


The types of the expressions involved:

  reg->var_off.value + off + size
  u64                  int   int

The promotion/conversion rules:

  u64 + (u64)(s64)int -> u64

In other words, 'off' and 'size' would be sign extended to s64 and
then treated as u64. Hence any negative offset would be represented
as a large unsigned value in max_tp_access.

Now, the real head scratcher is whether u64 -> u32 truncation on the
assignment can bite here (mask higher bits of a big positive or
negative value). Given the bounds on the values involved:
- reg->var_off.value is -2**29..2**29
- off is -2**16+1..2**16-1
- size is 1, 2, 4 or 8

It cannot.

If one wants to be truly paranoid, one can add a truncation check
before the assignment, just to avoid the mental gymnastics.
But that won't be a fix for any observable behaviour.

[...]