Commit 1b688a19 authored by Edward Cree's avatar Edward Cree Committed by David S. Miller
Browse files

bpf/verifier: remove varlen_map_value_access flag 



The optimisation it does is broken when the 'new' register value has a
 variable offset and the 'old' was constant.  I broke it with my pointer
 types unification (see Fixes tag below), before which the 'new' value
 would have type PTR_TO_MAP_VALUE_ADJ and would thus not compare equal;
 other changes in that patch mean that its original behaviour (ignore
 min/max values) cannot be restored.
Tests on a sample set of cilium programs show no change in count of
 processed instructions.

Fixes: f1174f77 ("bpf/verifier: rework value tracking")
Signed-off-by: default avatarEdward Cree <ecree@solarflare.com>
Acked-by: default avatarAlexei Starovoitov <ast@kernel.org>
Acked-by: default avatarDaniel Borkmann <daniel@iogearbox.net>
Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
parent df20cb7e

include/linux/bpf_verifier.h

+0 −1
Original line number Original line Diff line number Diff line
@@ -125,7 +125,6 @@ struct bpf_verifier_env { 
	u32 id_gen;			/* used to generate unique reg IDs */

	u32 id_gen;			/* used to generate unique reg IDs */

	bool allow_ptr_leaks;

	bool allow_ptr_leaks;

	bool seen_direct_write;

	bool seen_direct_write;

	bool varlen_map_value_access;

	struct bpf_insn_aux_data *insn_aux_data; /* array of per-insn state */

	struct bpf_insn_aux_data *insn_aux_data; /* array of per-insn state */

};

};

kernel/bpf/verifier.c

+12 −29
Original line number Original line Diff line number Diff line
@@ -832,11 +832,6 @@ static int check_map_access(struct bpf_verifier_env *env, u32 regno, 
	 */

	 */

	if (log_level)

	if (log_level)

		print_verifier_state(state);

		print_verifier_state(state);

	/* If the offset is variable, we will need to be stricter in state

	 * pruning from now on.

	 */

	if (!tnum_is_const(reg->var_off))

		env->varlen_map_value_access = true;

	/* The minimum value is only important with signed

	/* The minimum value is only important with signed

	 * comparisons where we can't assume the floor of a

	 * comparisons where we can't assume the floor of a

	 * value is 0.  If we are using signed variables for our

	 * value is 0.  If we are using signed variables for our
@@ -3247,9 +3242,8 @@ static bool check_ids(u32 old_id, u32 cur_id, struct idpair *idmap) 
}

}





/* Returns true if (rold safe implies rcur safe) */

/* Returns true if (rold safe implies rcur safe) */

static bool regsafe(struct bpf_reg_state *rold,

static bool regsafe(struct bpf_reg_state *rold, struct bpf_reg_state *rcur,

		    struct bpf_reg_state *rcur,

		    struct idpair *idmap)

		    bool varlen_map_access, struct idpair *idmap)

{

{

	if (!(rold->live & REG_LIVE_READ))

	if (!(rold->live & REG_LIVE_READ))

		/* explored state didn't use this */

		/* explored state didn't use this */
@@ -3281,7 +3275,6 @@ static bool regsafe(struct bpf_reg_state *rold, 
			       tnum_is_unknown(rold->var_off);

			       tnum_is_unknown(rold->var_off);

		}

		}

	case PTR_TO_MAP_VALUE:

	case PTR_TO_MAP_VALUE:

		if (varlen_map_access) {

		/* If the new min/max/var_off satisfy the old ones and

		/* If the new min/max/var_off satisfy the old ones and

		 * everything else matches, we are OK.

		 * everything else matches, we are OK.

		 * We don't care about the 'id' value, because nothing

		 * We don't care about the 'id' value, because nothing
@@ -3290,13 +3283,6 @@ static bool regsafe(struct bpf_reg_state *rold, 
		return memcmp(rold, rcur, offsetof(struct bpf_reg_state, id)) == 0 &&

		return memcmp(rold, rcur, offsetof(struct bpf_reg_state, id)) == 0 &&

		       range_within(rold, rcur) &&

		       range_within(rold, rcur) &&

		       tnum_in(rold->var_off, rcur->var_off);

		       tnum_in(rold->var_off, rcur->var_off);

		} else {

			/* If the ranges/var_off were not the same, but

			 * everything else was and we didn't do a variable

			 * access into a map then we are a-ok.

			 */

			return memcmp(rold, rcur, offsetof(struct bpf_reg_state, id)) == 0;

		}

	case PTR_TO_MAP_VALUE_OR_NULL:

	case PTR_TO_MAP_VALUE_OR_NULL:

		/* a PTR_TO_MAP_VALUE could be safe to use as a

		/* a PTR_TO_MAP_VALUE could be safe to use as a

		 * PTR_TO_MAP_VALUE_OR_NULL into the same map.

		 * PTR_TO_MAP_VALUE_OR_NULL into the same map.
@@ -3380,7 +3366,6 @@ static bool states_equal(struct bpf_verifier_env *env, 
			 struct bpf_verifier_state *old,

			 struct bpf_verifier_state *old,

			 struct bpf_verifier_state *cur)

			 struct bpf_verifier_state *cur)

{

{

	bool varlen_map_access = env->varlen_map_value_access;

	struct idpair *idmap;

	struct idpair *idmap;

	bool ret = false;

	bool ret = false;

	int i;

	int i;
@@ -3391,8 +3376,7 @@ static bool states_equal(struct bpf_verifier_env *env, 
		return false;

		return false;





	for (i = 0; i < MAX_BPF_REG; i++) {

	for (i = 0; i < MAX_BPF_REG; i++) {

		if (!regsafe(&old->regs[i], &cur->regs[i], varlen_map_access,

		if (!regsafe(&old->regs[i], &cur->regs[i], idmap))

			     idmap))

			goto out_free;

			goto out_free;

	}

	}
@@ -3412,7 +3396,7 @@ static bool states_equal(struct bpf_verifier_env *env, 
			continue;

			continue;

		if (!regsafe(&old->spilled_regs[i / BPF_REG_SIZE],

		if (!regsafe(&old->spilled_regs[i / BPF_REG_SIZE],

			     &cur->spilled_regs[i / BPF_REG_SIZE],

			     &cur->spilled_regs[i / BPF_REG_SIZE],

			     varlen_map_access, idmap))

			     idmap))

			/* when explored and current stack slot are both storing

			/* when explored and current stack slot are both storing

			 * spilled registers, check that stored pointers types

			 * spilled registers, check that stored pointers types

			 * are the same as well.

			 * are the same as well.
@@ -3555,7 +3539,6 @@ static int do_check(struct bpf_verifier_env *env) 
	init_reg_state(regs);

	init_reg_state(regs);

	state->parent = NULL;

	state->parent = NULL;

	insn_idx = 0;

	insn_idx = 0;

	env->varlen_map_value_access = false;

	for (;;) {

	for (;;) {

		struct bpf_insn *insn;

		struct bpf_insn *insn;

		u8 class;

		u8 class;

CRA Git | Maintained and supported by SUSTech CRA and CCSE