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

[sun jian]

On Tue, Jul 7, 2026 at 3:17 PM Shung-Hsi Yu <shung-hsi.yu@xxxxxxxx> wrote:
>
> Sorry I missed that there was a v2 already. Reposting the comments I
> sent to v1 below.
>
> On Mon, Jul 06, 2026 at 11:08:03PM -0700, Sun Jian wrote:
> [...]
> > @@ -5344,6 +5348,29 @@ static int __check_buffer_access(struct bpf_verifier_env *env,
> >               return -EACCES;
> >       }
> >
> > +     var_off = (s64)reg->var_off.value;
> > +     if (check_add_overflow(var_off, (s64)off, &start)) {
> > +             verbose(env,
> > +                     "%s invalid %s buffer access: off=%d, var_off=%lld\n",
> > +                     reg_arg_name(env, argno), buf_info, off, var_off);
> > +             return -EACCES;
> > +     }
>
> Other pointer offset checks (check_mem_region_access and
> adjust_ptr_min_max_vals) seem to check var_off against BPF_MAX_VAR_OFF,
> and that `off` being >= 0. It would be great if the convention can be
> kept for consistency, albeit being more conservative.
>
> From a blind guess I would guess that compiler does not product a
> pattern where `off` encoded in the instruction is negative but the net
> overall offset is positive for PTR_TO_TP_BUFFER/PTR_TO_BUF, and
> rejecting such pattern is fine.
>
>   r6 = *(u64 *)(r1 + 0);
>   r6 += 16;
>   r0 = *(u64 *)(r6 - 8);
>
> One more comment below.
>
> > +
> > +     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;
> > +     }
> > +
> > +     if (start > U32_MAX || size < 0 ||
> > +         check_add_overflow((u32)start, (u32)size, access_end)) {
> > +             verbose(env,
> > +                     "%s invalid %s buffer access: off=%lld, size=%d\n",
> > +                     reg_arg_name(env, argno), buf_info, start, size);
> > +             return -EACCES;
> > +     }
>
> If we check that var_off is within BPF_MAX_VAR_OFF and that `off` is not
> negative, then `var_off + off + size` should be guaranteed to not
> overflow max_tp_access, since the maximum of `off` is S16_MAX and the
> maximum of `size` is 8.
>
> > +
> >       return 0;
> >  }
> >
> [...]

Thanks a lot for taking a look!

This makes sense. I’ll rework this to align the buffer access check with
the existing verifier offset convention, including the BPF_MAX_VAR_OFF
bounds, and send a v3.

Thanks again,
Sun Jian