[PATCH bpf-next v3 0/8] Fixes for bad PTR_TO_BTF_ID offset

[PATCH bpf-next v3 0/8] Fixes for bad PTR_TO_BTF_ID offset

[Kumar Kartikeya Dwivedi]

This set fixes a bug related to bad var_off being permitted for kfunc call in
case of PTR_TO_BTF_ID, consolidates offset checks for all register types allowed
as helper or kfunc arguments into a common shared helper, and introduces a
couple of other checks to harden the kfunc release logic and prevent future
bugs. Some selftests are also included that fail in absence of these fixes,
serving as demonstration of the issues being fixed.

Changelog:
----------
v2 -> v3:
v2: https://lore.kernel.org/bpf/20220303045029.2645297-1-memxor@gmail.com

 * Add my SoB to __diag for clang patch (Nathan)

v1 -> v2:
v1: https://lore.kernel.org/bpf/20220301065745.1634848-1-memxor@gmail.com

 * Put reg->off check for release kfunc inside check_func_arg_reg_off,
   make the check a bit more readable
 * Squash verifier selftests errstr update into patch 3 for bisect (Alexei)
 * Include fix from Nathan for clang warning about missing prototypes
 * Add unified __diag_ingore_all that works for both GCC/LLVM (Alexei)

Older discussion:
Link: https://lore.kernel.org/bpf/20220219113744.1852259-1-memxor@gmail.com

Kumar Kartikeya Dwivedi (7):
  bpf: Add check_func_arg_reg_off function
  bpf: Fix PTR_TO_BTF_ID var_off check
  bpf: Disallow negative offset in check_ptr_off_reg
  bpf: Harden register offset checks for release helpers and kfuncs
  compiler_types.h: Add unified __diag_ignore_all for GCC/LLVM
  bpf: Replace __diag_ignore with unified __diag_ignore_all
  selftests/bpf: Add tests for kfunc register offset checks

Nathan Chancellor (1):
  compiler-clang.h: Add __diag infrastructure for clang

 include/linux/bpf_verifier.h                  |  4 +
 include/linux/compiler-clang.h                | 25 +++++
 include/linux/compiler-gcc.h                  |  3 +
 include/linux/compiler_types.h                |  4 +
 kernel/bpf/btf.c                              | 20 ++--
 kernel/bpf/verifier.c                         | 94 +++++++++++++------
 net/bpf/test_run.c                            | 15 ++-
 net/netfilter/nf_conntrack_bpf.c              |  5 +-
 .../selftests/bpf/verifier/bounds_deduction.c |  2 +-
 tools/testing/selftests/bpf/verifier/calls.c  | 83 ++++++++++++++++
 tools/testing/selftests/bpf/verifier/ctx.c    |  8 +-
 11 files changed, 220 insertions(+), 43 deletions(-)

-- 
2.35.1

[PATCH bpf-next v3 1/8] bpf: Add check_func_arg_reg_off function

[Kumar Kartikeya Dwivedi]

Lift the list of register types allowed for having fixed and variable
offsets when passed as helper function arguments into a common helper,
so that they can be reused for kfunc checks in later commits. Keeping a
common helper aids maintainability and allows us to follow the same
consistent rules across helpers and kfuncs. Also, convert check_func_arg
to use this function.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 include/linux/bpf_verifier.h |  3 ++
 kernel/bpf/verifier.c        | 69 +++++++++++++++++++++---------------
 2 files changed, 44 insertions(+), 28 deletions(-)

diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
index 7a7be8c057f2..38b24ee8d8c2 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -521,6 +521,9 @@ bpf_prog_offload_remove_insns(struct bpf_verifier_env *env, u32 off, u32 cnt);
 
 int check_ptr_off_reg(struct bpf_verifier_env *env,
 		      const struct bpf_reg_state *reg, int regno);
+int check_func_arg_reg_off(struct bpf_verifier_env *env,
+			   const struct bpf_reg_state *reg, int regno,
+			   enum bpf_arg_type arg_type);
 int check_kfunc_mem_size_reg(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
 			     u32 regno);
 int check_mem_reg(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index a57db4b2803c..c85f4b2458f4 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -5359,6 +5359,44 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno,
 	return 0;
 }
 
