On Fri, Jul 10, 2026 at 4:00 PM Shung-Hsi Yu <[email protected]> wrote:
>
> On Fri, Jul 10, 2026 at 01:52:26PM +0800, sun jian wrote:
> > On Fri, Jul 10, 2026 at 2:21 AM Eduard Zingerman <[email protected]> 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.
>
Thanks for checking this and for the verifier logs.
The before/after logs match what I see as well. Before 022ac0750883, the
constant pointer offset was folded into off, so this case was rejected at
load time with:
R6 invalid tracepoint buffer access: off=-8, size=8
After 022ac0750883, the same constant offset remains in reg->var_off.value,
and the program is accepted at load time with:
R6=tp_buffer(imm=-8)
processed 4 insns
So I agree that the behavior change comes from 022ac0750883, and that the
right direction is to restore the load-time rejection of negative effective
buffer offsets.
> 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.
>
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.
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.
In any case, I agree that v5 should restore the previous load-time rejection
semantics. For v5, I'm inclined to follow the direction you suggested: fold the
constant reg->var_off.value into off before the negative check, similar to the
022ac0750883 handling in check_ptr_to_btf_access(), rather than keeping the v4
explicit access_end form. I'll hold off on respinning until you and Eduard are
settled on the approach.
> 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.
>
> [...]
