Re: [PATCH bpf-next 1/2] bpf: Do not ignore offsets for loads from insn_arrays

[Anton Protopopov <a.s.protopopov@gmail.com>]

On 26/04/01 11:47PM, Mykyta Yatsenko wrote:
> 
> 
> On 4/1/26 5:15 PM, Anton Protopopov wrote:
> > When a pointer to PTR_TO_INSN is dereferenced it is possible to
> > specify an offset inside the load instruction. This is a bug,
> > because while the verifier ignores the field, JITs are not.
> > So, patch the verifier to not ignore this field.
> > 
> > Reported-by: Jiyong Yang <ksur673@gmail.com>
> > Fixes: 493d9e0d6083 ("bpf, x86: add support for indirect jumps")
> > Signed-off-by: Anton Protopopov <a.s.protopopov@gmail.com>
> > ---
> >   kernel/bpf/verifier.c | 17 +++++++++++++++++
> >   1 file changed, 17 insertions(+)
> > 
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 8c1cf2eb6cbb..f1b1c8e9dc26 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -212,6 +212,8 @@ static int ref_set_non_owning(struct bpf_verifier_env *env,
> >   static bool is_trusted_reg(const struct bpf_reg_state *reg);
> >   static inline bool in_sleepable_context(struct bpf_verifier_env *env);
> >   static const char *non_sleepable_context_description(struct bpf_verifier_env *env);
> > +static void scalar32_min_max_add(struct bpf_reg_state *dst_reg, struct bpf_reg_state *src_reg);
> > +static void scalar_min_max_add(struct bpf_reg_state *dst_reg, struct bpf_reg_state *src_reg);
> >   static bool bpf_map_ptr_poisoned(const struct bpf_insn_aux_data *aux)
> >   {
> > @@ -7735,6 +7737,20 @@ static bool get_func_retval_range(struct bpf_prog *prog,
> >   	return false;
> >   }
> > +static inline void add_scalar_to_reg(struct bpf_reg_state *dst_reg, s64 val)
> 
> Why does it need to be manually inlined?

No reason, thanks, fixed.

> Other than that the change looks good, the implementation of the
> add_scalar_to_reg() is similar to a piece of code in sync_linked_regs().

Thanks, I will send v2 today. Also will add a "if (!val) return"
check to skip adding 0.

Just in case, sashiko had some comments on this patch, all are
false-negative (for selftests it found same two issues as you).

> > +{
> > +	struct bpf_reg_state fake_reg;
> > +
> > +	fake_reg.type = SCALAR_VALUE;
> > +	__mark_reg_known(&fake_reg, val);
> > +
> > +	scalar32_min_max_add(dst_reg, &fake_reg);
> > +	scalar_min_max_add(dst_reg, &fake_reg);
> > +	dst_reg->var_off = tnum_add(dst_reg->var_off, fake_reg.var_off);
> > +
> > +	reg_bounds_sync(dst_reg);
> > +}
> > +
> >   /* check whether memory at (regno + off) is accessible for t = (read | write)
> >    * if t==write, value_regno is a register which value is stored into memory
> >    * if t==read, value_regno is a register which will receive the value from memory
> > @@ -7816,6 +7832,7 @@ static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, u32 regn
> >   					return -EACCES;
> >   				}
> >   				copy_register_state(&regs[value_regno], reg);
> > +				add_scalar_to_reg(&regs[value_regno], off);
> >   				regs[value_regno].type = PTR_TO_INSN;
> >   			} else {
> >   				mark_reg_unknown(env, regs, value_regno);
>