Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit bab89add authored by Daniel Borkmann's avatar Daniel Borkmann
Browse files

Merge branch 'bpf-improve-verifier-state-analysis' 



Alexei Starovoitov says:

====================
v1->v2:
With optimization suggested by Jakub patch 4 safety check became
cheap enough.

Several improvements to verifier state logic.
Patch 1 - trivial optimization
Patch 3 - significant optimization for stack state equivalence
Patch 4 - safety check for liveness and prep for future state merging
====================

Signed-off-by: default avatarDaniel Borkmann <daniel@iogearbox.net>
parents eb415c98 9242b5f5

include/linux/bpf_verifier.h

+1 −0
Original line number Diff line number Diff line
@@ -38,6 +38,7 @@ enum bpf_reg_liveness { 
	REG_LIVE_NONE = 0, /* reg hasn't been read or written this branch */

	REG_LIVE_READ, /* reg was read, so we're sensitive to initial value */

	REG_LIVE_WRITTEN, /* reg was written first, screening off later reads */

	REG_LIVE_DONE = 4, /* liveness won't be updating this register anymore */

};



struct bpf_reg_state {

kernel/bpf/verifier.c

+117 −8
Original line number Diff line number Diff line
@@ -397,12 +397,14 @@ static char slot_type_char[] = { 
static void print_liveness(struct bpf_verifier_env *env,

			   enum bpf_reg_liveness live)

{

	if (live & (REG_LIVE_READ | REG_LIVE_WRITTEN))

	if (live & (REG_LIVE_READ | REG_LIVE_WRITTEN | REG_LIVE_DONE))

	    verbose(env, "_");

	if (live & REG_LIVE_READ)

		verbose(env, "r");

	if (live & REG_LIVE_WRITTEN)

		verbose(env, "w");

	if (live & REG_LIVE_DONE)

		verbose(env, "D");

}



static struct bpf_func_state *func(struct bpf_verifier_env *env,
@@ -1132,6 +1134,12 @@ static int mark_reg_read(struct bpf_verifier_env *env, 
		/* if read wasn't screened by an earlier write ... */

		if (writes && state->live & REG_LIVE_WRITTEN)

			break;

		if (parent->live & REG_LIVE_DONE) {

			verbose(env, "verifier BUG type %s var_off %lld off %d\n",

				reg_type_str[parent->type],

				parent->var_off.value, parent->off);

			return -EFAULT;

		}

		/* ... then we depend on parent's value */

		parent->live |= REG_LIVE_READ;

		state = parent;
@@ -5078,6 +5086,102 @@ static bool check_ids(u32 old_id, u32 cur_id, struct idpair *idmap) 
	return false;

}



static void clean_func_state(struct bpf_verifier_env *env,

			     struct bpf_func_state *st)

{

	enum bpf_reg_liveness live;

	int i, j;



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

		live = st->regs[i].live;

		/* liveness must not touch this register anymore */

		st->regs[i].live |= REG_LIVE_DONE;

		if (!(live & REG_LIVE_READ))

			/* since the register is unused, clear its state

			 * to make further comparison simpler

			 */

			__mark_reg_not_init(&st->regs[i]);

	}



	for (i = 0; i < st->allocated_stack / BPF_REG_SIZE; i++) {

		live = st->stack[i].spilled_ptr.live;

		/* liveness must not touch this stack slot anymore */

		st->stack[i].spilled_ptr.live |= REG_LIVE_DONE;

		if (!(live & REG_LIVE_READ)) {

			__mark_reg_not_init(&st->stack[i].spilled_ptr);

			for (j = 0; j < BPF_REG_SIZE; j++)

				st->stack[i].slot_type[j] = STACK_INVALID;

		}

	}

}



static void clean_verifier_state(struct bpf_verifier_env *env,

				 struct bpf_verifier_state *st)

{

	int i;



	if (st->frame[0]->regs[0].live & REG_LIVE_DONE)

		/* all regs in this state in all frames were already marked */

		return;



	for (i = 0; i <= st->curframe; i++)

		clean_func_state(env, st->frame[i]);

}



/* the parentage chains form a tree.

 * the verifier states are added to state lists at given insn and

 * pushed into state stack for future exploration.

 * when the verifier reaches bpf_exit insn some of the verifer states

 * stored in the state lists have their final liveness state already,

 * but a lot of states will get revised from liveness point of view when

 * the verifier explores other branches.

 * Example:

 * 1: r0 = 1

 * 2: if r1 == 100 goto pc+1

 * 3: r0 = 2

 * 4: exit

 * when the verifier reaches exit insn the register r0 in the state list of

 * insn 2 will be seen as !REG_LIVE_READ. Then the verifier pops the other_branch

 * of insn 2 and goes exploring further. At the insn 4 it will walk the

 * parentage chain from insn 4 into insn 2 and will mark r0 as REG_LIVE_READ.

 *

 * Since the verifier pushes the branch states as it sees them while exploring

 * the program the condition of walking the branch instruction for the second

 * time means that all states below this branch were already explored and

 * their final liveness markes are already propagated.

 * Hence when the verifier completes the search of state list in is_state_visited()

 * we can call this clean_live_states() function to mark all liveness states

 * as REG_LIVE_DONE to indicate that 'parent' pointers of 'struct bpf_reg_state'

 * will not be used.

 * This function also clears the registers and stack for states that !READ

 * to simplify state merging.

 *

 * Important note here that walking the same branch instruction in the callee

 * doesn't meant that the states are DONE. The verifier has to compare

 * the callsites

 */

static void clean_live_states(struct bpf_verifier_env *env, int insn,

			      struct bpf_verifier_state *cur)

{

	struct bpf_verifier_state_list *sl;

	int i;



	sl = env->explored_states[insn];

	if (!sl)

		return;



	while (sl != STATE_LIST_MARK) {

		if (sl->state.curframe != cur->curframe)

			goto next;

		for (i = 0; i <= cur->curframe; i++)

			if (sl->state.frame[i]->callsite != cur->frame[i]->callsite)

				goto next;

		clean_verifier_state(env, &sl->state);

next:

		sl = sl->next;

	}

}



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

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

		    struct idpair *idmap)
@@ -5191,12 +5295,6 @@ static bool stacksafe(struct bpf_func_state *old, 
{

	int i, spi;



	/* if explored stack has more populated slots than current stack

	 * such stacks are not equivalent

	 */

	if (old->allocated_stack > cur->allocated_stack)

		return false;



	/* walk slots of the explored stack and ignore any additional

	 * slots in the current stack, since explored(safe) state

	 * didn't use them
@@ -5204,12 +5302,21 @@ static bool stacksafe(struct bpf_func_state *old, 
	for (i = 0; i < old->allocated_stack; i++) {

		spi = i / BPF_REG_SIZE;



		if (!(old->stack[spi].spilled_ptr.live & REG_LIVE_READ))

		if (!(old->stack[spi].spilled_ptr.live & REG_LIVE_READ)) {

			i += BPF_REG_SIZE - 1;

			/* explored state didn't use this */

			continue;

		}



		if (old->stack[spi].slot_type[i % BPF_REG_SIZE] == STACK_INVALID)

			continue;



		/* explored stack has more populated slots than current stack

		 * and these slots were used

		 */

		if (i >= cur->allocated_stack)

			return false;



		/* if old state was safe with misc data in the stack

		 * it will be safe with zero-initialized stack.

		 * The opposite is not true
@@ -5393,6 +5500,8 @@ static int is_state_visited(struct bpf_verifier_env *env, int insn_idx) 
		 */

		return 0;



	clean_live_states(env, insn_idx, cur);



	while (sl != STATE_LIST_MARK) {

		if (states_equal(env, &sl->state, cur)) {

			/* reached equivalent register/stack state,

tools/testing/selftests/bpf/test_verifier.c

+36 −1
Original line number Diff line number Diff line
@@ -76,7 +76,7 @@ struct bpf_test { 
	int fixup_percpu_cgroup_storage[MAX_FIXUPS];

	const char *errstr;

	const char *errstr_unpriv;

	uint32_t retval, retval_unpriv;

	uint32_t retval, retval_unpriv, insn_processed;

	enum {

		UNDEF,

		ACCEPT,
@@ -13647,6 +13647,28 @@ static struct bpf_test tests[] = { 
		.prog_type = BPF_PROG_TYPE_SCHED_CLS,

		.result = ACCEPT,

	},

	{

		"allocated_stack",

		.insns = {

			BPF_ALU64_REG(BPF_MOV, BPF_REG_6, BPF_REG_1),

			BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_get_prandom_u32),

			BPF_ALU64_REG(BPF_MOV, BPF_REG_7, BPF_REG_0),

			BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 5),

			BPF_MOV64_IMM(BPF_REG_0, 0),

			BPF_STX_MEM(BPF_DW, BPF_REG_10, BPF_REG_6, -8),

			BPF_LDX_MEM(BPF_DW, BPF_REG_6, BPF_REG_10, -8),

			BPF_STX_MEM(BPF_B, BPF_REG_10, BPF_REG_7, -9),

			BPF_LDX_MEM(BPF_B, BPF_REG_7, BPF_REG_10, -9),

			BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, 0),

			BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, 0),

			BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, 0),

			BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, 0),

			BPF_EXIT_INSN(),

		},

		.result = ACCEPT,

		.result_unpriv = ACCEPT,

		.insn_processed = 15,

	},

	{

		"reference tracking in call: free reference in subprog and outside",

		.insns = {
@@ -14444,6 +14466,19 @@ static void do_test_single(struct bpf_test *test, bool unpriv, 
		}

	}



	if (test->insn_processed) {

		uint32_t insn_processed;

		char *proc;



		proc = strstr(bpf_vlog, "processed ");

		insn_processed = atoi(proc + 10);

		if (test->insn_processed != insn_processed) {

			printf("FAIL\nUnexpected insn_processed %u vs %u\n",

			       insn_processed, test->insn_processed);

			goto fail_log;

		}

	}



	if (fd_prog >= 0) {

		__u8 tmp[TEST_DATA_LEN << 2];

		__u32 size_tmp = sizeof(tmp);