Re: [PATCH bpf-next v1 04/15] bpf: Allow storing referenced PTR_TO_BTF_ID in map

[Kumar Kartikeya Dwivedi <memxor@gmail.com>]

On Tue, Feb 22, 2022 at 12:23:49PM IST, Alexei Starovoitov wrote:
> On Sun, Feb 20, 2022 at 07:18:02PM +0530, Kumar Kartikeya Dwivedi wrote:
> >  static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, u32 regno,
> >  			    int off, int bpf_size, enum bpf_access_type t,
> > -			    int value_regno, bool strict_alignment_once)
> > +			    int value_regno, bool strict_alignment_once,
> > +			    struct bpf_reg_state *atomic_load_reg)
>
> No new side effects please.
> value_regno is not pretty already.
> At least its known ugliness that we need to clean up one day.
>
> >  static int check_atomic(struct bpf_verifier_env *env, int insn_idx, struct bpf_insn *insn)
> >  {
> > +	struct bpf_reg_state atomic_load_reg;
> >  	int load_reg;
> >  	int err;
> >
> > +	__mark_reg_unknown(env, &atomic_load_reg);
> > +
> >  	switch (insn->imm) {
> >  	case BPF_ADD:
> >  	case BPF_ADD | BPF_FETCH:
> > @@ -4813,6 +4894,7 @@ static int check_atomic(struct bpf_verifier_env *env, int insn_idx, struct bpf_i
> >  		else
> >  			load_reg = insn->src_reg;
> >
> > +		atomic_load_reg = *reg_state(env, load_reg);
> >  		/* check and record load of old value */
> >  		err = check_reg_arg(env, load_reg, DST_OP);
> >  		if (err)
> > @@ -4825,20 +4907,21 @@ static int check_atomic(struct bpf_verifier_env *env, int insn_idx, struct bpf_i
> >  	}
> >
> >  	/* Check whether we can read the memory, with second call for fetch
> > -	 * case to simulate the register fill.
> > +	 * case to simulate the register fill, which also triggers checks
> > +	 * for manipulation of BTF ID pointers embedded in BPF maps.
> >  	 */
> >  	err = check_mem_access(env, insn_idx, insn->dst_reg, insn->off,
> > -			       BPF_SIZE(insn->code), BPF_READ, -1, true);
> > +			       BPF_SIZE(insn->code), BPF_READ, -1, true, NULL);
> >  	if (!err && load_reg >= 0)
> >  		err = check_mem_access(env, insn_idx, insn->dst_reg, insn->off,
> >  				       BPF_SIZE(insn->code), BPF_READ, load_reg,
> > -				       true);
> > +				       true, load_reg >= 0 ? &atomic_load_reg : NULL);
>
> Special xchg logic should be down outside of check_mem_access()
> instead of hidden by layers of calls.

Right, it's ugly, but if we don't capture the reg state before that
check_reg_arg(env, load_reg, DST_OP), it's not possible to see the actual
PTR_TO_BTF_ID being moved into the map, since check_reg_arg will do a
mark_reg_unknown for value_regno. Any other ideas on what I can do?

37086bfdc737 ("bpf: Propagate stack bounds to registers in atomics w/ BPF_FETCH")
changed the order of check_mem_access and DST_OP check_reg_arg.

--
Kartikeya