On Thu, Jul 9, 2026 at 2:47 PM Shung-Hsi Yu <[email protected]> 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`.
>
Thanks for the additional analysis.
I checked the related verifier code paths and the following provenance items:
1. The source and initial state of PTR_TO_TP_BUFFER on the raw_tp writable
path.
2. The shared PTR_TO_TP_BUFFER/PTR_TO_BUF buffer access checker path.
3. The pointer arithmetic path through adjust_ptr_min_max_vals() and the
BPF_MAX_VAR_OFF bounds.
4. The source and range guarantees of the access size.
5. The source and range of the instruction offset passed to buffer access
checks.
6.The accounting path from effective offset calculation to access_end and
max_tp_access/max_access.
Based on these checks, folding the constant var_off back into the access
offset is consistent with the current pointer offset tracking model and is
also closer to the pre-022ac0750883 behavior.
> 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.
>
> ---
>
One remaining question is the intended semantics after restoring the
offset-folding behavior.
For example:
r6 += 16;
r0 = *(u64 *)(r6 - 8);
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index d46f7db20d8f..e116b33ad83e 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -5359,12 +5359,6 @@ static int __check_buffer_access(struct
> bpf_verifier_env *env,
> const struct bpf_reg_state *reg,
> argno_t argno, int off, int size)
> {
> - 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;
> - }
The current v4 approach rejects this because the instruction offset is negative.
> if (!tnum_is_const(reg->var_off)) {
> char tn_buf[48];
>
> @@ -5375,6 +5369,14 @@ static int __check_buffer_access(struct
> bpf_verifier_env *env,
> return -EACCES;
> }
>
> + off += reg->var_off.value;
> + 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;
> + }
> +
> return 0;
> }
>
The folded-offset approach would accept it because the final effective
offset is non-negative.
So this would change the accepted program set if we choose to restore the
previous offset-folding behavior. It would be good to clarify whether the
effective-offset behavior or the current instruction-offset restriction is
the desired convention.
I'll wait for the maintainers' preference.
Thanks,
Sun Jian
