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.
[...]
