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.
