[PATCH bpf v1 1/5] bpf: Fix kfunc register offset check for PTR_TO_BTF_ID

From: Kumar Kartikeya Dwivedi <memxor@gmail.com>
To: bpf@vger.kernel.org, Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Andrii Nakryiko <andrii@kernel.org>
Cc: Martin KaFai Lau <kafai@fb.com>
Subject: [PATCH bpf v1 1/5] bpf: Fix kfunc register offset check for PTR_TO_BTF_ID
Date: Sat, 19 Feb 2022 17:07:40 +0530	[thread overview]
Message-ID: <20220219113744.1852259-2-memxor@gmail.com> (raw)
In-Reply-To: <20220219113744.1852259-1-memxor@gmail.com>

When kfunc support was added, the ptr_to_ctx block did check_ctx_reg,
but the block checking PTR_TO_BTF_ID only did a btf_struct_ids_match.
This meant that when using a variable offset to the PTR_TO_BTF_ID, we
could pass it and make the kernel think the type at offset matches.

Commit 6788ab23508b ("bpf: Generally fix helper register offset check")
made some fixes in this area, and generalised these checks to prevent
future problem.

In case of helpers, __check_ptr_off_reg is used to reject this case in
check_func_arg.

Make this function argument register offset checking more generic, by
extracting the code out into a common helper, and calling it from both
helper and kfunc code paths. For consistency, also do the check from
check_mem_reg, even though it shouldn't be a problem there, because the
types permitted by check_helper_mem_access do allow variable and fixed
offsets, but a future refactoring may change such assumption.

In case of ptr_to_mem_ok block, we do allow NULL pointers, patching them
as non-NULL for call verification purposes, hence since the register
pointer is passed into this check_func_arg_reg_off function, the check
needs to happen inside check_mem_reg.

While we are at it, finally reject the cases of reg->off < 0 early.
fixed_off_ok is only ever set for the case of PTR_TO_BTF_ID when we
reach __check_ptr_off_reg, and negative offset in any case is incorrect.
This frees later checks the burden of sanitizing the offset when doing
type matching. This also leads to nicer verifier error than something
confusing like:
...
16: (07) r1 += -4 ; R1_w=ptr_prog_test_ref_kfunc(id=0,ref_obj_id=2,off=-4,imm=0) refs=2
17: (85) call bpf_kfunc_call_test_release#118834
access beyond struct prog_test_ref_kfunc at off 4294967292 size 1

A reason to do this check_func_arg_reg_off call in each block instead of
once for btf_check_func_arg_match, is because the verifier errors would
be confusing (instead of argument type not supported, something about
the offset).

Cc: Martin KaFai Lau <kafai@fb.com>
Fixes: e6ac2450d6de ("bpf: Support bpf program calling kernel function")
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 include/linux/bpf_verifier.h |  3 ++
 kernel/bpf/btf.c             | 17 ++++++-
 kernel/bpf/verifier.c        | 89 +++++++++++++++++++++++-------------
 3 files changed, 77 insertions(+), 32 deletions(-)

diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
index e9993172f892..f657c8ce01b8 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -519,6 +519,9 @@ bpf_prog_offload_replace_insn(struct bpf_verifier_env *env, u32 off,
 void
 bpf_prog_offload_remove_insns(struct bpf_verifier_env *env, u32 off, u32 cnt);
 
+int check_func_arg_reg_off(struct bpf_verifier_env *env,
+			   const struct bpf_reg_state *reg, int regno,
+			   bool arg_alloc_mem);
 int check_ptr_off_reg(struct bpf_verifier_env *env,
 		      const struct bpf_reg_state *reg, int regno);
 int check_mem_reg(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 3e23b3fa79ff..9c8c429aa4dc 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -5686,7 +5686,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 					i, btf_type_str(t));
 				return -EINVAL;
 			}
-			if (check_ptr_off_reg(env, reg, regno))
+			if (check_func_arg_reg_off(env, reg, regno, false))
 				return -EINVAL;
 		} else if (is_kfunc && (reg->type == PTR_TO_BTF_ID ||
 			   (reg2btf_ids[base_type(reg->type)] && !type_flag(reg->type)))) {
@@ -5714,6 +5714,16 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 							    &reg_ref_id);
 			reg_ref_tname = btf_name_by_offset(reg_btf,
 							   reg_ref_t->name_off);
+			/* In case of PTR_TO_SOCKET, PTR_TO_SOCK_COMMON,
+			 * PTR_TO_TCP_SOCK, we do type check using BTF IDs of
+			 * in-kernel types they point to, but
+			 * check_func_arg_reg_off using original register type,
+			 * as for them fixed offset case must be disallowed.
+			 * In case of PTR_TO_BTF_ID, check_func_arg_reg_off will
+			 * allow having a reg->off >= 0 fixed offset.
+			 */
+			if (check_func_arg_reg_off(env, reg, regno, false))
+				return -EINVAL;
 			if (!btf_struct_ids_match(log, reg_btf, reg_ref_id,
 						  reg->off, btf, ref_id)) {
 				bpf_log(log, "kernel function %s args#%d expected pointer to %s %s but R%d has a pointer to %s %s\n",
@@ -5724,6 +5734,10 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 				return -EINVAL;
 			}
 		} else if (ptr_to_mem_ok) {
+			/* All check_func_arg_reg_off checks happen inside
+			 * check_mem_reg, because the reg->type needs to be
+			 * cleared of PTR_MAYBE_NULL before the check is done.
+			 */
 			const struct btf_type *resolve_ret;
 			u32 type_size;
 
@@ -5750,6 +5764,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 				return -EINVAL;
 			}
 
