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