On Mon, Feb 21, 2022 at 11:10 PM Kumar Kartikeya Dwivedi
<memxor@xxxxxxxxx> wrote:
>
> 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.
That highlights my point that side effects are bad.
That commit tries to work around that behavior and makes things
harder to extend like you found out with xchg logic.
Another option would be to add bpf_kptr_xchg() helper
instead of dealing with insn. It will be tiny bit slower,
but it will work on all architectures. While xchg bpf jit is
on x86,s390,mips so far.
We need to think more on how to refactor check_mem_acess without
digging ourselves into an even bigger hole.
