Commit 83a28819 authored by Ilya Leoshkevich's avatar Ilya Leoshkevich Committed by Daniel Borkmann
Browse files

bpf: Account for BPF_FETCH in insn_has_def32() 

insn_has_def32() returns false for 32-bit BPF_FETCH insns. This makes
adjust_insn_aux_data() incorrectly set zext_dst, as can be seen in [1].
This happens because insn_no_def() does not know about the BPF_FETCH
variants of BPF_STX.

Fix in two steps.

First, replace insn_no_def() with insn_def_regno(), which returns the
register an insn defines. Normally insn_no_def() calls are followed by
insn->dst_reg uses; replace those with the insn_def_regno() return
value.

Second, adjust the BPF_STX special case in is_reg64() to deal with
queries made from opt_subreg_zext_lo32_rnd_hi32(), where the state
information is no longer available. Add a comment, since the purpose
of this special case is not clear at first glance.

  [1] https://lore.kernel.org/bpf/20210223150845.1857620-1-jackmanb@google.com/



Fixes: 5ffa2550 ("bpf: Add instructions for atomic_[cmp]xchg")
Signed-off-by: default avatarIlya Leoshkevich <iii@linux.ibm.com>
Signed-off-by: default avatarDaniel Borkmann <daniel@iogearbox.net>
Acked-by: default avatarMartin KaFai Lau <kafai@fb.com>
Acked-by: default avatarBrendan Jackman <jackmanb@google.com>
Link: https://lore.kernel.org/bpf/20210301154019.129110-1-iii@linux.ibm.com
parent 2b2aedab

kernel/bpf/verifier.c

+39 −31
Original line number Diff line number Diff line
@@ -1703,7 +1703,11 @@ static bool is_reg64(struct bpf_verifier_env *env, struct bpf_insn *insn, 
	}



	if (class == BPF_STX) {

		if (reg->type != SCALAR_VALUE)

		/* BPF_STX (including atomic variants) has multiple source

		 * operands, one of which is a ptr. Check whether the caller is

		 * asking about it.

		 */

		if (t == SRC_OP && reg->type != SCALAR_VALUE)

			return true;

		return BPF_SIZE(code) == BPF_DW;

	}
@@ -1735,22 +1739,38 @@ static bool is_reg64(struct bpf_verifier_env *env, struct bpf_insn *insn, 
	return true;

}



/* Return TRUE if INSN doesn't have explicit value define. */

static bool insn_no_def(struct bpf_insn *insn)

/* Return the regno defined by the insn, or -1. */

static int insn_def_regno(const struct bpf_insn *insn)

{

	u8 class = BPF_CLASS(insn->code);



	return (class == BPF_JMP || class == BPF_JMP32 ||

		class == BPF_STX || class == BPF_ST);

	switch (BPF_CLASS(insn->code)) {

	case BPF_JMP:

	case BPF_JMP32:

	case BPF_ST:

		return -1;

	case BPF_STX:

		if (BPF_MODE(insn->code) == BPF_ATOMIC &&

		    (insn->imm & BPF_FETCH)) {

			if (insn->imm == BPF_CMPXCHG)

				return BPF_REG_0;

			else

				return insn->src_reg;

		} else {

			return -1;

		}

	default:

		return insn->dst_reg;

	}

}



/* Return TRUE if INSN has defined any 32-bit value explicitly. */

static bool insn_has_def32(struct bpf_verifier_env *env, struct bpf_insn *insn)

{

	if (insn_no_def(insn))

	int dst_reg = insn_def_regno(insn);



	if (dst_reg == -1)

		return false;



	return !is_reg64(env, insn, insn->dst_reg, NULL, DST_OP);

	return !is_reg64(env, insn, dst_reg, NULL, DST_OP);

}



static void mark_insn_zext(struct bpf_verifier_env *env,
@@ -11006,9 +11026,10 @@ static int opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env, 
	for (i = 0; i < len; i++) {

		int adj_idx = i + delta;

		struct bpf_insn insn;

		u8 load_reg;

		int load_reg;



		insn = insns[adj_idx];

		load_reg = insn_def_regno(&insn);

		if (!aux[adj_idx].zext_dst) {

			u8 code, class;

			u32 imm_rnd;
@@ -11018,14 +11039,14 @@ static int opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env, 


			code = insn.code;

			class = BPF_CLASS(code);

			if (insn_no_def(&insn))

			if (load_reg == -1)

				continue;



			/* NOTE: arg "reg" (the fourth one) is only used for

			 *       BPF_STX which has been ruled out in above

			 *       check, it is safe to pass NULL here.

			 *       BPF_STX + SRC_OP, so it is safe to pass NULL

			 *       here.

			 */

			if (is_reg64(env, &insn, insn.dst_reg, NULL, DST_OP)) {

			if (is_reg64(env, &insn, load_reg, NULL, DST_OP)) {

				if (class == BPF_LD &&

				    BPF_MODE(code) == BPF_IMM)

					i++;
@@ -11040,7 +11061,7 @@ static int opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env, 
			imm_rnd = get_random_int();

			rnd_hi32_patch[0] = insn;

			rnd_hi32_patch[1].imm = imm_rnd;

			rnd_hi32_patch[3].dst_reg = insn.dst_reg;

			rnd_hi32_patch[3].dst_reg = load_reg;

			patch = rnd_hi32_patch;

			patch_len = 4;

			goto apply_patch_buffer;
@@ -11049,22 +11070,9 @@ static int opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env, 
		if (!bpf_jit_needs_zext())

			continue;



		/* zext_dst means that we want to zero-extend whatever register

		 * the insn defines, which is dst_reg most of the time, with

		 * the notable exception of BPF_STX + BPF_ATOMIC + BPF_FETCH.

		 */

		if (BPF_CLASS(insn.code) == BPF_STX &&

		    BPF_MODE(insn.code) == BPF_ATOMIC) {

			/* BPF_STX + BPF_ATOMIC insns without BPF_FETCH do not

			 * define any registers, therefore zext_dst cannot be

			 * set.

			 */

			if (WARN_ON(!(insn.imm & BPF_FETCH)))

				return -EINVAL;

			load_reg = insn.imm == BPF_CMPXCHG ? BPF_REG_0

							   : insn.src_reg;

		} else {

			load_reg = insn.dst_reg;

		if (WARN_ON(load_reg == -1)) {

			verbose(env, "verifier bug. zext_dst is set, but no reg is defined\n");

			return -EFAULT;

		}



		zext_patch[0] = insn;