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

[Mykyta Yatsenko <mykyta.yatsenko5@gmail.com>]

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().

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