Re: [PATCH v1 bpf-next 08/11] bpf, x86: add support for indirect jumps

[Eduard Zingerman <eddyz87@gmail.com>]

On Wed, 2025-08-27 at 15:34 +0000, Anton Protopopov wrote:

[...]

> > > diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> > > index aca43c284203..6e68e0082c81 100644
> > > --- a/include/linux/bpf_verifier.h
> > > +++ b/include/linux/bpf_verifier.h
> > > @@ -77,7 +77,15 @@ struct bpf_reg_state {
> > >  			 * the map_uid is non-zero for registers
> > >  			 * pointing to inner maps.
> > >  			 */
> > > -			u32 map_uid;
> > > +			union {
> > > +				u32 map_uid;
> > > +
> > > +				/* Used to track boundaries of a PTR_TO_INSN */
> > > +				struct {
> > > +					u32 min_index;
> > > +					u32 max_index;
> > 
> > Could you please elaborate why these fields are necessary?
> > It appears that .var_off/.{s,u}{32_,}{min,max}_value fields can be
> > used to track current index bounds (min/max fields for bounds,
> > .var_off field to check 8-byte alignment).
> 
> I thought it is better readable (and not wasting memory anymore).
> They clearly say "pointer X was loaded from an instruction pointer
> map M and can point to any of {M[min_index], ..., M[max_index]}".
> Those indexes come from off_reg, not ptr_reg. In order to use
> ptr_reg->u_min/u_max instead, more checks should be added (like those
> in BPF_ADD for min/max_index) to check that registers doesn't point
> to outside of M->ips. I am not sure this will be easier to read.
> 
> Also, PTR_TO_INSN is created by dereferencing the address, and right
> now it looks easier just to copy min/max_index. As I understand,
> normally this register is set to ips[var_off] and marked as unknown,
> so there will be additional code to use u_min/u_max to keep track of
> boundaries.
> 
> Or do you think this is still more clear?
> 
> I will try to look into this again in the morning.

The main point is uniformity. For other pointer types current
boundaries are tracked via .var_off/.{s,u}{32_,}{min,max}_value,
out of range access is reported at the point of actual access.
Imo, preserving this uniformity simplifies reasoning about the code.

[...]

> > > @@ -173,6 +172,20 @@ static u64 insn_array_mem_usage(const struct bpf_map *map)
> > >  	return insn_array_alloc_size(map->max_entries) + extra_size;
> > >  }
> > >  
> > > +static int insn_array_map_direct_value_addr(const struct bpf_map *map, u64 *imm, u32 off)
> > > +{
> > > +	struct bpf_insn_array *insn_array = cast_insn_array(map);
> > > +
> > > +	if ((off % sizeof(long)) != 0 ||
> > > +	    (off / sizeof(long)) >= map->max_entries)
> > > +		return -EINVAL;
> > > +
> > > +	/* from BPF's point of view, this map is a jump table */
> > > +	*imm = (unsigned long)insn_array->ips + off / sizeof(long);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > 
> > This function is called during main verification pass by
> > verifier.c:check_mem_access() -> verifier.c:bpf_map_direct_read().
> > However, insn_array->ips is filled by bpf_jit_comp.c:do_jit()
> > bpf_insn_array.c:bpf_prog_update_insn_ptr(), which is called *after*
> > main verification pass. Do I miss something, or this can't work?
> 
> This gets an address &ips[off], not the address of the bpf program.
> Ad this moment ips[off] contains garbage. Later when
> bpf_prog_update_insn_ptr() is executed, ips[off] is populated with
> the real IP. The running program then reads it by dereferencing the
> [correct at this time] address, i.e., *(&ips[off]).

Ack, missed the address part, thank you for explaining

[...]