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/02 10:37AM, sun jian wrote:
> On Thu, Apr 2, 2026 at 6:47 AM Mykyta Yatsenko
> <mykyta.yatsenko5@gmail.com> 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?
> > 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().
> Agreed.
> 
> nit: maybe this helper could be called from sync_linked_regs() too?

Looks like out of scope for this particular patch.

> >
> > > +{
> > > +     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);
> >
> >