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

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

On Fri, Jul 10, 2026 at 01:52:26PM +0800, sun jian wrote:
> On Fri, Jul 10, 2026 at 2:21 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
> > 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:
[...]
> > > 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 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.

I tried to exercise a truly negative offset, and it was indeed rejected
on bpf-next. I had thought it would pass.

But more below.

> Thanks for pushing on this. I rechecked the issue more carefully.
> 
> This series targets the bpf tree, with base 12091470c6b4. On that base,
> with only the selftest change applied, the negative-offset verifier case
> is not rejected at load time. The test fails with an unexpected load
> success:
> 
> #664/2 verifier_raw_tp_writable/raw_tracepoint_writable: reject
> negative const offset:FAIL
> run_subtest:FAIL:unexpected_load_success unexpected success: 0
> 
> It is possible that current bpf-next rejects this earlier through another
> path, but on the target bpf base it does not.

So I think I now have a better picture of what's going on. Before
022ac0750883, negative const offset is rejected at *load* time because
the const offset is accumulated into `off` before the negative check:

    // check_mem_access()
    off += reg->off
	// __check_buffer_access()
	if (off < 0) return return -EACCES;
	if (!tnum_is_const(reg->var_off) || reg->var_off.value) return -EACCES;

After 022ac0750883 the const offset accumulation does not happen, and
thus now negative const offset is no longer rejected at load time.

	// __check_buffer_access()
	if (off < 0) return return -EACCES;
	if (!tnum_is_const(reg->var_off)) return -EACCES;

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

I entirely missed that, yes, the attached-time check does prevented the
negative offset going through. So there isn't a bug, but the behavior
has changed, and it seems better to restore to the one where we
straightout reject negative offset during load time.

That can be done by reordering `off < 0` check after tnum_is_const(),
similar to how check_ptr_to_btf_access() was update in commit
022ac0750883.

[...]