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

[Eduard Zingerman <eddyz87@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3978 bytes --]

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.

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

Let's respin v5 keeping minimal necessary checks and also update the
test case:
- use sub-tests.
- move raw_tp_writable_reject_nbd_invalid.c to use tracepoing from
  the test module instead of nbd, to avoid nbd config dependency
  (as in the attachment).
- if possible, could you please add a test that shows missing negative
  offset access check to PTR_TO_BUF?

[...]

[-- Attachment #2: tp-test.diff --]
[-- Type: text/x-patch, Size: 1733 bytes --]

diff --git a/tools/testing/selftests/bpf/prog_tests/raw_tp_writable_reject_nbd_invalid.c b/tools/testing/selftests/bpf/prog_tests/raw_tp_writable_reject_bad_access.c
similarity index 75%
rename from tools/testing/selftests/bpf/prog_tests/raw_tp_writable_reject_nbd_invalid.c
rename to tools/testing/selftests/bpf/prog_tests/raw_tp_writable_reject_bad_access.c
index 216b0dfac0fe..e0a1ca3c1e6a 100644
--- a/tools/testing/selftests/bpf/prog_tests/raw_tp_writable_reject_nbd_invalid.c
+++ b/tools/testing/selftests/bpf/prog_tests/raw_tp_writable_reject_bad_access.c
@@ -1,10 +1,10 @@
 // SPDX-License-Identifier: GPL-2.0
 
 #include <test_progs.h>
-#include <linux/nbd.h>
+#include "test_kmods/bpf_testmod.h"
 #include "bpf_util.h"
 
-void test_raw_tp_writable_reject_nbd_invalid(void)
+void test_raw_tp_writable_reject_bad_access(void)
 {
 	__u32 duration = 0;
 	char error[4096];
@@ -13,9 +13,9 @@ void test_raw_tp_writable_reject_nbd_invalid(void)
 	const struct bpf_insn program[] = {
 		/* r6 is our tp buffer */
 		BPF_LDX_MEM(BPF_DW, BPF_REG_6, BPF_REG_1, 0),
-		/* one byte beyond the end of the nbd_request struct */
+		/* one byte beyond the end of the writable context struct */
 		BPF_LDX_MEM(BPF_B, BPF_REG_0, BPF_REG_6,
-			    sizeof(struct nbd_request)),
+			    sizeof(struct bpf_testmod_test_writable_ctx)),
 		BPF_EXIT_INSN(),
 	};
 
@@ -32,7 +32,7 @@ void test_raw_tp_writable_reject_nbd_invalid(void)
 		  "failed: %d errno %d\n", bpf_fd, errno))
 		return;
 
-	tp_fd = bpf_raw_tracepoint_open("nbd_send_request", bpf_fd);
+	tp_fd = bpf_raw_tracepoint_open("bpf_testmod_test_writable_bare_tp", bpf_fd);
 	if (CHECK(tp_fd >= 0, "bpf_raw_tracepoint_writable open",
 		  "erroneously succeeded\n"))
 		goto out_bpffd;