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: > > [...] > > > > > @@ -5326,14 +5326,18 @@ static int check_max_stack_depth(struct > > > > > bpf_verifier_env *env) > > > > > static int __check_buffer_access(struct bpf_verifier_env *env, > > > > > const char *buf_info, > > > > > const struct bpf_reg_state *reg, > > > > > - argno_t argno, int off, int size) > > > > > + argno_t argno, int off, int size, > > > > > + u32 *access_end) > > > > > { > > > > > + s64 start, var_off; > > > > > + > > > > > if (off < 0) { > > > > > verbose(env, > > > > > "%s invalid %s buffer access: off=%d, > > > > > size=%d\n", > > > > > reg_arg_name(env, argno), buf_info, off, size); > > > > > return -EACCES; > > > > > } > > > > > + > > > > > if (!tnum_is_const(reg->var_off)) { > > > > > char tn_buf[48]; > > > > > > > > > > @@ -5344,6 +5348,29 @@ static int __check_buffer_access(struct > > > > > bpf_verifier_env *env, > > > > > return -EACCES; > > > > > } > > > > > > > > > > + var_off = (s64)reg->var_off.value; > > > > > + if (var_off >= BPF_MAX_VAR_OFF || var_off <= -BPF_MAX_VAR_OFF) { > > > > > + verbose(env, "%s %s buffer offset %lld is not > > > > > allowed\n", > > > > > + reg_arg_name(env, argno), buf_info, var_off); > > > > > + return -EACCES; > > > > > + } > > > > > + > > > > > + start = var_off + off; > > > > > + if (start < 0) { > > > > > + verbose(env, > > > > > + "%s invalid negative %s buffer offset: off=%d, > > > > > var_off=%lld\n", > > > > > + reg_arg_name(env, argno), buf_info, off, > > > > > var_off); > > > > > + return -EACCES; > > > > > + } > > > > > > > > I was thinking of suggest to just do a single unsigned check > > > > > > > > var_off = reg->var_off.value; > > > > if (var_off >= BPF_MAX_VAR_OFF) { > > > > ... > > > > > > > > But looking at the code before 022ac0750883, what you have is closer > > > > aligned to the previous behavior, let's stick to this. > > > > 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 agree that the size check is redundant given the current call path. > > > I’ll leave > > > v4 as-is for now to avoid another respin unless maintainers prefer > > > dropping it > > > or adding a short comment around the access_end calculation. > > > > Agree and make sense. Let's see what @Eduard thinks. > > 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. >
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. > 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. > Agreed, this is not a u64-to-u32 truncation issue. For the problematic case the access range is [-8, 0), so the end offset is 0. Since max_tp_access only tracks the upper bound, the attach-time writable_size check cannot catch the negative start offset. The missing piece is the lower-bound check. > Now, the real head scratcher is whether u64 -> u32 truncation on the > assignment can bite here (mask higher bits of a big positive or > negative value). Given the bounds on the values involved: > - reg->var_off.value is -2**29..2**29 > - off is -2**16+1..2**16-1 > - size is 1, 2, 4 or 8 > > It cannot. > > If one wants to be truly paranoid, one can add a truncation check > before the assignment, just to avoid the mental gymnastics. > But that won't be a fix for any observable behaviour. > There is an observable effect on the target bpf base. I tested it with a temporary raw_tp writable test_run reproducer, not part of the series. It attaches a raw_tp writable program to bpf_test_finish and triggers it through BPF_PROG_TEST_RUN. The program does: r6 = *(u64 *)(r1 + 0) r6 += -8 *(u64 *)(r6 + 0) = 0 With KASAN enabled and the BPF JIT disabled, on the bpf base without the verifier fix, the program loads, attaches, executes, and KASAN reports: BUG: KASAN: stack-out-of-bounds in ___bpf_prog_run Write of size 8 with the relevant call trace: ___bpf_prog_run __bpf_prog_run32 bpf_trace_run1 bpf_test_finish bpf_prog_test_run_skb bpf_prog_test_run __sys_bpf The report points to the bpf_test_finish stack frame, which matches the raw_tp writable program being attached to bpf_test_finish. With this series applied, the same negative-offset access is rejected at load time: R6 invalid negative tracepoint buffer offset: off=0, var_off=-8 Rerunning the same reproducer against the fixed kernel, the negative program is rejected by bpf_prog_load() and the KASAN log stays clean: NEGATIVE_RUNTIME_REPRO: negative_bpf_load rejected at load time: fd=-13 errno=13 So the observable behavior is the missing lower-bound check: on the target bpf base, the verifier can accept and execute an access before the writable tracepoint buffer base. The patch prevents that by rejecting negative effective buffer offsets at verification time. Thanks, Sun Jian > [...]
