On Wed, Jul 8, 2026 at 9:13 PM Shung-Hsi Yu <[email protected]> wrote: > > On Wed, Jul 08, 2026 at 02:01:50AM -0700, Sun Jian wrote: > [...] > > Fixes: 022ac0750883 ("bpf: use reg->var_off instead of reg->off for > > pointers") > > Agree that above is the commit that introduced the issue. > > More comments below. > > [...] > > @@ -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. > > > + > > + 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; > > + } > > `size` comes from bpf_size_to_bytes(bpf_size) in check_mem_access(), and > is already checked to be not negative; plus other check_* helpers does > not check for this, so I think it can be dropped. > > Otherwise, LGTM: > > Acked-by: Shung-Hsi Yu <[email protected]> > > > + > > + *access_end = (u32)start + (u32)size; > > If you want, this could use a quick one-line comments regarding the fact > that it won't overflow, or even verifier_bug_if(). > > > + > > return 0; > > } > > > [...]
Thanks for taking another look, and thanks for the Ack. 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. Best Regards Sun Jian
