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

[Shung-Hsi Yu <shung-hsi.yu@suse.com>]

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.

---

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index d46f7db20d8f..e116b33ad83e 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -5359,12 +5359,6 @@ static int __check_buffer_access(struct bpf_verifier_env *env,
 				 const struct bpf_reg_state *reg,
 				 argno_t argno, int off, int size)
 {
-	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];
 
@@ -5375,6 +5369,14 @@ static int __check_buffer_access(struct bpf_verifier_env *env,
 		return -EACCES;
 	}
 
+	off += reg->var_off.value;
+	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;
+	}
+
 	return 0;
 }