Commit 8b7df50f authored by Luis Gerhorst's avatar Luis Gerhorst Committed by Alexei Starovoitov
Browse files

bpf: Move insn if/else into do_check_insn() 

This is required to catch the errors later and fall back to a nospec if
on a speculative path.

Eliminate the regs variable as it is only used once and insn_idx is not
modified in-between the definition and usage.

Do not pass insn but compute it in the function itself. As Eduard points
out [1], insn is assumed to correspond to env->insn_idx in many places
(e.g, __check_reg_arg()).

Move code into do_check_insn(), replace
* "continue" with "return 0" after modifying insn_idx
* "goto process_bpf_exit" with "return PROCESS_BPF_EXIT"
* "goto process_bpf_exit_full" with "return process_bpf_exit_full()"
* "do_print_state = " with "*do_print_state = "

[1] https://lore.kernel.org/all/293dbe3950a782b8eb3b87b71d7a967e120191fd.camel@gmail.com/



Signed-off-by: default avatarLuis Gerhorst <luis.gerhorst@fau.de>
Acked-by: default avatarKumar Kartikeya Dwivedi <memxor@gmail.com>
Acked-by: default avatarHenriette Herzog <henriette.herzog@rub.de>
Cc: Maximilian Ott <ott@cs.fau.de>
Cc: Milan Stephan <milan.stephan@fau.de>
Link: https://lore.kernel.org/r/20250603205800.334980-2-luis.gerhorst@fau.de


Signed-off-by: default avatarAlexei Starovoitov <ast@kernel.org>
parent 2bc0575f

kernel/bpf/verifier.c

+223 −205
Original line number Diff line number Diff line
@@ -19430,106 +19430,58 @@ static int save_aux_ptr_type(struct bpf_verifier_env *env, enum bpf_reg_type typ 
	return 0;

}









static int do_check(struct bpf_verifier_env *env)