+int check_func_arg_reg_off(struct bpf_verifier_env *env,
+			   const struct bpf_reg_state *reg, int regno,
+			   enum bpf_arg_type arg_type)
+{
+	enum bpf_reg_type type = reg->type;
+	int 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;
+	}
+	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)
@@ -5408,34 +5446,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);
+	if (err)
+		return err;
 
 skip_type_check:
 	if (reg->ref_obj_id) {
-- 
2.35.1

[PATCH bpf-next v3 2/8] bpf: Fix PTR_TO_BTF_ID var_off check

[Kumar Kartikeya Dwivedi]

When kfunc support was added, check_ctx_reg was called for PTR_TO_CTX
register, but no offset checks were made for PTR_TO_BTF_ID. Only
reg->off was taken into account by btf_struct_ids_match, which protected
against type mismatch due to non-zero reg->off, but when reg->off was
zero, a user could set the variable offset of the register and allow it
to be passed to kfunc, leading to bad pointer being passed into the
kernel.

Fix this by reusing the extracted helper check_func_arg_reg_off from
previous commit, and make one call before checking all supported
register types. Since the list is maintained, any future changes will be
taken into account by updating check_func_arg_reg_off. This function
prevents non-zero var_off to be set for PTR_TO_BTF_ID, but still allows
a fixed non-zero reg->off, which is needed for type matching to work
correctly when using pointer arithmetic.

ARG_DONTCARE is passed as arg_type, since kfunc doesn't support
accepting a ARG_PTR_TO_ALLOC_MEM without relying on size of parameter
type from BTF (in case of pointer), or using a mem, len pair. The
forcing of offset check for ARG_PTR_TO_ALLOC_MEM is done because ringbuf
helpers obtain the size from the header located at the beginning of the
memory region, hence any changes to the original pointer shouldn't be
allowed. In case of kfunc, size is always known, either at verification
time, or using the length parameter, hence this forcing is not required.

Since this check will happen once already for PTR_TO_CTX, remove the
check_ptr_off_reg call inside its block.

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>
---
 kernel/bpf/btf.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index b472cf0c8fdb..7f6a0ae5028b 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -5726,7 +5726,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 	const char *func_name, *ref_tname;
 	const struct btf_type *t, *ref_t;
 	const struct btf_param *args;
-	int ref_regno = 0;
+	int ref_regno = 0, ret;
 	bool rel = false;
 
 	t = btf_type_by_id(btf, func_id);
@@ -5776,6 +5776,11 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 
 		ref_t = btf_type_skip_modifiers(btf, t->type, &ref_id);
 		ref_tname = btf_name_by_offset(btf, ref_t->name_off);
+
+		ret = check_func_arg_reg_off(env, reg, regno, ARG_DONTCARE);
+		if (ret < 0)
+			return ret;
+
 		if (btf_get_prog_ctx_type(log, btf, t,
 					  env->prog->type, i)) {
 			/* If function expects ctx type in BTF check that caller
@@ -5787,8 +5792,6 @@ 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))
-				return -EINVAL;
 		} else if (is_kfunc && (reg->type == PTR_TO_BTF_ID ||
 			   (reg2btf_ids[base_type(reg->type)] && !type_flag(reg->type)))) {
 			const struct btf_type *reg_ref_t;
-- 
2.35.1

[PATCH bpf-next v3 3/8] bpf: Disallow negative offset in check_ptr_off_reg

[Kumar Kartikeya Dwivedi]

check_ptr_off_reg only allows fixed offset to be set for PTR_TO_BTF_ID,
where reg->off < 0 doesn't make sense. This would shift the pointer
backwards, and fails later in btf_struct_ids_match or btf_struct_walk
due to out of bounds access (since offset is interpreted as unsigned).

Improve the verifier by rejecting this case by using a better error
message for BPF helpers and kfunc, by putting a check inside the
check_func_arg_reg_off function.

Also, update existing verifier selftests to work with new error string.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 kernel/bpf/verifier.c                                   | 6 ++++++
 tools/testing/selftests/bpf/verifier/bounds_deduction.c | 2 +-
 tools/testing/selftests/bpf/verifier/ctx.c              | 8 ++++----
 3 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index c85f4b2458f4..e55bfd23e81b 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -3990,6 +3990,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);
diff --git a/tools/testing/selftests/bpf/verifier/bounds_deduction.c b/tools/testing/selftests/bpf/verifier/bounds_deduction.c
index 91869aea6d64..3931c481e30c 100644
--- a/tools/testing/selftests/bpf/verifier/bounds_deduction.c
+++ b/tools/testing/selftests/bpf/verifier/bounds_deduction.c
@@ -105,7 +105,7 @@
 		BPF_EXIT_INSN(),
 	},
 	.errstr_unpriv = "R1 has pointer with unsupported alu operation",
-	.errstr = "dereference of modified ctx ptr",
+	.errstr = "negative offset ctx ptr R1 off=-1 disallowed",
 	.result = REJECT,
 	.flags = F_NEEDS_EFFICIENT_UNALIGNED_ACCESS,
 },
diff --git a/tools/testing/selftests/bpf/verifier/ctx.c b/tools/testing/selftests/bpf/verifier/ctx.c
index 60f6fbe03f19..c8eaf0536c24 100644
--- a/tools/testing/selftests/bpf/verifier/ctx.c
+++ b/tools/testing/selftests/bpf/verifier/ctx.c
@@ -58,7 +58,7 @@
 	},
 	.prog_type = BPF_PROG_TYPE_SCHED_CLS,
 	.result = REJECT,
-	.errstr = "dereference of modified ctx ptr",
+	.errstr = "negative offset ctx ptr R1 off=-612 disallowed",
 },
 {
 	"pass modified ctx pointer to helper, 2",
@@ -71,8 +71,8 @@
 	},
 	.result_unpriv = REJECT,
 	.result = REJECT,
-	.errstr_unpriv = "dereference of modified ctx ptr",
-	.errstr = "dereference of modified ctx ptr",
+	.errstr_unpriv = "negative offset ctx ptr R1 off=-612 disallowed",
+	.errstr = "negative offset ctx ptr R1 off=-612 disallowed",
 },
 {
 	"pass modified ctx pointer to helper, 3",
@@ -141,7 +141,7 @@
 	.prog_type = BPF_PROG_TYPE_CGROUP_SOCK_ADDR,
 	.expected_attach_type = BPF_CGROUP_UDP6_SENDMSG,
 	.result = REJECT,
-	.errstr = "dereference of modified ctx ptr",
+	.errstr = "negative offset ctx ptr R1 off=-612 disallowed",
 },
 {
 	"pass ctx or null check, 5: null (connect)",
-- 
2.35.1

[PATCH bpf-next v3 4/8] bpf: Harden register offset checks for release helpers and kfuncs

[Kumar Kartikeya Dwivedi]

Let's ensure that the PTR_TO_BTF_ID reg being passed in to release BPF
helpers and kfuncs always has its offset set to 0. While not a real
problem now, there's a very real possibility this will become a problem
when more and more kfuncs are exposed, and more BPF helpers are added
which can release PTR_TO_BTF_ID.

Previous commits already protected against non-zero var_off. One of the
case we are concerned about now is when we have a type that can be
returned by e.g. an acquire kfunc:

struct foo {
	int a;
	int b;
	struct bar b;
};

... and struct bar is also a type that can be returned by another
acquire kfunc.

Then, doing the following sequence:

	struct foo *f = bpf_get_foo(); // acquire kfunc
	if (!f)
		return 0;
	bpf_put_bar(&f->b); // release kfunc

... would work with the current code, since the btf_struct_ids_match
takes reg->off into account for matching pointer type with release kfunc
argument type, but would obviously be incorrect, and most likely lead to
a kernel crash. A test has been included later to prevent regressions in
this area.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 include/linux/bpf_verifier.h |  3 ++-
 kernel/bpf/btf.c             | 13 +++++++++----
 kernel/bpf/verifier.c        | 27 +++++++++++++++++++++++----
 3 files changed, 34 insertions(+), 9 deletions(-)

diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
index 38b24ee8d8c2..7a684050495a 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -523,7 +523,8 @@ int check_ptr_off_reg(struct bpf_verifier_env *env,
 		      const struct bpf_reg_state *reg, int regno);
 int check_func_arg_reg_off(struct bpf_verifier_env *env,
 			   const struct bpf_reg_state *reg, int regno,
-			   enum bpf_arg_type arg_type);
+			   enum bpf_arg_type arg_type,
+			   bool is_release_function);
 int check_kfunc_mem_size_reg(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
 			     u32 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 7f6a0ae5028b..c9a1019dc60d 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -5753,6 +5753,9 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 		return -EINVAL;
 	}
 
+	if (is_kfunc)
+		rel = btf_kfunc_id_set_contains(btf, resolve_prog_type(env->prog),
+						BTF_KFUNC_TYPE_RELEASE, func_id);
 	/* check that BTF function arguments match actual types that the
 	 * verifier sees.
 	 */
@@ -5777,7 +5780,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 		ref_t = btf_type_skip_modifiers(btf, t->type, &ref_id);
 		ref_tname = btf_name_by_offset(btf, ref_t->name_off);
 
-		ret = check_func_arg_reg_off(env, reg, regno, ARG_DONTCARE);
+		ret = check_func_arg_reg_off(env, reg, regno, ARG_DONTCARE, rel);
 		if (ret < 0)
 			return ret;
 
@@ -5809,7 +5812,11 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 			if (reg->type == PTR_TO_BTF_ID) {
 				reg_btf = reg->btf;
 				reg_ref_id = reg->btf_id;
-				/* Ensure only one argument is referenced PTR_TO_BTF_ID */
+				/* Ensure only one argument is referenced
+				 * PTR_TO_BTF_ID, check_func_arg_reg_off relies
+				 * on only one referenced register being allowed
+				 * for kfuncs.
+				 */
 				if (reg->ref_obj_id) {
 					if (ref_obj_id) {
 						bpf_log(log, "verifier internal error: more than one arg with ref_obj_id R%d %u %u\n",
@@ -5892,8 +5899,6 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 	/* Either both are set, or neither */
 	WARN_ON_ONCE((ref_obj_id && !ref_regno) || (!ref_obj_id && ref_regno));
 	if (is_kfunc) {
-		rel = btf_kfunc_id_set_contains(btf, resolve_prog_type(env->prog),
-						BTF_KFUNC_TYPE_RELEASE, func_id);
 		/* We already made sure ref_obj_id is set only for one argument */
 		if (rel && !ref_obj_id) {
 			bpf_log(log, "release kernel function %s expects refcounted PTR_TO_BTF_ID\n",
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index e55bfd23e81b..c31407d156e7 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -5367,11 +5367,28 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno,
 
 int check_func_arg_reg_off(struct bpf_verifier_env *env,
 			   const struct bpf_reg_state *reg, int regno,
-			   enum bpf_arg_type arg_type)
+			   enum bpf_arg_type arg_type,
+			   bool is_release_func)
 {
 	enum bpf_reg_type type = reg->type;
+	bool fixed_off_ok = false;
 	int err;
 
+	/* When referenced PTR_TO_BTF_ID is passed to release function, it's
+	 * fixed offset must be 0. We rely on the property that only one
+	 * referenced register can be passed to BPF helpers and kfuncs.
+	 */
+	if (type == PTR_TO_BTF_ID) {
+		bool release_reg = is_release_func && reg->ref_obj_id;
+
+		if (release_reg && reg->off) {
+			verbose(env, "R%d must have zero offset when passed to release func\n",
+				regno);
+			return -EINVAL;
+		}
+		fixed_off_ok = release_reg ? false : true;
+	}
+
 	switch ((u32)type) {
 	case SCALAR_VALUE:
 	/* Pointer types where reg offset is explicitly allowed: */
@@ -5394,8 +5411,7 @@ int check_func_arg_reg_off(struct bpf_verifier_env *env,
 	/* All the rest must be rejected: */
 	default:
 force_off_check:
-		err = __check_ptr_off_reg(env, reg, regno,
-					  type == PTR_TO_BTF_ID);
+		err = __check_ptr_off_reg(env, reg, regno, fixed_off_ok);
 		if (err < 0)
 			return err;
 		break;
@@ -5452,11 +5468,14 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
 	if (err)
 		return err;
 
-	err = check_func_arg_reg_off(env, reg, regno, arg_type);
+	err = check_func_arg_reg_off(env, reg, regno, arg_type, is_release_function(meta->func_id));
 	if (err)
 		return err;
 
 skip_type_check:
+	/* check_func_arg_reg_off relies on only one referenced register being
+	 * allowed for BPF helpers.
+	 */
 	if (reg->ref_obj_id) {
 		if (meta->ref_obj_id) {
 			verbose(env, "verifier internal error: more than one arg with ref_obj_id R%d %u %u\n",
-- 
2.35.1

[PATCH bpf-next v3 5/8] compiler-clang.h: Add __diag infrastructure for clang

[Kumar Kartikeya Dwivedi]

From: Nathan Chancellor <nathan@kernel.org>

Add __diag macros similar to those in compiler-gcc.h, so that warnings
that need to be adjusted for specific cases but not globally can be
ignored for LLVM compilation mode as well.

Cc: Nathan Chancellor <nathan@kernel.org>
Cc: Nick Desaulniers <ndesaulniers@google.com>
Cc: llvm@lists.linux.dev
Signed-off-by: Nathan Chancellor <nathan@kernel.org>
[ Kartikeya: wrote commit message ]
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 include/linux/compiler-clang.h | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h
index 3c4de9b6c6e3..f1aa41d520bd 100644
--- a/include/linux/compiler-clang.h
+++ b/include/linux/compiler-clang.h
@@ -68,3 +68,25 @@
 
 #define __nocfi		__attribute__((__no_sanitize__("cfi")))
 #define __cficanonical	__attribute__((__cfi_canonical_jump_table__))
+
+/*
+ * Turn individual warnings and errors on and off locally, depending
+ * on version.
+ */
+#define __diag_clang(version, severity, s) \
+	__diag_clang_ ## version(__diag_clang_ ## severity s)
+
+/* Severity used in pragma directives */
+#define __diag_clang_ignore	ignored
+#define __diag_clang_warn	warning
+#define __diag_clang_error	error
+
+#define __diag_str1(s)		#s
+#define __diag_str(s)		__diag_str1(s)
+#define __diag(s)		_Pragma(__diag_str(clang diagnostic s))
+
+#if CONFIG_CLANG_VERSION >= 110000
+#define __diag_clang_11(s)	__diag(s)
+#else
+#define __diag_clang_11(s)
+#endif
-- 
2.35.1

[PATCH bpf-next v3 6/8] compiler_types.h: Add unified __diag_ignore_all for GCC/LLVM

[Kumar Kartikeya Dwivedi]

Add a __diag_ignore_all macro, to ignore warnings for both GCC and LLVM,
without having to specify the compiler type and version. By default, GCC
8 and clang 11 are used. This will be used by bpf subsystem to ignore
-Wmissing-prototypes warning for functions that are meant to be global
functions so that they are in vmlinux BTF, but don't have a prototype.

Cc: linux-kernel@vger.kernel.org
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 include/linux/compiler-clang.h | 3 +++
 include/linux/compiler-gcc.h   | 3 +++
 include/linux/compiler_types.h | 4 ++++
 3 files changed, 10 insertions(+)

diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h
index f1aa41d520bd..babb1347148c 100644
--- a/include/linux/compiler-clang.h
+++ b/include/linux/compiler-clang.h
@@ -90,3 +90,6 @@
 #else
 #define __diag_clang_11(s)
 #endif
+
+#define __diag_ignore_all(option, comment) \
+	__diag_clang(11, ignore, option)
diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
index ccbbd31b3aae..d364c98a4a80 100644
--- a/include/linux/compiler-gcc.h
+++ b/include/linux/compiler-gcc.h
@@ -151,6 +151,9 @@
 #define __diag_GCC_8(s)
 #endif
 
+#define __diag_ignore_all(option, comment) \
+	__diag_GCC(8, ignore, option)
+
 /*
  * Prior to 9.1, -Wno-alloc-size-larger-than (and therefore the "alloc_size"
  * attribute) do not work, and must be disabled.
diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
index 3f31ff400432..8e5d2f50f951 100644
--- a/include/linux/compiler_types.h
+++ b/include/linux/compiler_types.h
@@ -371,4 +371,8 @@ struct ftrace_likely_data {
 #define __diag_error(compiler, version, option, comment) \
 	__diag_ ## compiler(version, error, option)
 
+#ifndef __diag_ignore_all
+#define __diag_ignore_all(option, comment)
+#endif
+
 #endif /* __LINUX_COMPILER_TYPES_H */
-- 
2.35.1

[PATCH bpf-next v3 7/8] bpf: Replace __diag_ignore with unified __diag_ignore_all

[Kumar Kartikeya Dwivedi]

Currently, -Wmissing-prototypes warning is ignored for GCC, but not
clang. This leads to clang build warning in W=1 mode. Since the flag
used by both compilers is same, we can use the unified __diag_ignore_all
macro that works for all supported versions and compilers which have
__diag macro support (currently GCC >= 8.0, and Clang >= 11.0).

Also add nf_conntrack_bpf.h include to prevent missing prototype warning
for register_nf_conntrack_bpf.

Cc: netfilter-devel@vger.kernel.org
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 net/bpf/test_run.c               | 4 ++--
 net/netfilter/nf_conntrack_bpf.c | 5 +++--
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
index eb129e48f90b..fcc83017cd03 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -201,8 +201,8 @@ static int bpf_test_finish(const union bpf_attr *kattr,
  * future.
  */
 __diag_push();
-__diag_ignore(GCC, 8, "-Wmissing-prototypes",
-	      "Global functions as their definitions will be in vmlinux BTF");
+__diag_ignore_all("-Wmissing-prototypes",
+		  "Global functions as their definitions will be in vmlinux BTF");
 int noinline bpf_fentry_test1(int a)
 {
 	return a + 1;
diff --git a/net/netfilter/nf_conntrack_bpf.c b/net/netfilter/nf_conntrack_bpf.c
index 8ad3f52579f3..fe98673dd5ac 100644
--- a/net/netfilter/nf_conntrack_bpf.c
+++ b/net/netfilter/nf_conntrack_bpf.c
@@ -12,6 +12,7 @@
 #include <linux/btf_ids.h>
 #include <linux/net_namespace.h>
 #include <net/netfilter/nf_conntrack.h>
+#include <net/netfilter/nf_conntrack_bpf.h>
 #include <net/netfilter/nf_conntrack_core.h>
 
 /* bpf_ct_opts - Options for CT lookup helpers
@@ -102,8 +103,8 @@ static struct nf_conn *__bpf_nf_ct_lookup(struct net *net,
 }
 
 __diag_push();
-__diag_ignore(GCC, 8, "-Wmissing-prototypes",
-	      "Global functions as their definitions will be in nf_conntrack BTF");
+__diag_ignore_all("-Wmissing-prototypes",
+		  "Global functions as their definitions will be in nf_conntrack BTF");
 
 /* bpf_xdp_ct_lookup - Lookup CT entry for the given tuple, and acquire a
  *		       reference to it
-- 
2.35.1

[PATCH bpf-next v3 8/8] selftests/bpf: Add tests for kfunc register offset checks

[Kumar Kartikeya Dwivedi]

Include a few verifier selftests that test against the problems being
fixed by previous commits, i.e. release kfunc always require
PTR_TO_BTF_ID fixed and var_off to be 0, and negative offset is not
permitted and returns a helpful error message.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 net/bpf/test_run.c                           | 11 +++
 tools/testing/selftests/bpf/verifier/calls.c | 83 ++++++++++++++++++++
 2 files changed, 94 insertions(+)

diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
index fcc83017cd03..ba410b069824 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -270,9 +270,14 @@ struct sock * noinline bpf_kfunc_call_test3(struct sock *sk)
 	return sk;
 }
 
+struct prog_test_member {
+	u64 c;
+};
+
 struct prog_test_ref_kfunc {
 	int a;
 	int b;
+	struct prog_test_member memb;
 	struct prog_test_ref_kfunc *next;
 };
 
@@ -295,6 +300,10 @@ noinline void bpf_kfunc_call_test_release(struct prog_test_ref_kfunc *p)
 {
 }
 
+noinline void bpf_kfunc_call_memb_release(struct prog_test_member *p)
+{
+}
+
 struct prog_test_pass1 {
 	int x0;
 	struct {
@@ -379,6 +388,7 @@ BTF_ID(func, bpf_kfunc_call_test2)
 BTF_ID(func, bpf_kfunc_call_test3)
 BTF_ID(func, bpf_kfunc_call_test_acquire)
 BTF_ID(func, bpf_kfunc_call_test_release)
+BTF_ID(func, bpf_kfunc_call_memb_release)
 BTF_ID(func, bpf_kfunc_call_test_pass_ctx)
 BTF_ID(func, bpf_kfunc_call_test_pass1)
 BTF_ID(func, bpf_kfunc_call_test_pass2)
@@ -396,6 +406,7 @@ BTF_SET_END(test_sk_acquire_kfunc_ids)
 
 BTF_SET_START(test_sk_release_kfunc_ids)
 BTF_ID(func, bpf_kfunc_call_test_release)
+BTF_ID(func, bpf_kfunc_call_memb_release)
 BTF_SET_END(test_sk_release_kfunc_ids)
 
 BTF_SET_START(test_sk_ret_null_kfunc_ids)
diff --git a/tools/testing/selftests/bpf/verifier/calls.c b/tools/testing/selftests/bpf/verifier/calls.c
index f890333259ad..2e03decb11b6 100644
--- a/tools/testing/selftests/bpf/verifier/calls.c
+++ b/tools/testing/selftests/bpf/verifier/calls.c
@@ -115,6 +115,89 @@
 		{ "bpf_kfunc_call_test_release", 5 },
 	},
 },
+{
+	"calls: invalid kfunc call: reg->off must be zero when passed to release kfunc",
+	.insns = {
+	BPF_MOV64_REG(BPF_REG_1, BPF_REG_10),
+	BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, -8),
+	BPF_ST_MEM(BPF_DW, BPF_REG_1, 0, 0),
+	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0),
+	BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, 1),
+	BPF_EXIT_INSN(),
+	BPF_ALU64_IMM(BPF_ADD, BPF_REG_0, 8),
+	BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
+	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0),
+	BPF_MOV64_IMM(BPF_REG_0, 0),
+	BPF_EXIT_INSN(),
+	},
+	.prog_type = BPF_PROG_TYPE_SCHED_CLS,
+	.result = REJECT,
+	.errstr = "R1 must have zero offset when passed to release func",
+	.fixup_kfunc_btf_id = {
+		{ "bpf_kfunc_call_test_acquire", 3 },
+		{ "bpf_kfunc_call_memb_release", 8 },
+	},
+},
+{
+	"calls: invalid kfunc call: PTR_TO_BTF_ID with negative offset",
+	.insns = {
+	BPF_MOV64_REG(BPF_REG_1, BPF_REG_10),
+	BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, -8),
+	BPF_ST_MEM(BPF_DW, BPF_REG_1, 0, 0),
+	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0),
+	BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, 1),
+	BPF_EXIT_INSN(),
+	BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
+	BPF_LDX_MEM(BPF_DW, BPF_REG_1, BPF_REG_1, 16),
+	BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, -4),
+	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0),
+	BPF_MOV64_IMM(BPF_REG_0, 0),
+	BPF_EXIT_INSN(),
+	},
+	.prog_type = BPF_PROG_TYPE_SCHED_CLS,
+	.fixup_kfunc_btf_id = {
+		{ "bpf_kfunc_call_test_acquire", 3 },
+		{ "bpf_kfunc_call_test_release", 9 },
+	},
+	.result_unpriv = REJECT,
+	.result = REJECT,
+	.errstr = "negative offset ptr_ ptr R1 off=-4 disallowed",
+},
+{
+	"calls: invalid kfunc call: PTR_TO_BTF_ID with variable offset",
+	.insns = {
+	BPF_MOV64_REG(BPF_REG_1, BPF_REG_10),
+	BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, -8),
+	BPF_ST_MEM(BPF_DW, BPF_REG_1, 0, 0),
+	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0),
+	BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, 1),
+	BPF_EXIT_INSN(),
+	BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
+	BPF_LDX_MEM(BPF_W, BPF_REG_2, BPF_REG_0, 4),
+	BPF_JMP_IMM(BPF_JLE, BPF_REG_2, 4, 3),
+	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0),
+	BPF_MOV64_IMM(BPF_REG_0, 0),
+	BPF_EXIT_INSN(),
+	BPF_JMP_IMM(BPF_JGE, BPF_REG_2, 0, 3),
+	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0),
+	BPF_MOV64_IMM(BPF_REG_0, 0),
+	BPF_EXIT_INSN(),
+	BPF_ALU64_REG(BPF_ADD, BPF_REG_1, BPF_REG_2),
+	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0),
+	BPF_MOV64_IMM(BPF_REG_0, 0),
+	BPF_EXIT_INSN(),
+	},
+	.prog_type = BPF_PROG_TYPE_SCHED_CLS,
+	.fixup_kfunc_btf_id = {
+		{ "bpf_kfunc_call_test_acquire", 3 },
+		{ "bpf_kfunc_call_test_release", 9 },
+		{ "bpf_kfunc_call_test_release", 13 },
+		{ "bpf_kfunc_call_test_release", 17 },
+	},
+	.result_unpriv = REJECT,
+	.result = REJECT,
+	.errstr = "variable ptr_ access var_off=(0x0; 0x7) disallowed",
+},
 {
 	"calls: basic sanity",
 	.insns = {
-- 
2.35.1

Re: [PATCH bpf-next v3 1/8] bpf: Add check_func_arg_reg_off function

[Martin KaFai Lau]

On Fri, Mar 04, 2022 at 05:35:01AM +0530, Kumar Kartikeya Dwivedi wrote:
> Lift the list of register types allowed for having fixed and variable
> offsets when passed as helper function arguments into a common helper,
> so that they can be reused for kfunc checks in later commits. Keeping a
> common helper aids maintainability and allows us to follow the same
> consistent rules across helpers and kfuncs. Also, convert check_func_arg
> to use this function.
> 
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> ---
>  include/linux/bpf_verifier.h |  3 ++
>  kernel/bpf/verifier.c        | 69 +++++++++++++++++++++---------------
>  2 files changed, 44 insertions(+), 28 deletions(-)
> 
> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> index 7a7be8c057f2..38b24ee8d8c2 100644
> --- a/include/linux/bpf_verifier.h
> +++ b/include/linux/bpf_verifier.h
> @@ -521,6 +521,9 @@ bpf_prog_offload_remove_insns(struct bpf_verifier_env *env, u32 off, u32 cnt);
>  
>  int check_ptr_off_reg(struct bpf_verifier_env *env,
>  		      const struct bpf_reg_state *reg, int regno);
> +int check_func_arg_reg_off(struct bpf_verifier_env *env,
> +			   const struct bpf_reg_state *reg, int regno,
> +			   enum bpf_arg_type arg_type);
>  int check_kfunc_mem_size_reg(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
>  			     u32 regno);
>  int check_mem_reg(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index a57db4b2803c..c85f4b2458f4 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -5359,6 +5359,44 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno,
>  	return 0;
>  }
>  
> +int check_func_arg_reg_off(struct bpf_verifier_env *env,
> +			   const struct bpf_reg_state *reg, int regno,
> +			   enum bpf_arg_type arg_type)
> +{
> +	enum bpf_reg_type type = reg->type;
> +	int 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;
Nit. Since it is refactoring to a new function now,
it can directly return __check_ptr_off_reg().

		
> +		break;
> +	}
> +	return 0;
> +}
May as well flip the arg_type check and leave calling
the __check_ptr_off_reg at the end.

Uncompiled code:

int check_func_arg_reg_off(struct bpf_verifier_env *env,
			   const struct bpf_reg_state *reg, int regno,
			   enum bpf_arg_type arg_type)
{
	enum bpf_reg_type type = reg->type;
	bool fixed_off_ok = false;

	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)
			return 0;
		break;
	case PTR_TO_BTF_ID:
		fixed_off_ok = true;
		break;
	}

	return __check_ptr_off_reg(env, reg, regno, fixed_off_ok);
}

Then 'case PTR_TO_BTF_ID' can then be reused in patch 4 instead
of adding a special 'if (type == PTR_TO_BTF_ID)' just
before the 'switch ((u32)type)'

Both are nits. can be ignored.

> +
>  static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
>  			  struct bpf_call_arg_meta *meta,
>  			  const struct bpf_func_proto *fn)
> @@ -5408,34 +5446,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);
> +	if (err)
> +		return err;
>  
>  skip_type_check:
>  	if (reg->ref_obj_id) {
> -- 
> 2.35.1
> 

Re: [PATCH bpf-next v3 4/8] bpf: Harden register offset checks for release helpers and kfuncs

[Martin KaFai Lau]

On Fri, Mar 04, 2022 at 05:35:04AM +0530, Kumar Kartikeya Dwivedi wrote:
> Let's ensure that the PTR_TO_BTF_ID reg being passed in to release BPF
> helpers and kfuncs always has its offset set to 0. While not a real
> problem now, there's a very real possibility this will become a problem
> when more and more kfuncs are exposed, and more BPF helpers are added
> which can release PTR_TO_BTF_ID.
> 
> Previous commits already protected against non-zero var_off. One of the
> case we are concerned about now is when we have a type that can be
> returned by e.g. an acquire kfunc:
> 
> struct foo {
> 	int a;
> 	int b;
> 	struct bar b;
> };
> 
> ... and struct bar is also a type that can be returned by another
> acquire kfunc.
> 
> Then, doing the following sequence:
> 
> 	struct foo *f = bpf_get_foo(); // acquire kfunc
> 	if (!f)
> 		return 0;
> 	bpf_put_bar(&f->b); // release kfunc
> 
> ... would work with the current code, since the btf_struct_ids_match
> takes reg->off into account for matching pointer type with release kfunc
> argument type, but would obviously be incorrect, and most likely lead to
> a kernel crash. A test has been included later to prevent regressions in
> this area.
> 
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> ---
>  include/linux/bpf_verifier.h |  3 ++-
>  kernel/bpf/btf.c             | 13 +++++++++----
>  kernel/bpf/verifier.c        | 27 +++++++++++++++++++++++----
>  3 files changed, 34 insertions(+), 9 deletions(-)
> 
> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> index 38b24ee8d8c2..7a684050495a 100644
> --- a/include/linux/bpf_verifier.h
> +++ b/include/linux/bpf_verifier.h
> @@ -523,7 +523,8 @@ int check_ptr_off_reg(struct bpf_verifier_env *env,
>  		      const struct bpf_reg_state *reg, int regno);
>  int check_func_arg_reg_off(struct bpf_verifier_env *env,
>  			   const struct bpf_reg_state *reg, int regno,
> -			   enum bpf_arg_type arg_type);
> +			   enum bpf_arg_type arg_type,
> +			   bool is_release_function);
>  int check_kfunc_mem_size_reg(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
>  			     u32 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 7f6a0ae5028b..c9a1019dc60d 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -5753,6 +5753,9 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
>  		return -EINVAL;
>  	}
>  
> +	if (is_kfunc)
> +		rel = btf_kfunc_id_set_contains(btf, resolve_prog_type(env->prog),
> +						BTF_KFUNC_TYPE_RELEASE, func_id);
>  	/* check that BTF function arguments match actual types that the
>  	 * verifier sees.
>  	 */
> @@ -5777,7 +5780,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
>  		ref_t = btf_type_skip_modifiers(btf, t->type, &ref_id);
>  		ref_tname = btf_name_by_offset(btf, ref_t->name_off);
>  
> -		ret = check_func_arg_reg_off(env, reg, regno, ARG_DONTCARE);
> +		ret = check_func_arg_reg_off(env, reg, regno, ARG_DONTCARE, rel);
>  		if (ret < 0)
>  			return ret;
>  
> @@ -5809,7 +5812,11 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
>  			if (reg->type == PTR_TO_BTF_ID) {
>  				reg_btf = reg->btf;
>  				reg_ref_id = reg->btf_id;
> -				/* Ensure only one argument is referenced PTR_TO_BTF_ID */
> +				/* Ensure only one argument is referenced
> +				 * PTR_TO_BTF_ID, check_func_arg_reg_off relies
> +				 * on only one referenced register being allowed
> +				 * for kfuncs.
> +				 */
>  				if (reg->ref_obj_id) {
>  					if (ref_obj_id) {
>  						bpf_log(log, "verifier internal error: more than one arg with ref_obj_id R%d %u %u\n",
> @@ -5892,8 +5899,6 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
>  	/* Either both are set, or neither */
>  	WARN_ON_ONCE((ref_obj_id && !ref_regno) || (!ref_obj_id && ref_regno));
>  	if (is_kfunc) {
This test is no longer needed?

> -		rel = btf_kfunc_id_set_contains(btf, resolve_prog_type(env->prog),
> -						BTF_KFUNC_TYPE_RELEASE, func_id);
>  		/* We already made sure ref_obj_id is set only for one argument */
>  		if (rel && !ref_obj_id) {
>  			bpf_log(log, "release kernel function %s expects refcounted PTR_TO_BTF_ID\n",
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index e55bfd23e81b..c31407d156e7 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -5367,11 +5367,28 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno,
>  
>  int check_func_arg_reg_off(struct bpf_verifier_env *env,
>  			   const struct bpf_reg_state *reg, int regno,
> -			   enum bpf_arg_type arg_type)
> +			   enum bpf_arg_type arg_type,
> +			   bool is_release_func)
>  {
>  	enum bpf_reg_type type = reg->type;
> +	bool fixed_off_ok = false;
>  	int err;
>  
> +	/* When referenced PTR_TO_BTF_ID is passed to release function, it's
> +	 * fixed offset must be 0. We rely on the property that only one
> +	 * referenced register can be passed to BPF helpers and kfuncs.
> +	 */
> +	if (type == PTR_TO_BTF_ID) {
> +		bool release_reg = is_release_func && reg->ref_obj_id;
> +
> +		if (release_reg && reg->off) {
iiuc, the reason for not going through __check_ptr_off_reg() is
because it prefers a different verifier log message for release_reg
case for fixed off.  How about var_off?

> +			verbose(env, "R%d must have zero offset when passed to release func\n",
> +				regno);
> +			return -EINVAL;
> +		}
> +		fixed_off_ok = release_reg ? false : true;
nit.
		fixed_off_ok = !release_reg;

but this is a bit moot here considering the reg->off
check has already been done for the release_reg case.

> +	}
> +
>  	switch ((u32)type) {
>  	case SCALAR_VALUE:
>  	/* Pointer types where reg offset is explicitly allowed: */
> @@ -5394,8 +5411,7 @@ int check_func_arg_reg_off(struct bpf_verifier_env *env,
>  	/* All the rest must be rejected: */
>  	default:
>  force_off_check:
> -		err = __check_ptr_off_reg(env, reg, regno,
> -					  type == PTR_TO_BTF_ID);
> +		err = __check_ptr_off_reg(env, reg, regno, fixed_off_ok);
>  		if (err < 0)
>  			return err;
>  		break;
> @@ -5452,11 +5468,14 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
>  	if (err)
>  		return err;
>  
> -	err = check_func_arg_reg_off(env, reg, regno, arg_type);
> +	err = check_func_arg_reg_off(env, reg, regno, arg_type, is_release_function(meta->func_id));
>  	if (err)
>  		return err;
>  
>  skip_type_check:
> +	/* check_func_arg_reg_off relies on only one referenced register being
> +	 * allowed for BPF helpers.
> +	 */
>  	if (reg->ref_obj_id) {
>  		if (meta->ref_obj_id) {
>  			verbose(env, "verifier internal error: more than one arg with ref_obj_id R%d %u %u\n",
> -- 
> 2.35.1
> 

Re: [PATCH bpf-next v3 4/8] bpf: Harden register offset checks for release helpers and kfuncs

[Kumar Kartikeya Dwivedi]

On Sat, Mar 05, 2022 at 01:58:30AM IST, Martin KaFai Lau wrote:
> On Fri, Mar 04, 2022 at 05:35:04AM +0530, Kumar Kartikeya Dwivedi wrote:
> > Let's ensure that the PTR_TO_BTF_ID reg being passed in to release BPF
> > helpers and kfuncs always has its offset set to 0. While not a real
> > problem now, there's a very real possibility this will become a problem
> > when more and more kfuncs are exposed, and more BPF helpers are added
> > which can release PTR_TO_BTF_ID.
> >
> > Previous commits already protected against non-zero var_off. One of the
> > case we are concerned about now is when we have a type that can be
> > returned by e.g. an acquire kfunc:
> >
> > struct foo {
> > 	int a;
> > 	int b;
> > 	struct bar b;
> > };
> >
> > ... and struct bar is also a type that can be returned by another
> > acquire kfunc.
> >
> > Then, doing the following sequence:
> >
> > 	struct foo *f = bpf_get_foo(); // acquire kfunc
> > 	if (!f)
> > 		return 0;
> > 	bpf_put_bar(&f->b); // release kfunc
> >
> > ... would work with the current code, since the btf_struct_ids_match
> > takes reg->off into account for matching pointer type with release kfunc
> > argument type, but would obviously be incorrect, and most likely lead to
> > a kernel crash. A test has been included later to prevent regressions in
> > this area.
> >
> > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> > ---
> >  include/linux/bpf_verifier.h |  3 ++-
> >  kernel/bpf/btf.c             | 13 +++++++++----
> >  kernel/bpf/verifier.c        | 27 +++++++++++++++++++++++----
> >  3 files changed, 34 insertions(+), 9 deletions(-)
> >
> > diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> > index 38b24ee8d8c2..7a684050495a 100644
> > --- a/include/linux/bpf_verifier.h
> > +++ b/include/linux/bpf_verifier.h
> > @@ -523,7 +523,8 @@ int check_ptr_off_reg(struct bpf_verifier_env *env,
> >  		      const struct bpf_reg_state *reg, int regno);
> >  int check_func_arg_reg_off(struct bpf_verifier_env *env,
> >  			   const struct bpf_reg_state *reg, int regno,
> > -			   enum bpf_arg_type arg_type);
> > +			   enum bpf_arg_type arg_type,
> > +			   bool is_release_function);
> >  int check_kfunc_mem_size_reg(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
> >  			     u32 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 7f6a0ae5028b..c9a1019dc60d 100644
> > --- a/kernel/bpf/btf.c
> > +++ b/kernel/bpf/btf.c
> > @@ -5753,6 +5753,9 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
> >  		return -EINVAL;
> >  	}
> >
> > +	if (is_kfunc)
> > +		rel = btf_kfunc_id_set_contains(btf, resolve_prog_type(env->prog),
> > +						BTF_KFUNC_TYPE_RELEASE, func_id);
> >  	/* check that BTF function arguments match actual types that the
> >  	 * verifier sees.
> >  	 */
> > @@ -5777,7 +5780,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
> >  		ref_t = btf_type_skip_modifiers(btf, t->type, &ref_id);
> >  		ref_tname = btf_name_by_offset(btf, ref_t->name_off);
> >
> > -		ret = check_func_arg_reg_off(env, reg, regno, ARG_DONTCARE);
> > +		ret = check_func_arg_reg_off(env, reg, regno, ARG_DONTCARE, rel);
> >  		if (ret < 0)
> >  			return ret;
> >
> > @@ -5809,7 +5812,11 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
> >  			if (reg->type == PTR_TO_BTF_ID) {
> >  				reg_btf = reg->btf;
> >  				reg_ref_id = reg->btf_id;
> > -				/* Ensure only one argument is referenced PTR_TO_BTF_ID */
> > +				/* Ensure only one argument is referenced
> > +				 * PTR_TO_BTF_ID, check_func_arg_reg_off relies
> > +				 * on only one referenced register being allowed
> > +				 * for kfuncs.
> > +				 */
> >  				if (reg->ref_obj_id) {
> >  					if (ref_obj_id) {
> >  						bpf_log(log, "verifier internal error: more than one arg with ref_obj_id R%d %u %u\n",
> > @@ -5892,8 +5899,6 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
> >  	/* Either both are set, or neither */
> >  	WARN_ON_ONCE((ref_obj_id && !ref_regno) || (!ref_obj_id && ref_regno));
> >  	if (is_kfunc) {
> This test is no longer needed?
>

If you mean the rel && !ref_obj_id below (which is guarded by this check), I do
think it is needed, why do you think so? Because of the check in
check_func_arg_reg_off? That only checks reg->off when it sees that both
release_func and ref_obj_id are true, but ref_obj_id may not be set for any
argument(s) passed to a release function, so we need to reject when we don't get
atleast one referenced register for release function.

Or were you referring to the WARN_ON_ONCE above it?

> > -		rel = btf_kfunc_id_set_contains(btf, resolve_prog_type(env->prog),
> > -						BTF_KFUNC_TYPE_RELEASE, func_id);
> >  		/* We already made sure ref_obj_id is set only for one argument */
> >  		if (rel && !ref_obj_id) {
> >  			bpf_log(log, "release kernel function %s expects refcounted PTR_TO_BTF_ID\n",
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index e55bfd23e81b..c31407d156e7 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -5367,11 +5367,28 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno,
> >
> >  int check_func_arg_reg_off(struct bpf_verifier_env *env,
> >  			   const struct bpf_reg_state *reg, int regno,
> > -			   enum bpf_arg_type arg_type)
> > +			   enum bpf_arg_type arg_type,
> > +			   bool is_release_func)
> >  {
> >  	enum bpf_reg_type type = reg->type;
> > +	bool fixed_off_ok = false;
> >  	int err;
> >
> > +	/* When referenced PTR_TO_BTF_ID is passed to release function, it's
> > +	 * fixed offset must be 0. We rely on the property that only one
> > +	 * referenced register can be passed to BPF helpers and kfuncs.
> > +	 */
> > +	if (type == PTR_TO_BTF_ID) {
> > +		bool release_reg = is_release_func && reg->ref_obj_id;
> > +
> > +		if (release_reg && reg->off) {
> iiuc, the reason for not going through __check_ptr_off_reg() is
> because it prefers a different verifier log message for release_reg
> case for fixed off.  How about var_off?
>

If reg->off is zero, we still call __check_ptr_off_reg with fixed_off_ok =
false, which should handle non-zero var_off.

> > +			verbose(env, "R%d must have zero offset when passed to release func\n",
> > +				regno);
> > +			return -EINVAL;
> > +		}
> > +		fixed_off_ok = release_reg ? false : true;
> nit.
> 		fixed_off_ok = !release_reg;
>
> but this is a bit moot here considering the reg->off
> check has already been done for the release_reg case.
>

Yes, it would be a redundant check inside __check_ptr_off_reg, but we still need
to call it for checking bad var_off.

> > +	}
> > +
> >  	switch ((u32)type) {
> >  	case SCALAR_VALUE:
> >  	/* Pointer types where reg offset is explicitly allowed: */
> > @@ -5394,8 +5411,7 @@ int check_func_arg_reg_off(struct bpf_verifier_env *env,
> >  	/* All the rest must be rejected: */
> >  	default:
> >  force_off_check:
> > -		err = __check_ptr_off_reg(env, reg, regno,
> > -					  type == PTR_TO_BTF_ID);
> > +		err = __check_ptr_off_reg(env, reg, regno, fixed_off_ok);
> >  		if (err < 0)
> >  			return err;
> >  		break;
> > @@ -5452,11 +5468,14 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> >  	if (err)
> >  		return err;
> >
> > -	err = check_func_arg_reg_off(env, reg, regno, arg_type);
> > +	err = check_func_arg_reg_off(env, reg, regno, arg_type, is_release_function(meta->func_id));
> >  	if (err)
> >  		return err;
> >
> >  skip_type_check:
> > +	/* check_func_arg_reg_off relies on only one referenced register being
> > +	 * allowed for BPF helpers.
> > +	 */
> >  	if (reg->ref_obj_id) {
> >  		if (meta->ref_obj_id) {
> >  			verbose(env, "verifier internal error: more than one arg with ref_obj_id R%d %u %u\n",
> > --
> > 2.35.1
> >

--
Kartikeya

Re: [PATCH bpf-next v3 1/8] bpf: Add check_func_arg_reg_off function

[Kumar Kartikeya Dwivedi]

On Sat, Mar 05, 2022 at 01:45:39AM IST, Martin KaFai Lau wrote:
> On Fri, Mar 04, 2022 at 05:35:01AM +0530, Kumar Kartikeya Dwivedi wrote:
> > Lift the list of register types allowed for having fixed and variable
> > offsets when passed as helper function arguments into a common helper,
> > so that they can be reused for kfunc checks in later commits. Keeping a
> > common helper aids maintainability and allows us to follow the same
> > consistent rules across helpers and kfuncs. Also, convert check_func_arg
> > to use this function.
> >
> > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> > ---
> >  include/linux/bpf_verifier.h |  3 ++
> >  kernel/bpf/verifier.c        | 69 +++++++++++++++++++++---------------
> >  2 files changed, 44 insertions(+), 28 deletions(-)
> >
> > diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> > index 7a7be8c057f2..38b24ee8d8c2 100644
> > --- a/include/linux/bpf_verifier.h
> > +++ b/include/linux/bpf_verifier.h
> > @@ -521,6 +521,9 @@ bpf_prog_offload_remove_insns(struct bpf_verifier_env *env, u32 off, u32 cnt);
> >
> >  int check_ptr_off_reg(struct bpf_verifier_env *env,
> >  		      const struct bpf_reg_state *reg, int regno);
> > +int check_func_arg_reg_off(struct bpf_verifier_env *env,
> > +			   const struct bpf_reg_state *reg, int regno,
> > +			   enum bpf_arg_type arg_type);
> >  int check_kfunc_mem_size_reg(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
> >  			     u32 regno);
> >  int check_mem_reg(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index a57db4b2803c..c85f4b2458f4 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -5359,6 +5359,44 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno,
> >  	return 0;
> >  }
> >
> > +int check_func_arg_reg_off(struct bpf_verifier_env *env,
> > +			   const struct bpf_reg_state *reg, int regno,
> > +			   enum bpf_arg_type arg_type)
> > +{
> > +	enum bpf_reg_type type = reg->type;
> > +	int 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;
> Nit. Since it is refactoring to a new function now,
> it can directly return __check_ptr_off_reg().
>
>
> > +		break;
> > +	}
> > +	return 0;
> > +}
> May as well flip the arg_type check and leave calling
> the __check_ptr_off_reg at the end.
>
> Uncompiled code:
>
> int check_func_arg_reg_off(struct bpf_verifier_env *env,
> 			   const struct bpf_reg_state *reg, int regno,
> 			   enum bpf_arg_type arg_type)
> {
> 	enum bpf_reg_type type = reg->type;
> 	bool fixed_off_ok = false;
>
> 	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)
> 			return 0;
> 		break;
> 	case PTR_TO_BTF_ID:
> 		fixed_off_ok = true;
> 		break;
> 	}
>
> 	return __check_ptr_off_reg(env, reg, regno, fixed_off_ok);
> }
>
> Then 'case PTR_TO_BTF_ID' can then be reused in patch 4 instead
> of adding a special 'if (type == PTR_TO_BTF_ID)' just
> before the 'switch ((u32)type)'
>
> Both are nits. can be ignored.
>

Both suggestions are good, will wait for discussion to conclude and respin.

> > +
> >  static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> >  			  struct bpf_call_arg_meta *meta,
> >  			  const struct bpf_func_proto *fn)
> > @@ -5408,34 +5446,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);
> > +	if (err)
> > +		return err;
> >
> >  skip_type_check:
> >  	if (reg->ref_obj_id) {
> > --
> > 2.35.1
> >

--
Kartikeya

Re: [PATCH bpf-next v3 4/8] bpf: Harden register offset checks for release helpers and kfuncs

[Martin KaFai Lau]

On Sat, Mar 05, 2022 at 02:18:56AM +0530, Kumar Kartikeya Dwivedi wrote:
> On Sat, Mar 05, 2022 at 01:58:30AM IST, Martin KaFai Lau wrote:
> > > diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> > > index 38b24ee8d8c2..7a684050495a 100644
> > > --- a/include/linux/bpf_verifier.h
> > > +++ b/include/linux/bpf_verifier.h
> > > @@ -523,7 +523,8 @@ int check_ptr_off_reg(struct bpf_verifier_env *env,
> > >  		      const struct bpf_reg_state *reg, int regno);
> > >  int check_func_arg_reg_off(struct bpf_verifier_env *env,
> > >  			   const struct bpf_reg_state *reg, int regno,
> > > -			   enum bpf_arg_type arg_type);
> > > +			   enum bpf_arg_type arg_type,
> > > +			   bool is_release_function);
> > >  int check_kfunc_mem_size_reg(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
> > >  			     u32 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 7f6a0ae5028b..c9a1019dc60d 100644
> > > --- a/kernel/bpf/btf.c
> > > +++ b/kernel/bpf/btf.c
> > > @@ -5753,6 +5753,9 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
> > >  		return -EINVAL;
> > >  	}
> > >
> > > +	if (is_kfunc)
> > > +		rel = btf_kfunc_id_set_contains(btf, resolve_prog_type(env->prog),
> > > +						BTF_KFUNC_TYPE_RELEASE, func_id);
> > >  	/* check that BTF function arguments match actual types that the
> > >  	 * verifier sees.
> > >  	 */
> > > @@ -5777,7 +5780,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
> > >  		ref_t = btf_type_skip_modifiers(btf, t->type, &ref_id);
> > >  		ref_tname = btf_name_by_offset(btf, ref_t->name_off);
> > >
> > > -		ret = check_func_arg_reg_off(env, reg, regno, ARG_DONTCARE);
> > > +		ret = check_func_arg_reg_off(env, reg, regno, ARG_DONTCARE, rel);
> > >  		if (ret < 0)
> > >  			return ret;
> > >
> > > @@ -5809,7 +5812,11 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
> > >  			if (reg->type == PTR_TO_BTF_ID) {
> > >  				reg_btf = reg->btf;
> > >  				reg_ref_id = reg->btf_id;
> > > -				/* Ensure only one argument is referenced PTR_TO_BTF_ID */
> > > +				/* Ensure only one argument is referenced
> > > +				 * PTR_TO_BTF_ID, check_func_arg_reg_off relies
> > > +				 * on only one referenced register being allowed
> > > +				 * for kfuncs.
> > > +				 */
> > >  				if (reg->ref_obj_id) {
> > >  					if (ref_obj_id) {
> > >  						bpf_log(log, "verifier internal error: more than one arg with ref_obj_id R%d %u %u\n",
> > > @@ -5892,8 +5899,6 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
> > >  	/* Either both are set, or neither */
> > >  	WARN_ON_ONCE((ref_obj_id && !ref_regno) || (!ref_obj_id && ref_regno));
> > >  	if (is_kfunc) {
> > This test is no longer needed?
> >
> 
> If you mean the rel && !ref_obj_id below (which is guarded by this check), I do
> think it is needed, why do you think so? Because of the check in
> check_func_arg_reg_off? That only checks reg->off when it sees that both
> release_func and ref_obj_id are true, but ref_obj_id may not be set for any
> argument(s) passed to a release function, so we need to reject when we don't get
> atleast one referenced register for release function.
> 
> Or were you referring to the WARN_ON_ONCE above it?
I meant the "if (is_kfunc)" test.  rel can only be true
anyway when it is_kfunc.

> > > -		rel = btf_kfunc_id_set_contains(btf, resolve_prog_type(env->prog),
> > > -						BTF_KFUNC_TYPE_RELEASE, func_id);
> > >  		/* We already made sure ref_obj_id is set only for one argument */
> > >  		if (rel && !ref_obj_id) {
> > >  			bpf_log(log, "release kernel function %s expects refcounted PTR_TO_BTF_ID\n",
> > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > > index e55bfd23e81b..c31407d156e7 100644
> > > --- a/kernel/bpf/verifier.c
> > > +++ b/kernel/bpf/verifier.c
> > > @@ -5367,11 +5367,28 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno,
> > >
> > >  int check_func_arg_reg_off(struct bpf_verifier_env *env,
> > >  			   const struct bpf_reg_state *reg, int regno,
> > > -			   enum bpf_arg_type arg_type)
> > > +			   enum bpf_arg_type arg_type,
> > > +			   bool is_release_func)
> > >  {
> > >  	enum bpf_reg_type type = reg->type;
> > > +	bool fixed_off_ok = false;
> > >  	int err;
> > >
> > > +	/* When referenced PTR_TO_BTF_ID is passed to release function, it's
> > > +	 * fixed offset must be 0. We rely on the property that only one
> > > +	 * referenced register can be passed to BPF helpers and kfuncs.
> > > +	 */
> > > +	if (type == PTR_TO_BTF_ID) {
> > > +		bool release_reg = is_release_func && reg->ref_obj_id;
> > > +
> > > +		if (release_reg && reg->off) {
> > iiuc, the reason for not going through __check_ptr_off_reg() is
> > because it prefers a different verifier log message for release_reg
> > case for fixed off.  How about var_off?
> >
> 
> If reg->off is zero, we still call __check_ptr_off_reg with fixed_off_ok =
> false, which should handle non-zero var_off.
Understood that __check_ptr_off_reg handles both fixed_off and var_off case.

The question was why only single out reg->off case to have a special message
but not the var_off case.  The var_off case does not need a special message?

> 
> > > +			verbose(env, "R%d must have zero offset when passed to release func\n",
> > > +				regno);
> > > +			return -EINVAL;
> > > +		}
> > > +		fixed_off_ok = release_reg ? false : true;
> > nit.
> > 		fixed_off_ok = !release_reg;
> >
> > but this is a bit moot here considering the reg->off
> > check has already been done for the release_reg case.
> >
> 
> Yes, it would be a redundant check inside __check_ptr_off_reg, but we still need
> to call it for checking bad var_off.
Redundant check is fine.

The intention and the net effect here is fixed_off is always
allowed for the remaining case, so may as well directly set
fixed_off_ok to true.  "fixed_off_ok = !release_reg;"
made me go back to re-read what else has not been handled
for the release_reg case but it could be just me being
slow here.

It will be useful to at least leave a comment here
on the redundant check and the remaining cases for
PTR_TO_BTF_ID actually always allow fixed_off.

> 
> > > +	}
> > > +
> > >  	switch ((u32)type) {
> > >  	case SCALAR_VALUE:
> > >  	/* Pointer types where reg offset is explicitly allowed: */
> > > @@ -5394,8 +5411,7 @@ int check_func_arg_reg_off(struct bpf_verifier_env *env,
> > >  	/* All the rest must be rejected: */
> > >  	default:
> > >  force_off_check:
> > > -		err = __check_ptr_off_reg(env, reg, regno,
> > > -					  type == PTR_TO_BTF_ID);
> > > +		err = __check_ptr_off_reg(env, reg, regno, fixed_off_ok);
> > >  		if (err < 0)
> > >  			return err;
> > >  		break;
> > > @@ -5452,11 +5468,14 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> > >  	if (err)
> > >  		return err;
> > >
> > > -	err = check_func_arg_reg_off(env, reg, regno, arg_type);
> > > +	err = check_func_arg_reg_off(env, reg, regno, arg_type, is_release_function(meta->func_id));
> > >  	if (err)
> > >  		return err;
> > >
> > >  skip_type_check:
> > > +	/* check_func_arg_reg_off relies on only one referenced register being
> > > +	 * allowed for BPF helpers.
> > > +	 */
> > >  	if (reg->ref_obj_id) {
> > >  		if (meta->ref_obj_id) {
> > >  			verbose(env, "verifier internal error: more than one arg with ref_obj_id R%d %u %u\n",
> > > --
> > > 2.35.1
> > >
> 
> --
> Kartikeya

Re: [PATCH bpf-next v3 4/8] bpf: Harden register offset checks for release helpers and kfuncs

[Kumar Kartikeya Dwivedi]

On Sat, Mar 05, 2022 at 03:13:33AM IST, Martin KaFai Lau wrote:
> On Sat, Mar 05, 2022 at 02:18:56AM +0530, Kumar Kartikeya Dwivedi wrote:
> > On Sat, Mar 05, 2022 at 01:58:30AM IST, Martin KaFai Lau wrote:
> > > > diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> > > > index 38b24ee8d8c2..7a684050495a 100644
> > > > --- a/include/linux/bpf_verifier.h
> > > > +++ b/include/linux/bpf_verifier.h
> > > > @@ -523,7 +523,8 @@ int check_ptr_off_reg(struct bpf_verifier_env *env,
> > > >  		      const struct bpf_reg_state *reg, int regno);
> > > >  int check_func_arg_reg_off(struct bpf_verifier_env *env,
> > > >  			   const struct bpf_reg_state *reg, int regno,
> > > > -			   enum bpf_arg_type arg_type);
> > > > +			   enum bpf_arg_type arg_type,
> > > > +			   bool is_release_function);
> > > >  int check_kfunc_mem_size_reg(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
> > > >  			     u32 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 7f6a0ae5028b..c9a1019dc60d 100644
> > > > --- a/kernel/bpf/btf.c
> > > > +++ b/kernel/bpf/btf.c
> > > > @@ -5753,6 +5753,9 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
> > > >  		return -EINVAL;
> > > >  	}
> > > >
> > > > +	if (is_kfunc)
> > > > +		rel = btf_kfunc_id_set_contains(btf, resolve_prog_type(env->prog),
> > > > +						BTF_KFUNC_TYPE_RELEASE, func_id);
> > > >  	/* check that BTF function arguments match actual types that the
> > > >  	 * verifier sees.
> > > >  	 */
> > > > @@ -5777,7 +5780,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
> > > >  		ref_t = btf_type_skip_modifiers(btf, t->type, &ref_id);
> > > >  		ref_tname = btf_name_by_offset(btf, ref_t->name_off);
> > > >
> > > > -		ret = check_func_arg_reg_off(env, reg, regno, ARG_DONTCARE);
> > > > +		ret = check_func_arg_reg_off(env, reg, regno, ARG_DONTCARE, rel);
> > > >  		if (ret < 0)
> > > >  			return ret;
> > > >
> > > > @@ -5809,7 +5812,11 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
> > > >  			if (reg->type == PTR_TO_BTF_ID) {
> > > >  				reg_btf = reg->btf;
> > > >  				reg_ref_id = reg->btf_id;
> > > > -				/* Ensure only one argument is referenced PTR_TO_BTF_ID */
> > > > +				/* Ensure only one argument is referenced
> > > > +				 * PTR_TO_BTF_ID, check_func_arg_reg_off relies
> > > > +				 * on only one referenced register being allowed
> > > > +				 * for kfuncs.
> > > > +				 */
> > > >  				if (reg->ref_obj_id) {
> > > >  					if (ref_obj_id) {
> > > >  						bpf_log(log, "verifier internal error: more than one arg with ref_obj_id R%d %u %u\n",
> > > > @@ -5892,8 +5899,6 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
> > > >  	/* Either both are set, or neither */
> > > >  	WARN_ON_ONCE((ref_obj_id && !ref_regno) || (!ref_obj_id && ref_regno));
> > > >  	if (is_kfunc) {
> > > This test is no longer needed?
> > >
> >
> > If you mean the rel && !ref_obj_id below (which is guarded by this check), I do
> > think it is needed, why do you think so? Because of the check in
> > check_func_arg_reg_off? That only checks reg->off when it sees that both
> > release_func and ref_obj_id are true, but ref_obj_id may not be set for any
> > argument(s) passed to a release function, so we need to reject when we don't get
> > atleast one referenced register for release function.
> >
> > Or were you referring to the WARN_ON_ONCE above it?
> I meant the "if (is_kfunc)" test.  rel can only be true
> anyway when it is_kfunc.
>

Ah, indeed. I will remove it, but add a comment as well.

> > > > -		rel = btf_kfunc_id_set_contains(btf, resolve_prog_type(env->prog),
> > > > -						BTF_KFUNC_TYPE_RELEASE, func_id);
> > > >  		/* We already made sure ref_obj_id is set only for one argument */
> > > >  		if (rel && !ref_obj_id) {
> > > >  			bpf_log(log, "release kernel function %s expects refcounted PTR_TO_BTF_ID\n",
> > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > > > index e55bfd23e81b..c31407d156e7 100644
> > > > --- a/kernel/bpf/verifier.c
> > > > +++ b/kernel/bpf/verifier.c
> > > > @@ -5367,11 +5367,28 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno,
> > > >
> > > >  int check_func_arg_reg_off(struct bpf_verifier_env *env,
> > > >  			   const struct bpf_reg_state *reg, int regno,
> > > > -			   enum bpf_arg_type arg_type)
> > > > +			   enum bpf_arg_type arg_type,
> > > > +			   bool is_release_func)
> > > >  {
> > > >  	enum bpf_reg_type type = reg->type;
> > > > +	bool fixed_off_ok = false;
> > > >  	int err;
> > > >
> > > > +	/* When referenced PTR_TO_BTF_ID is passed to release function, it's
> > > > +	 * fixed offset must be 0. We rely on the property that only one
> > > > +	 * referenced register can be passed to BPF helpers and kfuncs.
> > > > +	 */
> > > > +	if (type == PTR_TO_BTF_ID) {
> > > > +		bool release_reg = is_release_func && reg->ref_obj_id;
> > > > +
> > > > +		if (release_reg && reg->off) {
> > > iiuc, the reason for not going through __check_ptr_off_reg() is
> > > because it prefers a different verifier log message for release_reg
> > > case for fixed off.  How about var_off?
> > >
> >
> > If reg->off is zero, we still call __check_ptr_off_reg with fixed_off_ok =
> > false, which should handle non-zero var_off.
> Understood that __check_ptr_off_reg handles both fixed_off and var_off case.
>
> The question was why only single out reg->off case to have a special message
> but not the var_off case.  The var_off case does not need a special message?
>

So my reasoning was, var_off is already disallowed even for normal case (it is
rejected in check_ptr_to_btf_access as well), so we don't need a special message
for that, just the existing one is fine. But reg->off is allowed, except in this
case, so we can return a helpful message on why verifier is returning an error.

> >
> > > > +			verbose(env, "R%d must have zero offset when passed to release func\n",
> > > > +				regno);
> > > > +			return -EINVAL;
> > > > +		}
> > > > +		fixed_off_ok = release_reg ? false : true;
> > > nit.
> > > 		fixed_off_ok = !release_reg;
> > >
> > > but this is a bit moot here considering the reg->off
> > > check has already been done for the release_reg case.
> > >
> >
> > Yes, it would be a redundant check inside __check_ptr_off_reg, but we still need
> > to call it for checking bad var_off.
> Redundant check is fine.
>
> The intention and the net effect here is fixed_off is always
> allowed for the remaining case, so may as well directly set
> fixed_off_ok to true.  "fixed_off_ok = !release_reg;"
> made me go back to re-read what else has not been handled
> for the release_reg case but it could be just me being
> slow here.
>

Right, I can see why that may be confusing. I just set it to !release_reg to
disable any other code that may be added using that bool later in the future.

> It will be useful to at least leave a comment here
> on the redundant check and the remaining cases for
> PTR_TO_BTF_ID actually always allow fixed_off.
>

Yes, I will add a comment to make it clearer.

Thank you for your review.

> >
> > > > +	}
> > > > +
> > > >  	switch ((u32)type) {
> > > >  	case SCALAR_VALUE:
> > > >  	/* Pointer types where reg offset is explicitly allowed: */
> > > > @@ -5394,8 +5411,7 @@ int check_func_arg_reg_off(struct bpf_verifier_env *env,
> > > >  	/* All the rest must be rejected: */
> > > >  	default:
> > > >  force_off_check:
> > > > -		err = __check_ptr_off_reg(env, reg, regno,
> > > > -					  type == PTR_TO_BTF_ID);
> > > > +		err = __check_ptr_off_reg(env, reg, regno, fixed_off_ok);
> > > >  		if (err < 0)
> > > >  			return err;
> > > >  		break;
> > > > @@ -5452,11 +5468,14 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> > > >  	if (err)
> > > >  		return err;
> > > >
> > > > -	err = check_func_arg_reg_off(env, reg, regno, arg_type);
> > > > +	err = check_func_arg_reg_off(env, reg, regno, arg_type, is_release_function(meta->func_id));
> > > >  	if (err)
> > > >  		return err;
> > > >
> > > >  skip_type_check:
> > > > +	/* check_func_arg_reg_off relies on only one referenced register being
> > > > +	 * allowed for BPF helpers.
> > > > +	 */
> > > >  	if (reg->ref_obj_id) {
> > > >  		if (meta->ref_obj_id) {
> > > >  			verbose(env, "verifier internal error: more than one arg with ref_obj_id R%d %u %u\n",
> > > > --
> > > > 2.35.1
> > > >
> >
> > --
> > Kartikeya

--
Kartikeya

Re: [PATCH bpf-next v3 4/8] bpf: Harden register offset checks for release helpers and kfuncs

[Martin KaFai Lau]

On Sat, Mar 05, 2022 at 03:25:56AM +0530, Kumar Kartikeya Dwivedi wrote:
> On Sat, Mar 05, 2022 at 03:13:33AM IST, Martin KaFai Lau wrote:
> > On Sat, Mar 05, 2022 at 02:18:56AM +0530, Kumar Kartikeya Dwivedi wrote:
> > > On Sat, Mar 05, 2022 at 01:58:30AM IST, Martin KaFai Lau wrote:

> > > > > +			verbose(env, "R%d must have zero offset when passed to release func\n",
> > > > > +				regno);
> > > > > +			return -EINVAL;
> > > > > +		}
> > > > > +		fixed_off_ok = release_reg ? false : true;
> > > > nit.
> > > > 		fixed_off_ok = !release_reg;
> > > >
> > > > but this is a bit moot here considering the reg->off
> > > > check has already been done for the release_reg case.
> > > >
> > >
> > > Yes, it would be a redundant check inside __check_ptr_off_reg, but we still need
> > > to call it for checking bad var_off.
> > Redundant check is fine.
> >
> > The intention and the net effect here is fixed_off is always
> > allowed for the remaining case, so may as well directly set
> > fixed_off_ok to true.  "fixed_off_ok = !release_reg;"
> > made me go back to re-read what else has not been handled
> > for the release_reg case but it could be just me being
> > slow here.
> >
> 
> Right, I can see why that may be confusing. I just set it to !release_reg to
> disable any other code that may be added using that bool later in the future.
hmm... If the concern is on future code,
how about using a comment to remind future cases instead
and directly set it to true?

/* All special cases were handled above, the remaining
 * PTR_TO_BTF_ID case always allows fixed off.
 */
fixed_off_ok = true;


> 
> > It will be useful to at least leave a comment here
> > on the redundant check and the remaining cases for
> > PTR_TO_BTF_ID actually always allow fixed_off.
> >
> 
> Yes, I will add a comment to make it clearer.

Re: [PATCH bpf-next v3 4/8] bpf: Harden register offset checks for release helpers and kfuncs

[Kumar Kartikeya Dwivedi]

On Sat, Mar 05, 2022 at 03:48:52AM IST, Martin KaFai Lau wrote:
> On Sat, Mar 05, 2022 at 03:25:56AM +0530, Kumar Kartikeya Dwivedi wrote:
> > On Sat, Mar 05, 2022 at 03:13:33AM IST, Martin KaFai Lau wrote:
> > > On Sat, Mar 05, 2022 at 02:18:56AM +0530, Kumar Kartikeya Dwivedi wrote:
> > > > On Sat, Mar 05, 2022 at 01:58:30AM IST, Martin KaFai Lau wrote:
>
> > > > > > +			verbose(env, "R%d must have zero offset when passed to release func\n",
> > > > > > +				regno);
> > > > > > +			return -EINVAL;
> > > > > > +		}
> > > > > > +		fixed_off_ok = release_reg ? false : true;
> > > > > nit.
> > > > > 		fixed_off_ok = !release_reg;
> > > > >
> > > > > but this is a bit moot here considering the reg->off
> > > > > check has already been done for the release_reg case.
> > > > >
> > > >
> > > > Yes, it would be a redundant check inside __check_ptr_off_reg, but we still need
> > > > to call it for checking bad var_off.
> > > Redundant check is fine.
> > >
> > > The intention and the net effect here is fixed_off is always
> > > allowed for the remaining case, so may as well directly set
> > > fixed_off_ok to true.  "fixed_off_ok = !release_reg;"
> > > made me go back to re-read what else has not been handled
> > > for the release_reg case but it could be just me being
> > > slow here.
> > >
> >
> > Right, I can see why that may be confusing. I just set it to !release_reg to
> > disable any other code that may be added using that bool later in the future.
> hmm... If the concern is on future code,
> how about using a comment to remind future cases instead
> and directly set it to true?
>
> /* All special cases were handled above, the remaining
>  * PTR_TO_BTF_ID case always allows fixed off.
>  */
> fixed_off_ok = true;
>
>

Fine by me.

> >
> > > It will be useful to at least leave a comment here
> > > on the redundant check and the remaining cases for
> > > PTR_TO_BTF_ID actually always allow fixed_off.
> > >
> >
> > Yes, I will add a comment to make it clearer.

--
Kartikeya