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

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

On Mon, Jul 13, 2026 at 01:44:18PM -0700, Eduard Zingerman wrote:
> On Mon, 2026-07-13 at 13:05 +0800, Shung-Hsi Yu wrote:
> > On Fri, Jul 10, 2026 at 06:27:41PM +0800, sun jian wrote:
> > [...]
> > > One detail about the attach-time check: I agree that some negative-offset
> > > constructions can still be rejected later if the max_tp_access accounting
> > > produces a large value. But the runtime reproducer I used exercises this
> > > specific access:
> > > 
> > >     r6 = *(u64 *)(r1 + 0)
> > >     r6 += -8
> > >     *(u64 *)(r6 + 0) = 0
> > > 
> > > Here reg->var_off.value is (u64)-8, off is 0, and size is 8. With the current
> > > accounting,
> > >     reg->var_off.value + off + size
> > > wraps to 0. So max_tp_access records 0 and the attach-time writable_size check
> > > does not reject it, even though the effective access starts at -8.
> > 
> > Yes, that should had been rejected.
> > 
> > > On the bpf base without the verifier fix, the temporary reproducer did load
> > > and attach successfully:
> > >     negative_bpf_load: PASS
> > >     negative_raw_tp_open: PASS
> > >     negative_test_run: PASS
> > > and KASAN reported the stack-out-of-bounds write in ___bpf_prog_run.
> > > 
> > > So for this particular range, this looks like more than a load-time policy
> > > difference: the missing lower-bound check leaves the access executable on the
> > > bpf tree.
> > 
> > Actually I got this reproduced in the *bpf-next* tree now. And I think I
> > know why Eduard wasn't able to reproduce the issue.
> > 
> > The main problem was that CONFIG_BLK_DEV_NBD was not enabled in the
> > default BPF selftests configuration, so nbd_send_request wasn't
> > available, and thus the attachment always fail with -2 without
> > exercising the checks that we have in bpf_probe_register().
> > 
> > With CONFIG_BLK_DEV_NBD enabled, and negative_var_off_program[] updated
> > to do use offset of 0 (instead of 28 + 8), I was able to reproduce the
> > issue on top of commit `9a3a07d06e7d` "Merge branch
> > 'bpf-fix-warning-in-bpf_trampoline_multi_detach'" with the following
> > updated version of your patch.
> > 
> >   0: R1=ctx() R10=fp0
> >   0: (79) r6 = *(u64 *)(r1 +0)          ; R1=ctx() R6=tp_buffer()
> >   1: (07) r6 += -8                      ; R6=tp_buffer(imm=-8)
> >   2: (79) r0 = *(u64 *)(r6 +0)          ; R0=scalar() R6=tp_buffer(imm=-8)
> >   3: (95) exit
> >   processed 4 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0
> >   tp_fd: 8
> >   check_nbd_attach_reject:FAIL:nbd_invalid_negative_var_off unexpected nbd_invalid_negative_var_off: actual 8 >= expected 0
> >   #319     raw_tp_writable_reject_nbd_invalid:FAIL
> > 
> > So I would say a fixes tag that points to 022ac0750883 is still needed
> > after all.
> 
> Hi Shung-Hsi, Sun,
> 
> Shung-Hsi, thank you for figuring out the quirk with the config.
> Indeed if I modify the test to be config independent the following
> program is both accepted at load and attached:
> 
>   *r6 = *(u64 *)(r1 +0)
>   r6 += -8
>   r0 = *(u64 *)(r6 +0)
> 
> Worse yet, the mechanism affects both PTR_TO_TP_BUFFER and PTR_TO_BUF.
> The PTR_TO_BUF is reachable from helpers/kfuncs with custom 'size' assignments,
> as far as I understand.
> 
> Sun, of checks added by this patch:
> 
> >	var_off = (s64)reg->var_off.value;
> >	if (var_off >= BPF_MAX_VAR_OFF || var_off <= -BPF_MAX_VAR_OFF)
> 
> This should hold already.

FWIW, I had thought so, but after checking I realize that the verifier
only ensure that

  -BPF_MAX_VAR_OFF < smin < BPF_MAX_VAR_OFF

holds true in check_reg_sane_offset_{scalar,ptr}(), and does not check
smax, so technically var_off.value can be greater than BPF_MAX_VAR_OFF
when we reach here, just that it doesn't really matter much as it is
rejected anyway. And if it is a constant then smin=smax, so for a
constant value it does always falls between +-BPF_MAX_VAR_OFF.

  0: (79) r6 = *(u64 *)(r1 +0)          ; R1=ctx() R6=tp_buffer()
  1: (85) call bpf_get_prandom_u32#7    ; R0=scalar()
  2: (bf) r2 = r0                       ; R0=scalar(id=1) R2=scalar(id=1)
  3: (67) r2 <<= 32                     ; R2=scalar(smax=0x7fffffff00000000,smin32=0,smax32=umax32=0,var_off=(0x0; 0xffffffff00000000))
  4: (77) r2 >>= 32                     ; R2=scalar(smin=0,smax=umax=0xffffffff,var_off=(0x0; 0xffffffff))
  5: (07) r2 += 536870911               ; R2=scalar(smin=umin=0x1fffffff,smax=umax=0x11ffffffe,var_off=(0x0; 0x1ffffffff))
                /* r2 is now [2^29 - 1, 2^32 + 2^29 - 2] */
  6: (0f) r6 += r2
  7: R2=scalar(smin=umin=0x1fffffff,smax=umax=0x11ffffffe,var_off=(0x0; 0x1ffffffff)) R6=tp_buffer(smin=umin=0x1fffffff,smax=umax=0x11ffffffe,var_off=(0x0; 0x1ffffffff))
  7: (79) r0 = *(u64 *)(r6 +0)
  R6 invalid variable buffer offset: off=0, var_off=(0x0; 0x1ffffffff)

In short, it is much simpler to just check tnum_is_const() right away
before using var_off.value.

> >	start = var_off + off;
> >	if (start < 0)
> 
> This one is necessary.
> 
> >	if (size < 0) {
> 
> Technically this one is bounded by 1,2,4,8 in case of instructions and
> by BPF_MAX_VAR_SIZ in case of a helper/kfunc (see check_mem_size_reg()).

Oh right, missed the check_mem_size_reg() case. But as Eduard said, this
should still be bounded, so can drop this check.

[...]