> The verifier rejects variable offsets for PTR_TO_TP_BUFFER and PTR_TO_BUF
> accesses, but it currently accepts a constant negative offset produced by
> pointer arithmetic.
>
> For example, a raw tracepoint writable program can load ctx[0] as a
> PTR_TO_TP_BUFFER, move it by -8, and then access the adjusted pointer.
> The access is before the tracepoint writable buffer base and should be
> rejected.
>
> Keep rejecting negative instruction offsets explicitly. Once the register
> offset is known to be constant, reject const var_off values outside the
> same +/-BPF_MAX_VAR_OFF range used by other verifier pointer offset
> checks before computing the effective access range.
>
> Fixes: 9df1c28bb752 ("bpf: add writable context for raw tracepoints")
> Signed-off-by: Sun Jian <[email protected]>
The Fixes: tag points at 9df1c28bb752 ("bpf: add writable context for
raw tracepoints"), but does that commit actually have this bug?
The code it introduced rejected any non-zero offset with:
if (!tnum_is_const(reg->var_off) || reg->var_off.value)
and a negative constant offset from pointer arithmetic was tracked in
reg->off and caught by the off < 0 check.
The constant negative offset became accepted later when
022ac0750883 ("bpf: use reg->var_off instead of reg->off for pointers")
folded static and varying offsets into reg->var_off and dropped the
|| reg->var_off.value guard, leaving:
if (!tnum_is_const(reg->var_off))
so a constant negative offset now stored in reg->var_off.value is no
longer rejected. Should the tag instead be:
Fixes: 022ac0750883 ("bpf: use reg->var_off instead of reg->off for pointers")
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 6515d4d3c003..7bcbcd43cc37 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -5326,14 +5326,19 @@ 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;
> + u64 end;
> +
> 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 +5349,36 @@ 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;
> + }
> +
> + if (size < 0) {
> + verbose(env, "%s invalid %s buffer access: off=%lld, size=%d\n",
> + reg_arg_name(env, argno), buf_info, start, size);
> + return -EACCES;
> + }
> +
> + end = (u64)start + (u64)size;
> + if (end > U32_MAX) {
> + verbose(env, "%s invalid %s buffer access: off=%lld, size=%d\n",
> + reg_arg_name(env, argno), buf_info, start, size);
> + return -EACCES;
> + }
Now that var_off is bounded within +/-BPF_MAX_VAR_OFF and start is
checked to be non-negative, can end > U32_MAX ever be true here?
With off capped at S16_MAX and size at 8, start + size stays well below
U32_MAX, so is the end > U32_MAX check reachable? This is the same
point raised in review of an earlier version, and the BPF_MAX_VAR_OFF
bound was added while this overflow check was kept.
> +
> + *access_end = (u32)end;
> +
> return 0;
> }
[ ... ]
---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/28916895865