+			/* This does the check_func_arg_reg_off call */
 			if (check_mem_reg(env, reg, regno, type_size))
 				return -EINVAL;
 		} else {
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index a39eedecc93a..732dcba85ce5 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -3979,6 +3979,12 @@ static int __check_ptr_off_reg(struct bpf_verifier_env *env,
 	 * is only allowed in its original, unmodified form.
 	 */
 
+	if (reg->off < 0) {
+		verbose(env, "negative offset %s ptr R%d off=%d disallowed\n",
+			reg_type_str(env, reg->type), regno, reg->off);
+		return -EACCES;
+	}
+
 	if (!fixed_off_ok && reg->off) {
 		verbose(env, "dereference of modified %s ptr R%d off=%d disallowed\n",
 			reg_type_str(env, reg->type), regno, reg->off);
@@ -4880,6 +4886,8 @@ static int check_helper_mem_access(struct bpf_verifier_env *env, int regno,
 int check_mem_reg(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
 		   u32 regno, u32 mem_size)
 {
+	int rv;
+
 	if (register_is_null(reg))
 		return 0;
 
@@ -4889,15 +4897,16 @@ int check_mem_reg(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
 		 * the conversion shouldn't be visible to a caller.
 		 */
 		const struct bpf_reg_state saved_reg = *reg;
-		int rv;
 
 		mark_ptr_not_null_reg(reg);
-		rv = check_helper_mem_access(env, regno, mem_size, true, NULL);
+		rv = check_func_arg_reg_off(env, reg, regno, false);
+		rv = rv ?: check_helper_mem_access(env, regno, mem_size, true, NULL);
 		*reg = saved_reg;
 		return rv;
 	}
 
-	return check_helper_mem_access(env, regno, mem_size, true, NULL);
+	rv = check_func_arg_reg_off(env, reg, regno, false);
+	return rv ?: check_helper_mem_access(env, regno, mem_size, true, NULL);
 }
 
 /* Implementation details:
@@ -5255,11 +5264,54 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno,
 				kernel_type_name(btf_vmlinux, *arg_btf_id));
 			return -EACCES;
 		}
+
+		/* var_off check happens later in check_func_arg_reg_off */
 	}
 
 	return 0;
 }
 
+/* Caller ensures reg->type does not have PTR_MAYBE_NULL */
+int check_func_arg_reg_off(struct bpf_verifier_env *env,
+			   const struct bpf_reg_state *reg, int regno,
+			   bool arg_alloc_mem)
+{
+	enum bpf_reg_type type = reg->type;
+	int err;
+
+	WARN_ON_ONCE(type & PTR_MAYBE_NULL);
+
+	switch ((u32)type) {
+	case SCALAR_VALUE:
+	/* Pointer types where reg offset is explicitly allowed: */
+	case PTR_TO_PACKET:
+	case PTR_TO_PACKET_META:
+	case PTR_TO_MAP_KEY:
+	case PTR_TO_MAP_VALUE:
+	case PTR_TO_MEM:
+	case PTR_TO_MEM | MEM_RDONLY:
+	case PTR_TO_MEM | MEM_ALLOC:
+	case PTR_TO_BUF:
+	case PTR_TO_BUF | MEM_RDONLY:
+	case PTR_TO_STACK:
+		/* Some of the argument types nevertheless require a
+		 * zero register offset.
+		 */
+		if (arg_alloc_mem)
+			goto force_off_check;
+		break;
+	/* All the rest must be rejected: */
+	default:
+force_off_check:
+		err = __check_ptr_off_reg(env, reg, regno,
+					  type == PTR_TO_BTF_ID);
+		if (err < 0)
+			return err;
+		break;
+	}
+	return 0;
+}
+
 static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
 			  struct bpf_call_arg_meta *meta,
 			  const struct bpf_func_proto *fn)
@@ -5309,34 +5361,9 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
 	if (err)
 		return err;
 
-	switch ((u32)type) {
-	case SCALAR_VALUE:
-	/* Pointer types where reg offset is explicitly allowed: */
-	case PTR_TO_PACKET:
-	case PTR_TO_PACKET_META:
-	case PTR_TO_MAP_KEY:
-	case PTR_TO_MAP_VALUE:
-	case PTR_TO_MEM:
-	case PTR_TO_MEM | MEM_RDONLY:
-	case PTR_TO_MEM | MEM_ALLOC:
-	case PTR_TO_BUF:
-	case PTR_TO_BUF | MEM_RDONLY:
-	case PTR_TO_STACK:
-		/* Some of the argument types nevertheless require a
-		 * zero register offset.
-		 */
-		if (arg_type == ARG_PTR_TO_ALLOC_MEM)
-			goto force_off_check;
-		break;
-	/* All the rest must be rejected: */
-	default:
-force_off_check:
-		err = __check_ptr_off_reg(env, reg, regno,
-					  type == PTR_TO_BTF_ID);
-		if (err < 0)
-			return err;
-		break;
-	}
+	err = check_func_arg_reg_off(env, reg, regno, arg_type == ARG_PTR_TO_ALLOC_MEM);
+	if (err < 0)
+		return err;
 
 skip_type_check:
 	if (reg->ref_obj_id) {
-- 
2.35.1