{









	bool pop_log = !(env->log.level & BPF_LOG_LEVEL2);









	struct bpf_verifier_state *state = env->cur_state;









	struct bpf_insn *insns = env->prog->insnsi;









	struct bpf_reg_state *regs;









	int insn_cnt = env->prog->len;









	bool do_print_state = false;









	int prev_insn_idx = -1;

















	for (;;) {









		bool exception_exit = false;









		struct bpf_insn *insn;









		u8 class;









		int err;

















		/* reset current history entry on each new instruction */









		env->cur_hist_ent = NULL;

















		env->prev_insn_idx = prev_insn_idx;









		if (env->insn_idx >= insn_cnt) {









			verbose(env, "invalid insn idx %d insn_cnt %d\n",









				env->insn_idx, insn_cnt);









			return -EFAULT;









		}

















		insn = &insns[env->insn_idx];









		class = BPF_CLASS(insn->code);

















		if (++env->insn_processed > BPF_COMPLEXITY_LIMIT_INSNS) {









			verbose(env,









				"BPF program is too large. Processed %d insn\n",









				env->insn_processed);









			return -E2BIG;









		}

















		state->last_insn_idx = env->prev_insn_idx;

















		if (is_prune_point(env, env->insn_idx)) {









			err = is_state_visited(env, env->insn_idx);









			if (err < 0)









				return err;









			if (err == 1) {









				/* found equivalent state, can prune the search */









				if (env->log.level & BPF_LOG_LEVEL) {









					if (do_print_state)









						verbose(env, "\nfrom %d to %d%s: safe\n",









							env->prev_insn_idx, env->insn_idx,









							env->cur_state->speculative ?









							" (speculative execution)" : "");









					else









						verbose(env, "%d: safe\n", env->insn_idx);









				}









				goto process_bpf_exit;









			}









		}









enum {









	PROCESS_BPF_EXIT = 1









};

















		if (is_jmp_point(env, env->insn_idx)) {









			err = push_insn_history(env, state, 0, 0);









static int process_bpf_exit_full(struct bpf_verifier_env *env,









				 bool *do_print_state,









				 bool exception_exit)









{









	/* We must do check_reference_leak here before









	 * prepare_func_exit to handle the case when









	 * state->curframe > 0, it may be a callback function,









	 * for which reference_state must match caller reference









	 * state when it exits.









	 */









	int err = check_resource_leak(env, exception_exit,









				      !env->cur_state->curframe,









				      "BPF_EXIT instruction in main prog");









	if (err)









		return err;









		}

















		if (signal_pending(current))









			return -EAGAIN;

















		if (need_resched())









			cond_resched();

















		if (env->log.level & BPF_LOG_LEVEL2 && do_print_state) {









			verbose(env, "\nfrom %d to %d%s:",









				env->prev_insn_idx, env->insn_idx,









				env->cur_state->speculative ?









				" (speculative execution)" : "");









			print_verifier_state(env, state, state->curframe, true);









			do_print_state = false;









		}

















		if (env->log.level & BPF_LOG_LEVEL) {









			if (verifier_state_scratched(env))









				print_insn_state(env, state, state->curframe);









	/* The side effect of the prepare_func_exit which is









	 * being skipped is that it frees bpf_func_state.









	 * Typically, process_bpf_exit will only be hit with









	 * outermost exit. copy_verifier_state in pop_stack will









	 * handle freeing of any extra bpf_func_state left over









	 * from not processing all nested function exits. We









	 * also skip return code checks as they are not needed









	 * for exceptional exits.









	 */









	if (exception_exit)









		return PROCESS_BPF_EXIT;

















			verbose_linfo(env, env->insn_idx, "; ");









			env->prev_log_pos = env->log.end_pos;









			verbose(env, "%d: ", env->insn_idx);









			verbose_insn(env, insn);









			env->prev_insn_print_pos = env->log.end_pos - env->prev_log_pos;









			env->prev_log_pos = env->log.end_pos;









	if (env->cur_state->curframe) {









		/* exit from nested function */









		err = prepare_func_exit(env, &env->insn_idx);









		if (err)









			return err;









		*do_print_state = true;









		return 0;









	}

















		if (bpf_prog_is_offloaded(env->prog->aux)) {









			err = bpf_prog_offload_verify_insn(env, env->insn_idx,









							   env->prev_insn_idx);









	err = check_return_code(env, BPF_REG_0, "R0");









	if (err)









		return err;









	return PROCESS_BPF_EXIT;









}

















		regs = cur_regs(env);









		sanitize_mark_insn_seen(env);









		prev_insn_idx = env->insn_idx;









static int do_check_insn(struct bpf_verifier_env *env, bool *do_print_state)









{









	int err;









	struct bpf_insn *insn = &env->prog->insnsi[env->insn_idx];









	u8 class = BPF_CLASS(insn->code);

















	if (class == BPF_ALU || class == BPF_ALU64) {









		err = check_alu_op(env, insn);








@@ -19542,8 +19494,7 @@ static int do_check(struct bpf_verifier_env *env)







		/* Check for reserved fields is already done in









		 * resolve_pseudo_ldimm64().









		 */









			err = check_load_mem(env, insn, false, is_ldsx, true,









					     "ldx");









		err = check_load_mem(env, insn, false, is_ldsx, true, "ldx");









		if (err)









			return err;









	} else if (class == BPF_STX) {








@@ -19552,7 +19503,7 @@ static int do_check(struct bpf_verifier_env *env)







			if (err)









				return err;









			env->insn_idx++;









				continue;









			return 0;









		}

















		if (BPF_MODE(insn->code) != BPF_MEM || insn->imm != 0) {








@@ -19576,7 +19527,7 @@ static int do_check(struct bpf_verifier_env *env)







		if (err)









			return err;

















			dst_reg_type = regs[insn->dst_reg].type;









		dst_reg_type = cur_regs(env)[insn->dst_reg].type;

















		/* check that memory (dst_reg + off) is writeable */









		err = check_mem_access(env, env->insn_idx, insn->dst_reg,








@@ -19594,22 +19545,23 @@ static int do_check(struct bpf_verifier_env *env)







		env->jmps_processed++;









		if (opcode == BPF_CALL) {









			if (BPF_SRC(insn->code) != BPF_K ||









				    (insn->src_reg != BPF_PSEUDO_KFUNC_CALL









				     && insn->off != 0) ||









			    (insn->src_reg != BPF_PSEUDO_KFUNC_CALL &&









			     insn->off != 0) ||









			    (insn->src_reg != BPF_REG_0 &&









			     insn->src_reg != BPF_PSEUDO_CALL &&









			     insn->src_reg != BPF_PSEUDO_KFUNC_CALL) ||









				    insn->dst_reg != BPF_REG_0 ||









				    class == BPF_JMP32) {









			    insn->dst_reg != BPF_REG_0 || class == BPF_JMP32) {









				verbose(env, "BPF_CALL uses reserved fields\n");









				return -EINVAL;









			}

















			if (env->cur_state->active_locks) {









					if ((insn->src_reg == BPF_REG_0 && insn->imm != BPF_FUNC_spin_unlock) ||









				if ((insn->src_reg == BPF_REG_0 &&









				     insn->imm != BPF_FUNC_spin_unlock) ||









				    (insn->src_reg == BPF_PSEUDO_KFUNC_CALL &&









				     (insn->off != 0 || !kfunc_spin_allowed(insn->imm)))) {









						verbose(env, "function calls are not allowed while holding a lock\n");









					verbose(env,









						"function calls are not allowed while holding a lock\n");









					return -EINVAL;









				}









			}








@@ -19617,10 +19569,8 @@ static int do_check(struct bpf_verifier_env *env)







				err = check_func_call(env, insn, &env->insn_idx);









			} else if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL) {









				err = check_kfunc_call(env, insn, &env->insn_idx);









					if (!err && is_bpf_throw_kfunc(insn)) {









						exception_exit = true;









						goto process_bpf_exit_full;









					}









				if (!err && is_bpf_throw_kfunc(insn))









					return process_bpf_exit_full(env, do_print_state, true);









			} else {









				err = check_helper_call(env, insn, &env->insn_idx);









			}








@@ -19642,8 +19592,7 @@ static int do_check(struct bpf_verifier_env *env)







				env->insn_idx += insn->off + 1;









			else









				env->insn_idx += insn->imm + 1;









				continue;

















			return 0;









		} else if (opcode == BPF_EXIT) {









			if (BPF_SRC(insn->code) != BPF_K ||









			    insn->imm != 0 ||








@@ -19653,59 +19602,7 @@ static int do_check(struct bpf_verifier_env *env)







				verbose(env, "BPF_EXIT uses reserved fields\n");









				return -EINVAL;









			}









process_bpf_exit_full:









				/* We must do check_reference_leak here before









				 * prepare_func_exit to handle the case when









				 * state->curframe > 0, it may be a callback









				 * function, for which reference_state must









				 * match caller reference state when it exits.









				 */









				err = check_resource_leak(env, exception_exit, !env->cur_state->curframe,









							  "BPF_EXIT instruction in main prog");









				if (err)









					return err;

















				/* The side effect of the prepare_func_exit









				 * which is being skipped is that it frees









				 * bpf_func_state. Typically, process_bpf_exit









				 * will only be hit with outermost exit.









				 * copy_verifier_state in pop_stack will handle









				 * freeing of any extra bpf_func_state left over









				 * from not processing all nested function









				 * exits. We also skip return code checks as









				 * they are not needed for exceptional exits.









				 */









				if (exception_exit)









					goto process_bpf_exit;

















				if (state->curframe) {









					/* exit from nested function */









					err = prepare_func_exit(env, &env->insn_idx);









					if (err)









						return err;









					do_print_state = true;









					continue;









				}

















				err = check_return_code(env, BPF_REG_0, "R0");









				if (err)









					return err;









process_bpf_exit:









				mark_verifier_state_scratched(env);









				update_branch_counts(env, env->cur_state);









				err = pop_stack(env, &prev_insn_idx,









						&env->insn_idx, pop_log);









				if (err < 0) {









					if (err != -ENOENT)









						return err;









					break;









				} else {









					if (verifier_bug_if(env->cur_state->loop_entry, env,









							    "broken loop detection"))









						return -EFAULT;









					do_print_state = true;









					continue;









				}









			return process_bpf_exit_full(env, do_print_state, false);









		} else {









			err = check_cond_jmp_op(env, insn, &env->insn_idx);









			if (err)








@@ -19736,6 +19633,127 @@ static int do_check(struct bpf_verifier_env *env)







	}

















	env->insn_idx++;









	return 0;









}

















static int do_check(struct bpf_verifier_env *env)









{









	bool pop_log = !(env->log.level & BPF_LOG_LEVEL2);









	struct bpf_verifier_state *state = env->cur_state;









	struct bpf_insn *insns = env->prog->insnsi;









	int insn_cnt = env->prog->len;









	bool do_print_state = false;









	int prev_insn_idx = -1;

















	for (;;) {









		struct bpf_insn *insn;









		int err;

















		/* reset current history entry on each new instruction */









		env->cur_hist_ent = NULL;

















		env->prev_insn_idx = prev_insn_idx;









		if (env->insn_idx >= insn_cnt) {









			verbose(env, "invalid insn idx %d insn_cnt %d\n",









				env->insn_idx, insn_cnt);









			return -EFAULT;









		}

















		insn = &insns[env->insn_idx];

















		if (++env->insn_processed > BPF_COMPLEXITY_LIMIT_INSNS) {









			verbose(env,









				"BPF program is too large. Processed %d insn\n",









				env->insn_processed);









			return -E2BIG;









		}

















		state->last_insn_idx = env->prev_insn_idx;

















		if (is_prune_point(env, env->insn_idx)) {









			err = is_state_visited(env, env->insn_idx);









			if (err < 0)









				return err;









			if (err == 1) {









				/* found equivalent state, can prune the search */









				if (env->log.level & BPF_LOG_LEVEL) {









					if (do_print_state)









						verbose(env, "\nfrom %d to %d%s: safe\n",









							env->prev_insn_idx, env->insn_idx,









							env->cur_state->speculative ?









							" (speculative execution)" : "");









					else









						verbose(env, "%d: safe\n", env->insn_idx);









				}









				goto process_bpf_exit;









			}









		}

















		if (is_jmp_point(env, env->insn_idx)) {









			err = push_insn_history(env, state, 0, 0);









			if (err)









				return err;









		}

















		if (signal_pending(current))









			return -EAGAIN;

















		if (need_resched())









			cond_resched();

















		if (env->log.level & BPF_LOG_LEVEL2 && do_print_state) {









			verbose(env, "\nfrom %d to %d%s:",









				env->prev_insn_idx, env->insn_idx,









				env->cur_state->speculative ?









				" (speculative execution)" : "");









			print_verifier_state(env, state, state->curframe, true);









			do_print_state = false;









		}

















		if (env->log.level & BPF_LOG_LEVEL) {









			if (verifier_state_scratched(env))









				print_insn_state(env, state, state->curframe);

















			verbose_linfo(env, env->insn_idx, "; ");









			env->prev_log_pos = env->log.end_pos;









			verbose(env, "%d: ", env->insn_idx);









			verbose_insn(env, insn);









			env->prev_insn_print_pos = env->log.end_pos - env->prev_log_pos;









			env->prev_log_pos = env->log.end_pos;









		}

















		if (bpf_prog_is_offloaded(env->prog->aux)) {









			err = bpf_prog_offload_verify_insn(env, env->insn_idx,









							   env->prev_insn_idx);









			if (err)









				return err;









		}

















		sanitize_mark_insn_seen(env);









		prev_insn_idx = env->insn_idx;

















		err = do_check_insn(env, &do_print_state);









		if (err < 0) {









			return err;









		} else if (err == PROCESS_BPF_EXIT) {









process_bpf_exit:









			mark_verifier_state_scratched(env);









			update_branch_counts(env, env->cur_state);









			err = pop_stack(env, &prev_insn_idx, &env->insn_idx,









					pop_log);









			if (err < 0) {









				if (err != -ENOENT)









					return err;









				break;









			} else {









				if (verifier_bug_if(env->cur_state->loop_entry, env,









						    "broken loop detection"))









					return -EFAULT;









				do_print_state = true;









				continue;









			}









		}









		WARN_ON_ONCE(err);









	}

















	return 0;