[PATCH bpf-next v1 0/6] Fixes for bad PTR_TO_BTF_ID offset

[PATCH bpf-next v1 0/6] 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.

Kumar Kartikeya Dwivedi (6):
  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 kfunc
  selftests/bpf: Update tests for new errstr
  selftests/bpf: Add tests for kfunc register offset checks

 include/linux/bpf_verifier.h                  |  3 +
 kernel/bpf/btf.c                              | 24 ++++--
 kernel/bpf/verifier.c                         | 75 ++++++++++-------
 net/bpf/test_run.c                            | 11 +++
 .../selftests/bpf/verifier/bounds_deduction.c |  2 +-
 tools/testing/selftests/bpf/verifier/calls.c  | 82 +++++++++++++++++++
 tools/testing/selftests/bpf/verifier/ctx.c    |  8 +-
 7 files changed, 167 insertions(+), 38 deletions(-)

-- 
2.35.1

[PATCH bpf-next v1 1/6] 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 d7473fee247c..a641e61767b4 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -5353,6 +5353,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)
@@ -5402,34 +5440,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 v1 2/6] 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 v1 3/6] 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.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 kernel/bpf/verifier.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index a641e61767b4..9f12a343bb6e 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -3984,6 +3984,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);
-- 
2.35.1

[PATCH bpf-next v1 4/6] bpf: Harden register offset checks for release kfunc

[Kumar Kartikeya Dwivedi]

Let's ensure that the PTR_TO_BTF_ID reg being passed in to release kfunc
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.

Previous commits already protected against non-zero var_off. The case we
are concerned about now is when we have a type that can be returned by
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>
---
 kernel/bpf/btf.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 7f6a0ae5028b..ba6845225b65 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.
 	 */
@@ -5816,6 +5819,16 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 							regno, reg->ref_obj_id, ref_obj_id);
 						return -EFAULT;
 					}
+					/* Ensure that offset of referenced PTR_TO_BTF_ID is
+					 * always zero, when passed to release function.
+					 * var_off has already been checked to be 0 by
+					 * check_func_arg_reg_off.
+					 */
+					if (rel && reg->off) {
+						bpf_log(log, "R%d with ref_obj_id=%d must have zero offset when passed to release kfunc\n",
+							regno, reg->ref_obj_id);
+						return -EINVAL;
+					}
 					ref_regno = regno;
 					ref_obj_id = reg->ref_obj_id;
 				}
@@ -5892,8 +5905,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",
-- 
2.35.1

[PATCH bpf-next v1 5/6] selftests/bpf: Update tests for new errstr

[Kumar Kartikeya Dwivedi]

Verifier for negative offset case returns a different, more clear error
message. Update existing verifier selftests to work with that.

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

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 23080862aafd..e47a001c2bcd 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 v1 6/6] 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 | 82 ++++++++++++++++++++
 2 files changed, 93 insertions(+)

diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
index f08034500813..21e1c2d64f25 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -265,9 +265,14 @@ struct sock * noinline bpf_kfunc_call_test3(struct sock *sk)
 	return sk;
 }
 
+struct prog_test_member {
+	unsigned long c;
+};
+
 struct prog_test_ref_kfunc {
 	int a;
 	int b;
+	struct prog_test_member memb;
 	struct prog_test_ref_kfunc *next;
 };
 
@@ -290,6 +295,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 {
@@ -374,6 +383,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)
@@ -391,6 +401,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 0a8ea60c2a80..985183d1310a 100644
--- a/tools/testing/selftests/bpf/verifier/calls.c
+++ b/tools/testing/selftests/bpf/verifier/calls.c
@@ -115,6 +115,88 @@
 		{ "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 with ref_obj_id=2 must have zero offset when passed to release kfunc",
+	.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_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", 8 },
+	},
+	.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 v1 6/6] selftests/bpf: Add tests for kfunc register offset checks

[kernel test robot]

Hi Kumar,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on bpf-next/master]

url:    https://github.com/0day-ci/linux/commits/Kumar-Kartikeya-Dwivedi/Fixes-for-bad-PTR_TO_BTF_ID-offset/20220301-150010
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: s390-randconfig-r021-20220301 (https://download.01.org/0day-ci/archive/20220301/202203011937.wMLpkfU3-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project d271fc04d5b97b12e6b797c6067d3c96a8d7470e)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install s390 cross compiling tool for clang build
        # apt-get install binutils-s390x-linux-gnu
        # https://github.com/0day-ci/linux/commit/37a0d686bce3b71b14a17ae57364ec45d1405b9e
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Kumar-Kartikeya-Dwivedi/Fixes-for-bad-PTR_TO_BTF_ID-offset/20220301-150010
        git checkout 37a0d686bce3b71b14a17ae57364ec45d1405b9e
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=s390 SHELL=/bin/bash net/bpf/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

           writesl(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   net/bpf/test_run.c:201:14: warning: no previous prototype for function 'bpf_fentry_test1' [-Wmissing-prototypes]
   int noinline bpf_fentry_test1(int a)
                ^
   net/bpf/test_run.c:201:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int noinline bpf_fentry_test1(int a)
   ^
   static 
   net/bpf/test_run.c:208:14: warning: no previous prototype for function 'bpf_fentry_test2' [-Wmissing-prototypes]
   int noinline bpf_fentry_test2(int a, u64 b)
                ^
   net/bpf/test_run.c:208:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int noinline bpf_fentry_test2(int a, u64 b)
   ^
   static 
   net/bpf/test_run.c:213:14: warning: no previous prototype for function 'bpf_fentry_test3' [-Wmissing-prototypes]
   int noinline bpf_fentry_test3(char a, int b, u64 c)
                ^
   net/bpf/test_run.c:213:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int noinline bpf_fentry_test3(char a, int b, u64 c)
   ^
   static 
   net/bpf/test_run.c:218:14: warning: no previous prototype for function 'bpf_fentry_test4' [-Wmissing-prototypes]
   int noinline bpf_fentry_test4(void *a, char b, int c, u64 d)
                ^
   net/bpf/test_run.c:218:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int noinline bpf_fentry_test4(void *a, char b, int c, u64 d)
   ^
   static 
   net/bpf/test_run.c:223:14: warning: no previous prototype for function 'bpf_fentry_test5' [-Wmissing-prototypes]
   int noinline bpf_fentry_test5(u64 a, void *b, short c, int d, u64 e)
                ^
   net/bpf/test_run.c:223:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int noinline bpf_fentry_test5(u64 a, void *b, short c, int d, u64 e)
   ^
   static 
   net/bpf/test_run.c:228:14: warning: no previous prototype for function 'bpf_fentry_test6' [-Wmissing-prototypes]
   int noinline bpf_fentry_test6(u64 a, void *b, short c, int d, void *e, u64 f)
                ^
   net/bpf/test_run.c:228:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int noinline bpf_fentry_test6(u64 a, void *b, short c, int d, void *e, u64 f)
   ^
   static 
   net/bpf/test_run.c:237:14: warning: no previous prototype for function 'bpf_fentry_test7' [-Wmissing-prototypes]
   int noinline bpf_fentry_test7(struct bpf_fentry_test_t *arg)
                ^
   net/bpf/test_run.c:237:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int noinline bpf_fentry_test7(struct bpf_fentry_test_t *arg)
   ^
   static 
   net/bpf/test_run.c:242:14: warning: no previous prototype for function 'bpf_fentry_test8' [-Wmissing-prototypes]
   int noinline bpf_fentry_test8(struct bpf_fentry_test_t *arg)
                ^
   net/bpf/test_run.c:242:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int noinline bpf_fentry_test8(struct bpf_fentry_test_t *arg)
   ^
   static 
   net/bpf/test_run.c:247:14: warning: no previous prototype for function 'bpf_modify_return_test' [-Wmissing-prototypes]
   int noinline bpf_modify_return_test(int a, int *b)
                ^
   net/bpf/test_run.c:247:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int noinline bpf_modify_return_test(int a, int *b)
   ^
   static 
   net/bpf/test_run.c:253:14: warning: no previous prototype for function 'bpf_kfunc_call_test1' [-Wmissing-prototypes]
   u64 noinline bpf_kfunc_call_test1(struct sock *sk, u32 a, u64 b, u32 c, u64 d)
                ^
   net/bpf/test_run.c:253:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   u64 noinline bpf_kfunc_call_test1(struct sock *sk, u32 a, u64 b, u32 c, u64 d)
   ^
   static 
   net/bpf/test_run.c:258:14: warning: no previous prototype for function 'bpf_kfunc_call_test2' [-Wmissing-prototypes]
   int noinline bpf_kfunc_call_test2(struct sock *sk, u32 a, u32 b)
                ^
   net/bpf/test_run.c:258:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int noinline bpf_kfunc_call_test2(struct sock *sk, u32 a, u32 b)
   ^
   static 
   net/bpf/test_run.c:263:24: warning: no previous prototype for function 'bpf_kfunc_call_test3' [-Wmissing-prototypes]
   struct sock * noinline bpf_kfunc_call_test3(struct sock *sk)
                          ^
   net/bpf/test_run.c:263:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   struct sock * noinline bpf_kfunc_call_test3(struct sock *sk)
   ^
   static 
   net/bpf/test_run.c:286:1: warning: no previous prototype for function 'bpf_kfunc_call_test_acquire' [-Wmissing-prototypes]
   bpf_kfunc_call_test_acquire(unsigned long *scalar_ptr)
   ^
   net/bpf/test_run.c:285:10: note: declare 'static' if the function is not intended to be used outside of this translation unit
   noinline struct prog_test_ref_kfunc *
            ^
            static 
   net/bpf/test_run.c:294:15: warning: no previous prototype for function 'bpf_kfunc_call_test_release' [-Wmissing-prototypes]
   noinline void bpf_kfunc_call_test_release(struct prog_test_ref_kfunc *p)
                 ^
   net/bpf/test_run.c:294:10: note: declare 'static' if the function is not intended to be used outside of this translation unit
   noinline void bpf_kfunc_call_test_release(struct prog_test_ref_kfunc *p)
            ^
            static 
>> net/bpf/test_run.c:298:15: warning: no previous prototype for function 'bpf_kfunc_call_memb_release' [-Wmissing-prototypes]
   noinline void bpf_kfunc_call_memb_release(struct prog_test_member *p)
                 ^
   net/bpf/test_run.c:298:10: note: declare 'static' if the function is not intended to be used outside of this translation unit
   noinline void bpf_kfunc_call_memb_release(struct prog_test_member *p)
            ^
            static 
   net/bpf/test_run.c:340:15: warning: no previous prototype for function 'bpf_kfunc_call_test_pass_ctx' [-Wmissing-prototypes]
   noinline void bpf_kfunc_call_test_pass_ctx(struct __sk_buff *skb)
                 ^
   net/bpf/test_run.c:340:10: note: declare 'static' if the function is not intended to be used outside of this translation unit
   noinline void bpf_kfunc_call_test_pass_ctx(struct __sk_buff *skb)
            ^
            static 
   net/bpf/test_run.c:344:15: warning: no previous prototype for function 'bpf_kfunc_call_test_pass1' [-Wmissing-prototypes]
   noinline void bpf_kfunc_call_test_pass1(struct prog_test_pass1 *p)
                 ^
   net/bpf/test_run.c:344:10: note: declare 'static' if the function is not intended to be used outside of this translation unit
   noinline void bpf_kfunc_call_test_pass1(struct prog_test_pass1 *p)
            ^
            static 
   net/bpf/test_run.c:348:15: warning: no previous prototype for function 'bpf_kfunc_call_test_pass2' [-Wmissing-prototypes]
   noinline void bpf_kfunc_call_test_pass2(struct prog_test_pass2 *p)
                 ^
   net/bpf/test_run.c:348:10: note: declare 'static' if the function is not intended to be used outside of this translation unit
   noinline void bpf_kfunc_call_test_pass2(struct prog_test_pass2 *p)
            ^
            static 
   net/bpf/test_run.c:352:15: warning: no previous prototype for function 'bpf_kfunc_call_test_fail1' [-Wmissing-prototypes]
   noinline void bpf_kfunc_call_test_fail1(struct prog_test_fail1 *p)
                 ^
   net/bpf/test_run.c:352:10: note: declare 'static' if the function is not intended to be used outside of this translation unit
   noinline void bpf_kfunc_call_test_fail1(struct prog_test_fail1 *p)
            ^
            static 
   net/bpf/test_run.c:356:15: warning: no previous prototype for function 'bpf_kfunc_call_test_fail2' [-Wmissing-prototypes]
   noinline void bpf_kfunc_call_test_fail2(struct prog_test_fail2 *p)
                 ^
   net/bpf/test_run.c:356:10: note: declare 'static' if the function is not intended to be used outside of this translation unit
   noinline void bpf_kfunc_call_test_fail2(struct prog_test_fail2 *p)
            ^
            static 
   net/bpf/test_run.c:360:15: warning: no previous prototype for function 'bpf_kfunc_call_test_fail3' [-Wmissing-prototypes]
   noinline void bpf_kfunc_call_test_fail3(struct prog_test_fail3 *p)
                 ^
   net/bpf/test_run.c:360:10: note: declare 'static' if the function is not intended to be used outside of this translation unit
   noinline void bpf_kfunc_call_test_fail3(struct prog_test_fail3 *p)
            ^
            static 
   net/bpf/test_run.c:364:15: warning: no previous prototype for function 'bpf_kfunc_call_test_mem_len_pass1' [-Wmissing-prototypes]
   noinline void bpf_kfunc_call_test_mem_len_pass1(void *mem, int mem__sz)
                 ^
   net/bpf/test_run.c:364:10: note: declare 'static' if the function is not intended to be used outside of this translation unit
   noinline void bpf_kfunc_call_test_mem_len_pass1(void *mem, int mem__sz)
            ^
            static 
   net/bpf/test_run.c:368:15: warning: no previous prototype for function 'bpf_kfunc_call_test_mem_len_fail1' [-Wmissing-prototypes]
   noinline void bpf_kfunc_call_test_mem_len_fail1(void *mem, int len)
                 ^
   net/bpf/test_run.c:368:10: note: declare 'static' if the function is not intended to be used outside of this translation unit
   noinline void bpf_kfunc_call_test_mem_len_fail1(void *mem, int len)
            ^
            static 
   net/bpf/test_run.c:372:15: warning: no previous prototype for function 'bpf_kfunc_call_test_mem_len_fail2' [-Wmissing-prototypes]
   noinline void bpf_kfunc_call_test_mem_len_fail2(u64 *mem, int len)
                 ^
   net/bpf/test_run.c:372:10: note: declare 'static' if the function is not intended to be used outside of this translation unit
   noinline void bpf_kfunc_call_test_mem_len_fail2(u64 *mem, int len)
            ^
            static 
   36 warnings generated.


vim +/bpf_kfunc_call_memb_release +298 net/bpf/test_run.c

   297	
 > 298	noinline void bpf_kfunc_call_memb_release(struct prog_test_member *p)
   299	{
   300	}
   301	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH bpf-next v1 6/6] selftests/bpf: Add tests for kfunc register offset checks

[Kumar Kartikeya Dwivedi]

On Tue, Mar 01, 2022 at 05:10:31PM IST, kernel test robot wrote:
> Hi Kumar,
>
> Thank you for the patch! Perhaps something to improve:
>
> [auto build test WARNING on bpf-next/master]
>
> url:    https://github.com/0day-ci/linux/commits/Kumar-Kartikeya-Dwivedi/Fixes-for-bad-PTR_TO_BTF_ID-offset/20220301-150010
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
> config: s390-randconfig-r021-20220301 (https://download.01.org/0day-ci/archive/20220301/202203011937.wMLpkfU3-lkp@intel.com/config)
> compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project d271fc04d5b97b12e6b797c6067d3c96a8d7470e)

The same warning is emitted on clang for all existing definitions, so I can
respin with a fix for the warning like we do for GCC, otherwise it can also
be a follow up patch.

> [...]

--
Kartikeya

Re: [PATCH bpf-next v1 6/6] selftests/bpf: Add tests for kfunc register offset checks

[Kumar Kartikeya Dwivedi]

[-- Attachment #1: Type: text/plain, Size: 860 bytes --]

On Tue, Mar 01, 2022 at 05:10:31PM IST, kernel test robot wrote:
> Hi Kumar,
>
> Thank you for the patch! Perhaps something to improve:
>
> [auto build test WARNING on bpf-next/master]
>
> url:    https://github.com/0day-ci/linux/commits/Kumar-Kartikeya-Dwivedi/Fixes-for-bad-PTR_TO_BTF_ID-offset/20220301-150010
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
> config: s390-randconfig-r021-20220301 (https://download.01.org/0day-ci/archive/20220301/202203011937.wMLpkfU3-lkp(a)intel.com/config)
> compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project d271fc04d5b97b12e6b797c6067d3c96a8d7470e)

The same warning is emitted on clang for all existing definitions, so I can
respin with a fix for the warning like we do for GCC, otherwise it can also
be a follow up patch.

> [...]

--
Kartikeya

Re: [PATCH bpf-next v1 4/6] bpf: Harden register offset checks for release kfunc

[Martin KaFai Lau]

On Tue, Mar 01, 2022 at 12:27:43PM +0530, Kumar Kartikeya Dwivedi wrote:
> Let's ensure that the PTR_TO_BTF_ID reg being passed in to release kfunc
> 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.
> 
> Previous commits already protected against non-zero var_off. The case we
> are concerned about now is when we have a type that can be returned by
> 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>
> ---
>  kernel/bpf/btf.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 7f6a0ae5028b..ba6845225b65 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.
>  	 */
> @@ -5816,6 +5819,16 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
>  							regno, reg->ref_obj_id, ref_obj_id);
>  						return -EFAULT;
>  					}
> +					/* Ensure that offset of referenced PTR_TO_BTF_ID is
> +					 * always zero, when passed to release function.
> +					 * var_off has already been checked to be 0 by
> +					 * check_func_arg_reg_off.
> +					 */
> +					if (rel && reg->off) {
Here is another reg->off check for PTR_TO_BTF_ID on top of the
one 'check_func_arg_reg_off' added to the same function in patch 2.
A nit, I also found passing ARG_DONTCARE in patch 2 a bit convoluted
considering the btf func does not need ARG_* to begin with.

How about directly use the __check_ptr_off_reg() here instead of
check_func_arg_reg_off()?  Then patch 1 is not needed.

Would something like this do the same thing (uncompiled code) ?

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 7f6a0ae5028b..768cef4de4cc 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -5794,6 +5797,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 			}
 		} else if (is_kfunc && (reg->type == PTR_TO_BTF_ID ||
 			   (reg2btf_ids[base_type(reg->type)] && !type_flag(reg->type)))) {
+			bool fixed_off_ok = reg->type == PTR_TO_BTF_ID;
 			const struct btf_type *reg_ref_t;
 			const struct btf *reg_btf;
 			const char *reg_ref_tname;
@@ -5816,6 +5820,13 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 							regno, reg->ref_obj_id, ref_obj_id);
 						return -EFAULT;
 					}
+					/* Ensure that offset of referenced PTR_TO_BTF_ID is
+					 * always zero, when passed to release function.
+					 * var_off has already been checked to be 0 by
+					 * check_func_arg_reg_off.
+					 */
+					if (rel)
+						fixed_off_ok = false;
 					ref_regno = regno;
 					ref_obj_id = reg->ref_obj_id;
 				}
@@ -5824,6 +5835,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 				reg_ref_id = *reg2btf_ids[base_type(reg->type)];
 			}
 
+			__check_ptr_off_reg(env, reg, regno, fixed_off_ok);
 			reg_ref_t = btf_type_skip_modifiers(reg_btf, reg_ref_id,
 							    &reg_ref_id);
 			reg_ref_tname = btf_name_by_offset(reg_btf,

Re: [PATCH bpf-next v1 4/6] bpf: Harden register offset checks for release kfunc

[Kumar Kartikeya Dwivedi]

On Wed, Mar 02, 2022 at 08:50:24AM IST, Martin KaFai Lau wrote:
> On Tue, Mar 01, 2022 at 12:27:43PM +0530, Kumar Kartikeya Dwivedi wrote:
> > Let's ensure that the PTR_TO_BTF_ID reg being passed in to release kfunc
> > 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.
> >
> > Previous commits already protected against non-zero var_off. The case we
> > are concerned about now is when we have a type that can be returned by
> > 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>
> > ---
> >  kernel/bpf/btf.c | 15 +++++++++++++--
> >  1 file changed, 13 insertions(+), 2 deletions(-)
> >
> > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > index 7f6a0ae5028b..ba6845225b65 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.
> >  	 */
> > @@ -5816,6 +5819,16 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
> >  							regno, reg->ref_obj_id, ref_obj_id);
> >  						return -EFAULT;
> >  					}
> > +					/* Ensure that offset of referenced PTR_TO_BTF_ID is
> > +					 * always zero, when passed to release function.
> > +					 * var_off has already been checked to be 0 by
> > +					 * check_func_arg_reg_off.
> > +					 */
> > +					if (rel && reg->off) {
> Here is another reg->off check for PTR_TO_BTF_ID on top of the
> one 'check_func_arg_reg_off' added to the same function in patch 2.
> A nit, I also found passing ARG_DONTCARE in patch 2 a bit convoluted
> considering the btf func does not need ARG_* to begin with.
>

Right, arg_type doesn't really matter here (unless we start indicating in BTF we
want to take ringbuf allocation directly without size parameter or getting size
from BTF type).

> How about directly use the __check_ptr_off_reg() here instead of
> check_func_arg_reg_off()?  Then patch 1 is not needed.
>
> Would something like this do the same thing (uncompiled code) ?
>

I should have included a link to the previous discussion, sorry about that:
https://lore.kernel.org/bpf/20220223031600.pvbhu3dbwxke4eia@apollo.legion

Yes, this should also do the same thing, but the idea was to avoid keeping the
same checks in multiple places. For now, there is only the special case of
ARG_TYPE_PTR_TO_ALLOC_MEM and PTR_TO_BTF_ID that require some special handling,
the former of which is currently not relevant for kfunc, but adding some future
type and ensuring kfunc, and helper do the offset checks correctly just means
updating check_func_arg_reg_off.

reg->off in case of PTR_TO_BTF_ID reg for release kfunc is a bit of a special
case. We should also do the same thing for BPF helpers, now that I look at it,
but there's only one which takes a PTR_TO_BTF_ID right now (bpf_sk_release), and
it isn't problematic currently, but now that referenced PTR_TO_BTF_ID is used it
is quite possible to support it in more BPF helpers later and forget to prevent
such case.

So, it would be possible to move this check inside check_func_arg_reg_off, based
on a new bool is_release_func parameter, and relying on the assumption that only
one referenced register can be passed to helper or kfunc at a time (already
enforced for both BPF helpers and kfuncs).

Basically, instead of doing type == PTR_TO_BTF_ID for fixed_off_ok inside it, we
will do:

	fixed_off_ok = false;
	if (type == PTR_TO_BTF_ID && (!is_release_func || !reg->ref_obj_id))
		fixed_off_ok = true;

Again, given we can only pass one referenced reg, if we see release func and a
reg with ref_obj_id, it is the one being released.

In the end, it's more of a preference thing, if you feel strongly about it I can
go with the __check_ptr_off_reg call too.

> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 7f6a0ae5028b..768cef4de4cc 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -5794,6 +5797,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
>  			}
>  		} else if (is_kfunc && (reg->type == PTR_TO_BTF_ID ||
>  			   (reg2btf_ids[base_type(reg->type)] && !type_flag(reg->type)))) {
> +			bool fixed_off_ok = reg->type == PTR_TO_BTF_ID;
>  			const struct btf_type *reg_ref_t;
>  			const struct btf *reg_btf;
>  			const char *reg_ref_tname;
> @@ -5816,6 +5820,13 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
>  							regno, reg->ref_obj_id, ref_obj_id);
>  						return -EFAULT;
>  					}
> +					/* Ensure that offset of referenced PTR_TO_BTF_ID is
> +					 * always zero, when passed to release function.
> +					 * var_off has already been checked to be 0 by
> +					 * check_func_arg_reg_off.
> +					 */
> +					if (rel)
> +						fixed_off_ok = false;
>  					ref_regno = regno;
>  					ref_obj_id = reg->ref_obj_id;
>  				}
> @@ -5824,6 +5835,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
>  				reg_ref_id = *reg2btf_ids[base_type(reg->type)];
>  			}
>
> +			__check_ptr_off_reg(env, reg, regno, fixed_off_ok);
>  			reg_ref_t = btf_type_skip_modifiers(reg_btf, reg_ref_id,
>  							    &reg_ref_id);
>  			reg_ref_tname = btf_name_by_offset(reg_btf,
>

--
Kartikeya

Re: [PATCH bpf-next v1 4/6] bpf: Harden register offset checks for release kfunc

[Martin KaFai Lau]

On Wed, Mar 02, 2022 at 03:12:18PM +0530, Kumar Kartikeya Dwivedi wrote:
> On Wed, Mar 02, 2022 at 08:50:24AM IST, Martin KaFai Lau wrote:
> > On Tue, Mar 01, 2022 at 12:27:43PM +0530, Kumar Kartikeya Dwivedi wrote:
> > > Let's ensure that the PTR_TO_BTF_ID reg being passed in to release kfunc
> > > 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.
> > >
> > > Previous commits already protected against non-zero var_off. The case we
> > > are concerned about now is when we have a type that can be returned by
> > > 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>
> > > ---
> > >  kernel/bpf/btf.c | 15 +++++++++++++--
> > >  1 file changed, 13 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > > index 7f6a0ae5028b..ba6845225b65 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.
> > >  	 */
> > > @@ -5816,6 +5819,16 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
> > >  							regno, reg->ref_obj_id, ref_obj_id);
> > >  						return -EFAULT;
> > >  					}
> > > +					/* Ensure that offset of referenced PTR_TO_BTF_ID is
> > > +					 * always zero, when passed to release function.
> > > +					 * var_off has already been checked to be 0 by
> > > +					 * check_func_arg_reg_off.
> > > +					 */
> > > +					if (rel && reg->off) {
> > Here is another reg->off check for PTR_TO_BTF_ID on top of the
> > one 'check_func_arg_reg_off' added to the same function in patch 2.
> > A nit, I also found passing ARG_DONTCARE in patch 2 a bit convoluted
> > considering the btf func does not need ARG_* to begin with.
> >
> 
> Right, arg_type doesn't really matter here (unless we start indicating in BTF we
> want to take ringbuf allocation directly without size parameter or getting size
> from BTF type).
> 
> > How about directly use the __check_ptr_off_reg() here instead of
> > check_func_arg_reg_off()?  Then patch 1 is not needed.
> >
> > Would something like this do the same thing (uncompiled code) ?
> >
> 
> I should have included a link to the previous discussion, sorry about that:
> https://lore.kernel.org/bpf/20220223031600.pvbhu3dbwxke4eia@apollo.legion
Ah. Thanks for the link.  I didn't go back to the list since the set is
tagged v1 ;)

> Yes, this should also do the same thing, but the idea was to avoid keeping the
> same checks in multiple places. For now, there is only the special case of
> ARG_TYPE_PTR_TO_ALLOC_MEM and PTR_TO_BTF_ID that require some special handling,
> the former of which is currently not relevant for kfunc, but adding some future
> type and ensuring kfunc, and helper do the offset checks correctly just means
> updating check_func_arg_reg_off.
> 
> reg->off in case of PTR_TO_BTF_ID reg for release kfunc is a bit of a special
> case. We should also do the same thing for BPF helpers, now that I look at it,
> but there's only one which takes a PTR_TO_BTF_ID right now (bpf_sk_release), and
> it isn't problematic currently, but now that referenced PTR_TO_BTF_ID is used it
> is quite possible to support it in more BPF helpers later and forget to prevent
> such case.
> 
> So, it would be possible to move this check inside check_func_arg_reg_off, based
> on a new bool is_release_func parameter, and relying on the assumption that only
> one referenced register can be passed to helper or kfunc at a time (already
> enforced for both BPF helpers and kfuncs).
> 
> Basically, instead of doing type == PTR_TO_BTF_ID for fixed_off_ok inside it, we
> will do:
> 
> 	fixed_off_ok = false;
> 	if (type == PTR_TO_BTF_ID && (!is_release_func || !reg->ref_obj_id))
> 		fixed_off_ok = true;
For the preemptive fix on release func and non zero reg->off,
should it be a release-without-acquire error instead of a ptr-type/reg->off error?
The fix should be either clearing the reg->ref_obj_id earlier or at least treat
ref_obj_id as zero here and then fallthrough the existing release-without-acquire
error.  It is more to do with the ref_obj_id becomes invalid after reg->off
becoming non-zero instead of reg->off is not allowed for a specific ptr
type.  It is better to separate this preemptive fix to another set.

> 
> Again, given we can only pass one referenced reg, if we see release func and a
> reg with ref_obj_id, it is the one being released.
> 
> In the end, it's more of a preference thing, if you feel strongly about it I can
> go with the __check_ptr_off_reg call too.
Yeah, it is a preference thing and not feeling strongly.  
Without the need for the release-func/reg->off preemptive fix, adding
one __check_ptr_off_reg() seems to be a cleaner fix to me but
I won't insist.

Re: [PATCH bpf-next v1 4/6] bpf: Harden register offset checks for release kfunc

[Kumar Kartikeya Dwivedi]

On Thu, Mar 03, 2022 at 03:26:40AM IST, Martin KaFai Lau wrote:
> On Wed, Mar 02, 2022 at 03:12:18PM +0530, Kumar Kartikeya Dwivedi wrote:
> > On Wed, Mar 02, 2022 at 08:50:24AM IST, Martin KaFai Lau wrote:
> > > On Tue, Mar 01, 2022 at 12:27:43PM +0530, Kumar Kartikeya Dwivedi wrote:
> > > > Let's ensure that the PTR_TO_BTF_ID reg being passed in to release kfunc
> > > > 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.
> > > >
> > > > Previous commits already protected against non-zero var_off. The case we
> > > > are concerned about now is when we have a type that can be returned by
> > > > 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>
> > > > ---
> > > >  kernel/bpf/btf.c | 15 +++++++++++++--
> > > >  1 file changed, 13 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > > > index 7f6a0ae5028b..ba6845225b65 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.
> > > >  	 */
> > > > @@ -5816,6 +5819,16 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
> > > >  							regno, reg->ref_obj_id, ref_obj_id);
> > > >  						return -EFAULT;
> > > >  					}
> > > > +					/* Ensure that offset of referenced PTR_TO_BTF_ID is
> > > > +					 * always zero, when passed to release function.
> > > > +					 * var_off has already been checked to be 0 by
> > > > +					 * check_func_arg_reg_off.
> > > > +					 */
> > > > +					if (rel && reg->off) {
> > > Here is another reg->off check for PTR_TO_BTF_ID on top of the
> > > one 'check_func_arg_reg_off' added to the same function in patch 2.
> > > A nit, I also found passing ARG_DONTCARE in patch 2 a bit convoluted
> > > considering the btf func does not need ARG_* to begin with.
> > >
> >
> > Right, arg_type doesn't really matter here (unless we start indicating in BTF we
> > want to take ringbuf allocation directly without size parameter or getting size
> > from BTF type).
> >
> > > How about directly use the __check_ptr_off_reg() here instead of
> > > check_func_arg_reg_off()?  Then patch 1 is not needed.
> > >
> > > Would something like this do the same thing (uncompiled code) ?
> > >
> >
> > I should have included a link to the previous discussion, sorry about that:
> > https://lore.kernel.org/bpf/20220223031600.pvbhu3dbwxke4eia@apollo.legion
> Ah. Thanks for the link.  I didn't go back to the list since the set is
> tagged v1 ;)
>

Right, I split the first patch out and then added this patch, so it felt more
appropriate to tag it as v1. But I will include this link in the cover letter
going forward.

> > Yes, this should also do the same thing, but the idea was to avoid keeping the
> > same checks in multiple places. For now, there is only the special case of
> > ARG_TYPE_PTR_TO_ALLOC_MEM and PTR_TO_BTF_ID that require some special handling,
> > the former of which is currently not relevant for kfunc, but adding some future
> > type and ensuring kfunc, and helper do the offset checks correctly just means
> > updating check_func_arg_reg_off.
> >
> > reg->off in case of PTR_TO_BTF_ID reg for release kfunc is a bit of a special
> > case. We should also do the same thing for BPF helpers, now that I look at it,
> > but there's only one which takes a PTR_TO_BTF_ID right now (bpf_sk_release), and
> > it isn't problematic currently, but now that referenced PTR_TO_BTF_ID is used it
> > is quite possible to support it in more BPF helpers later and forget to prevent
> > such case.
> >
> > So, it would be possible to move this check inside check_func_arg_reg_off, based
> > on a new bool is_release_func parameter, and relying on the assumption that only
> > one referenced register can be passed to helper or kfunc at a time (already
> > enforced for both BPF helpers and kfuncs).
> >
> > Basically, instead of doing type == PTR_TO_BTF_ID for fixed_off_ok inside it, we
> > will do:
> >
> > 	fixed_off_ok = false;
> > 	if (type == PTR_TO_BTF_ID && (!is_release_func || !reg->ref_obj_id))
> > 		fixed_off_ok = true;
> For the preemptive fix on release func and non zero reg->off,
> should it be a release-without-acquire error instead of a ptr-type/reg->off error?
> The fix should be either clearing the reg->ref_obj_id earlier or at least treat
> ref_obj_id as zero here and then fallthrough the existing release-without-acquire
> error.  It is more to do with the ref_obj_id becomes invalid after reg->off
> becoming non-zero instead of reg->off is not allowed for a specific ptr
> type.  It is better to separate this preemptive fix to another set.
>

So IIUC what you're saying is that once someone performs increment, we reset the
ref_obj_id to 0, then the reference state is still present so
check_reference_leak would complain, but releasing such modified register won't
work since ref_obj_id is 0 (so no ref state for that ref_obj_id).

But I think clang (or even user writing BPF ASM) would be well within its rights
to temporarily add an offset to the register, pass member pointer to some other
helper, or read some data, and then decrement it again to shift the pointer
backwards setting reg->off to 0. Then they should be able to again pass such
register to release helper or kfunc. I think it would be unlikely (you can save
original pointer to caller saved reg, or spill to stack, or use offset in LDX,
etc.) but certainly not impossible.

I think the key point is that we want to make user pass the register as it was
when it was acquired, they can do any changes to off between acquire and
release, just that it should be set back to 0 when release function is called.

> >
> > Again, given we can only pass one referenced reg, if we see release func and a
> > reg with ref_obj_id, it is the one being released.
> >
> > In the end, it's more of a preference thing, if you feel strongly about it I can
> > go with the __check_ptr_off_reg call too.
> Yeah, it is a preference thing and not feeling strongly.
> Without the need for the release-func/reg->off preemptive fix, adding
> one __check_ptr_off_reg() seems to be a cleaner fix to me but
> I won't insist.

--
Kartikeya

Re: [PATCH bpf-next v1 4/6] bpf: Harden register offset checks for release kfunc

[Alexei Starovoitov]

On Thu, Mar 03, 2022 at 04:00:20AM +0530, Kumar Kartikeya Dwivedi wrote:
> On Thu, Mar 03, 2022 at 03:26:40AM IST, Martin KaFai Lau wrote:
> > On Wed, Mar 02, 2022 at 03:12:18PM +0530, Kumar Kartikeya Dwivedi wrote:
> > > On Wed, Mar 02, 2022 at 08:50:24AM IST, Martin KaFai Lau wrote:
> > > > On Tue, Mar 01, 2022 at 12:27:43PM +0530, Kumar Kartikeya Dwivedi wrote:
> > > > > Let's ensure that the PTR_TO_BTF_ID reg being passed in to release kfunc
> > > > > 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.
> > > > >
> > > > > Previous commits already protected against non-zero var_off. The case we
> > > > > are concerned about now is when we have a type that can be returned by
> > > > > 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>
> > > > > ---
> > > > >  kernel/bpf/btf.c | 15 +++++++++++++--
> > > > >  1 file changed, 13 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > > > > index 7f6a0ae5028b..ba6845225b65 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.
> > > > >  	 */
> > > > > @@ -5816,6 +5819,16 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
> > > > >  							regno, reg->ref_obj_id, ref_obj_id);
> > > > >  						return -EFAULT;
> > > > >  					}
> > > > > +					/* Ensure that offset of referenced PTR_TO_BTF_ID is
> > > > > +					 * always zero, when passed to release function.
> > > > > +					 * var_off has already been checked to be 0 by
> > > > > +					 * check_func_arg_reg_off.
> > > > > +					 */
> > > > > +					if (rel && reg->off) {
> > > > Here is another reg->off check for PTR_TO_BTF_ID on top of the
> > > > one 'check_func_arg_reg_off' added to the same function in patch 2.
> > > > A nit, I also found passing ARG_DONTCARE in patch 2 a bit convoluted
> > > > considering the btf func does not need ARG_* to begin with.
> > > >
> > >
> > > Right, arg_type doesn't really matter here (unless we start indicating in BTF we
> > > want to take ringbuf allocation directly without size parameter or getting size
> > > from BTF type).
> > >
> > > > How about directly use the __check_ptr_off_reg() here instead of
> > > > check_func_arg_reg_off()?  Then patch 1 is not needed.
> > > >
> > > > Would something like this do the same thing (uncompiled code) ?
> > > >
> > >
> > > I should have included a link to the previous discussion, sorry about that:
> > > https://lore.kernel.org/bpf/20220223031600.pvbhu3dbwxke4eia@apollo.legion
> > Ah. Thanks for the link.  I didn't go back to the list since the set is
> > tagged v1 ;)
> >
> 
> Right, I split the first patch out and then added this patch, so it felt more
> appropriate to tag it as v1. But I will include this link in the cover letter
> going forward.
> 
> > > Yes, this should also do the same thing, but the idea was to avoid keeping the
> > > same checks in multiple places. For now, there is only the special case of
> > > ARG_TYPE_PTR_TO_ALLOC_MEM and PTR_TO_BTF_ID that require some special handling,
> > > the former of which is currently not relevant for kfunc, but adding some future
> > > type and ensuring kfunc, and helper do the offset checks correctly just means
> > > updating check_func_arg_reg_off.
> > >
> > > reg->off in case of PTR_TO_BTF_ID reg for release kfunc is a bit of a special
> > > case. We should also do the same thing for BPF helpers, now that I look at it,
> > > but there's only one which takes a PTR_TO_BTF_ID right now (bpf_sk_release), and
> > > it isn't problematic currently, but now that referenced PTR_TO_BTF_ID is used it
> > > is quite possible to support it in more BPF helpers later and forget to prevent
> > > such case.
> > >
> > > So, it would be possible to move this check inside check_func_arg_reg_off, based
> > > on a new bool is_release_func parameter, and relying on the assumption that only
> > > one referenced register can be passed to helper or kfunc at a time (already
> > > enforced for both BPF helpers and kfuncs).
> > >
> > > Basically, instead of doing type == PTR_TO_BTF_ID for fixed_off_ok inside it, we
> > > will do:
> > >
> > > 	fixed_off_ok = false;
> > > 	if (type == PTR_TO_BTF_ID && (!is_release_func || !reg->ref_obj_id))
> > > 		fixed_off_ok = true;
> > For the preemptive fix on release func and non zero reg->off,
> > should it be a release-without-acquire error instead of a ptr-type/reg->off error?
> > The fix should be either clearing the reg->ref_obj_id earlier or at least treat
> > ref_obj_id as zero here and then fallthrough the existing release-without-acquire
> > error.  It is more to do with the ref_obj_id becomes invalid after reg->off
> > becoming non-zero instead of reg->off is not allowed for a specific ptr
> > type.  It is better to separate this preemptive fix to another set.
> >
> 
> So IIUC what you're saying is that once someone performs increment, we reset the
> ref_obj_id to 0, then the reference state is still present so
> check_reference_leak would complain, but releasing such modified register won't
> work since ref_obj_id is 0 (so no ref state for that ref_obj_id).
> 
> But I think clang (or even user writing BPF ASM) would be well within its rights
> to temporarily add an offset to the register, pass member pointer to some other
> helper, or read some data, and then decrement it again to shift the pointer
> backwards setting reg->off to 0. Then they should be able to again pass such
> register to release helper or kfunc. I think it would be unlikely (you can save
> original pointer to caller saved reg, or spill to stack, or use offset in LDX,
> etc.) but certainly not impossible.

I don't think llvm will ever do such thing. Passing into a helper means
that the register is scratched. It won't be reused after the call.
Saving modified into a stack to restore later just to do a math on it
goes against "optimization" goal of the compiler.

> I think the key point is that we want to make user pass the register as it was
> when it was acquired, they can do any changes to off between acquire and
> release, just that it should be set back to 0 when release function is called.

Correct and this patch is covering that.
I'm not sure what is the contention point here.
Sorry I'm behind the mailing list.

> > >
> > > Again, given we can only pass one referenced reg, if we see release func and a
> > > reg with ref_obj_id, it is the one being released.
> > >
> > > In the end, it's more of a preference thing, if you feel strongly about it I can
> > > go with the __check_ptr_off_reg call too.
> > Yeah, it is a preference thing and not feeling strongly.
> > Without the need for the release-func/reg->off preemptive fix, adding
> > one __check_ptr_off_reg() seems to be a cleaner fix to me but
> > I won't insist.

fwiw I like patches 1-3.
I think extra check here for release func is justified on its own.
Converting it into:
  fixed_off_ok = false;
  if (type == PTR_TO_BTF_ID && (!is_release_func || !reg->ref_obj_id))
          fixed_off_ok = true;
obfuscates the check to me.
if (rel && reg->off) check
is pretty obvious.

Re: [PATCH bpf-next v1 5/6] selftests/bpf: Update tests for new errstr

[Alexei Starovoitov]

On Mon, Feb 28, 2022 at 10:58 PM Kumar Kartikeya Dwivedi
<memxor@gmail.com> wrote:
>
> Verifier for negative offset case returns a different, more clear error
> message. Update existing verifier selftests to work with that.
>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> ---
>  tools/testing/selftests/bpf/verifier/bounds_deduction.c | 2 +-
>  tools/testing/selftests/bpf/verifier/ctx.c              | 8 ++++----
>  2 files changed, 5 insertions(+), 5 deletions(-)
>
> 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",

Should this be a part of patch 3 to avoid breaking bisect?

Re: [PATCH bpf-next v1 6/6] selftests/bpf: Add tests for kfunc register offset checks

[Alexei Starovoitov]

On Tue, Mar 1, 2022 at 3:57 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
>
> On Tue, Mar 01, 2022 at 05:10:31PM IST, kernel test robot wrote:
> > Hi Kumar,
> >
> > Thank you for the patch! Perhaps something to improve:
> >
> > [auto build test WARNING on bpf-next/master]
> >
> > url:    https://github.com/0day-ci/linux/commits/Kumar-Kartikeya-Dwivedi/Fixes-for-bad-PTR_TO_BTF_ID-offset/20220301-150010
> > base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
> > config: s390-randconfig-r021-20220301 (https://download.01.org/0day-ci/archive/20220301/202203011937.wMLpkfU3-lkp@intel.com/config)
> > compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project d271fc04d5b97b12e6b797c6067d3c96a8d7470e)
>
> The same warning is emitted on clang for all existing definitions, so I can
> respin with a fix for the warning like we do for GCC, otherwise it can also
> be a follow up patch.

Separate patch is fine.
How do you plan on fixing it?
What is __diag_ignore equivalent for clang?

Re: [PATCH bpf-next v1 6/6] selftests/bpf: Add tests for kfunc register offset checks

[Alexei Starovoitov]

[-- Attachment #1: Type: text/plain, Size: 1049 bytes --]

On Tue, Mar 1, 2022 at 3:57 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
>
> On Tue, Mar 01, 2022 at 05:10:31PM IST, kernel test robot wrote:
> > Hi Kumar,
> >
> > Thank you for the patch! Perhaps something to improve:
> >
> > [auto build test WARNING on bpf-next/master]
> >
> > url:    https://github.com/0day-ci/linux/commits/Kumar-Kartikeya-Dwivedi/Fixes-for-bad-PTR_TO_BTF_ID-offset/20220301-150010
> > base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
> > config: s390-randconfig-r021-20220301 (https://download.01.org/0day-ci/archive/20220301/202203011937.wMLpkfU3-lkp(a)intel.com/config)
> > compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project d271fc04d5b97b12e6b797c6067d3c96a8d7470e)
>
> The same warning is emitted on clang for all existing definitions, so I can
> respin with a fix for the warning like we do for GCC, otherwise it can also
> be a follow up patch.

Separate patch is fine.
How do you plan on fixing it?
What is __diag_ignore equivalent for clang?

Re: [PATCH bpf-next v1 4/6] bpf: Harden register offset checks for release kfunc

[Kumar Kartikeya Dwivedi]

On Thu, Mar 03, 2022 at 04:14:18AM IST, Alexei Starovoitov wrote:
> On Thu, Mar 03, 2022 at 04:00:20AM +0530, Kumar Kartikeya Dwivedi wrote:
> > On Thu, Mar 03, 2022 at 03:26:40AM IST, Martin KaFai Lau wrote:
> > > On Wed, Mar 02, 2022 at 03:12:18PM +0530, Kumar Kartikeya Dwivedi wrote:
> > > > On Wed, Mar 02, 2022 at 08:50:24AM IST, Martin KaFai Lau wrote:
> > > > > On Tue, Mar 01, 2022 at 12:27:43PM +0530, Kumar Kartikeya Dwivedi wrote:
> > > > > > Let's ensure that the PTR_TO_BTF_ID reg being passed in to release kfunc
> > > > > > 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.
> > > > > >
> > > > > > Previous commits already protected against non-zero var_off. The case we
> > > > > > are concerned about now is when we have a type that can be returned by
> > > > > > 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>
> > > > > > ---
> > > > > >  kernel/bpf/btf.c | 15 +++++++++++++--
> > > > > >  1 file changed, 13 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > > > > > index 7f6a0ae5028b..ba6845225b65 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.
> > > > > >  	 */
> > > > > > @@ -5816,6 +5819,16 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
> > > > > >  							regno, reg->ref_obj_id, ref_obj_id);
> > > > > >  						return -EFAULT;
> > > > > >  					}
> > > > > > +					/* Ensure that offset of referenced PTR_TO_BTF_ID is
> > > > > > +					 * always zero, when passed to release function.
> > > > > > +					 * var_off has already been checked to be 0 by
> > > > > > +					 * check_func_arg_reg_off.
> > > > > > +					 */
> > > > > > +					if (rel && reg->off) {
> > > > > Here is another reg->off check for PTR_TO_BTF_ID on top of the
> > > > > one 'check_func_arg_reg_off' added to the same function in patch 2.
> > > > > A nit, I also found passing ARG_DONTCARE in patch 2 a bit convoluted
> > > > > considering the btf func does not need ARG_* to begin with.
> > > > >
> > > >
> > > > Right, arg_type doesn't really matter here (unless we start indicating in BTF we
> > > > want to take ringbuf allocation directly without size parameter or getting size
> > > > from BTF type).
> > > >
> > > > > How about directly use the __check_ptr_off_reg() here instead of
> > > > > check_func_arg_reg_off()?  Then patch 1 is not needed.
> > > > >
> > > > > Would something like this do the same thing (uncompiled code) ?
> > > > >
> > > >
> > > > I should have included a link to the previous discussion, sorry about that:
> > > > https://lore.kernel.org/bpf/20220223031600.pvbhu3dbwxke4eia@apollo.legion
> > > Ah. Thanks for the link.  I didn't go back to the list since the set is
> > > tagged v1 ;)
> > >
> >
> > Right, I split the first patch out and then added this patch, so it felt more
> > appropriate to tag it as v1. But I will include this link in the cover letter
> > going forward.
> >
> > > > Yes, this should also do the same thing, but the idea was to avoid keeping the
> > > > same checks in multiple places. For now, there is only the special case of
> > > > ARG_TYPE_PTR_TO_ALLOC_MEM and PTR_TO_BTF_ID that require some special handling,
> > > > the former of which is currently not relevant for kfunc, but adding some future
> > > > type and ensuring kfunc, and helper do the offset checks correctly just means
> > > > updating check_func_arg_reg_off.
> > > >
> > > > reg->off in case of PTR_TO_BTF_ID reg for release kfunc is a bit of a special
> > > > case. We should also do the same thing for BPF helpers, now that I look at it,
> > > > but there's only one which takes a PTR_TO_BTF_ID right now (bpf_sk_release), and
> > > > it isn't problematic currently, but now that referenced PTR_TO_BTF_ID is used it
> > > > is quite possible to support it in more BPF helpers later and forget to prevent
> > > > such case.
> > > >
> > > > So, it would be possible to move this check inside check_func_arg_reg_off, based
> > > > on a new bool is_release_func parameter, and relying on the assumption that only
> > > > one referenced register can be passed to helper or kfunc at a time (already
> > > > enforced for both BPF helpers and kfuncs).
> > > >
> > > > Basically, instead of doing type == PTR_TO_BTF_ID for fixed_off_ok inside it, we
> > > > will do:
> > > >
> > > > 	fixed_off_ok = false;
> > > > 	if (type == PTR_TO_BTF_ID && (!is_release_func || !reg->ref_obj_id))
> > > > 		fixed_off_ok = true;
> > > For the preemptive fix on release func and non zero reg->off,
> > > should it be a release-without-acquire error instead of a ptr-type/reg->off error?
> > > The fix should be either clearing the reg->ref_obj_id earlier or at least treat
> > > ref_obj_id as zero here and then fallthrough the existing release-without-acquire
> > > error.  It is more to do with the ref_obj_id becomes invalid after reg->off
> > > becoming non-zero instead of reg->off is not allowed for a specific ptr
> > > type.  It is better to separate this preemptive fix to another set.
> > >
> >
> > So IIUC what you're saying is that once someone performs increment, we reset the
> > ref_obj_id to 0, then the reference state is still present so
> > check_reference_leak would complain, but releasing such modified register won't
> > work since ref_obj_id is 0 (so no ref state for that ref_obj_id).
> >
> > But I think clang (or even user writing BPF ASM) would be well within its rights
> > to temporarily add an offset to the register, pass member pointer to some other
> > helper, or read some data, and then decrement it again to shift the pointer
> > backwards setting reg->off to 0. Then they should be able to again pass such
> > register to release helper or kfunc. I think it would be unlikely (you can save
> > original pointer to caller saved reg, or spill to stack, or use offset in LDX,
> > etc.) but certainly not impossible.
>
> I don't think llvm will ever do such thing. Passing into a helper means
> that the register is scratched. It won't be reused after the call.
> Saving modified into a stack to restore later just to do a math on it
> goes against "optimization" goal of the compiler.
>
> > I think the key point is that we want to make user pass the register as it was
> > when it was acquired, they can do any changes to off between acquire and
> > release, just that it should be set back to 0 when release function is called.
>
> Correct and this patch is covering that.
> I'm not sure what is the contention point here.
> Sorry I'm behind the mailing list.
>
> > > >
> > > > Again, given we can only pass one referenced reg, if we see release func and a
> > > > reg with ref_obj_id, it is the one being released.
> > > >
> > > > In the end, it's more of a preference thing, if you feel strongly about it I can
> > > > go with the __check_ptr_off_reg call too.
> > > Yeah, it is a preference thing and not feeling strongly.
> > > Without the need for the release-func/reg->off preemptive fix, adding
> > > one __check_ptr_off_reg() seems to be a cleaner fix to me but
> > > I won't insist.
>
> fwiw I like patches 1-3.
> I think extra check here for release func is justified on its own.
> Converting it into:
>   fixed_off_ok = false;
>   if (type == PTR_TO_BTF_ID && (!is_release_func || !reg->ref_obj_id))
>           fixed_off_ok = true;
> obfuscates the check to me.

I was talking of putting this inside check_func_arg_reg_off. I think we should
do the same check for BPF helpers as well (rn only one supports releasing
PTR_TO_BTF_ID, soon we may have others). Just passing a bool to
check_func_arg_reg_off to indicate we are checking for release func (helper or
kfunc have same rules here) would allow putting this check inside it.

> if (rel && reg->off) check
> is pretty obvious.

--
Kartikeya

Re: [PATCH bpf-next v1 5/6] selftests/bpf: Update tests for new errstr

[Kumar Kartikeya Dwivedi]

On Thu, Mar 03, 2022 at 04:15:06AM IST, Alexei Starovoitov wrote:
> On Mon, Feb 28, 2022 at 10:58 PM Kumar Kartikeya Dwivedi
> <memxor@gmail.com> wrote:
> >
> > Verifier for negative offset case returns a different, more clear error
> > message. Update existing verifier selftests to work with that.
> >
> > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> > ---
> >  tools/testing/selftests/bpf/verifier/bounds_deduction.c | 2 +-
> >  tools/testing/selftests/bpf/verifier/ctx.c              | 8 ++++----
> >  2 files changed, 5 insertions(+), 5 deletions(-)
> >
> > 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",
>
> Should this be a part of patch 3 to avoid breaking bisect?

Right, will squash into patch 3 in v2.

--
Kartikeya

Re: [PATCH bpf-next v1 6/6] selftests/bpf: Add tests for kfunc register offset checks

[Kumar Kartikeya Dwivedi]

On Thu, Mar 03, 2022 at 04:17:25AM IST, Alexei Starovoitov wrote:
> On Tue, Mar 1, 2022 at 3:57 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
> >
> > On Tue, Mar 01, 2022 at 05:10:31PM IST, kernel test robot wrote:
> > > Hi Kumar,
> > >
> > > Thank you for the patch! Perhaps something to improve:
> > >
> > > [auto build test WARNING on bpf-next/master]
> > >
> > > url:    https://github.com/0day-ci/linux/commits/Kumar-Kartikeya-Dwivedi/Fixes-for-bad-PTR_TO_BTF_ID-offset/20220301-150010
> > > base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
> > > config: s390-randconfig-r021-20220301 (https://download.01.org/0day-ci/archive/20220301/202203011937.wMLpkfU3-lkp@intel.com/config)
> > > compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project d271fc04d5b97b12e6b797c6067d3c96a8d7470e)
> >
> > The same warning is emitted on clang for all existing definitions, so I can
> > respin with a fix for the warning like we do for GCC, otherwise it can also
> > be a follow up patch.
>
> Separate patch is fine.
> How do you plan on fixing it?
> What is __diag_ignore equivalent for clang?

Hmm, looks like I'll have to add those in include/linux/compiler-clang.h. Quick
local testing suggests it will work with _Pragma("clang diagnostic ignored ...").

--
Kartikeya

Re: [PATCH bpf-next v1 6/6] selftests/bpf: Add tests for kfunc register offset checks

[Kumar Kartikeya Dwivedi]

[-- Attachment #1: Type: text/plain, Size: 1340 bytes --]

On Thu, Mar 03, 2022 at 04:17:25AM IST, Alexei Starovoitov wrote:
> On Tue, Mar 1, 2022 at 3:57 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
> >
> > On Tue, Mar 01, 2022 at 05:10:31PM IST, kernel test robot wrote:
> > > Hi Kumar,
> > >
> > > Thank you for the patch! Perhaps something to improve:
> > >
> > > [auto build test WARNING on bpf-next/master]
> > >
> > > url:    https://github.com/0day-ci/linux/commits/Kumar-Kartikeya-Dwivedi/Fixes-for-bad-PTR_TO_BTF_ID-offset/20220301-150010
> > > base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
> > > config: s390-randconfig-r021-20220301 (https://download.01.org/0day-ci/archive/20220301/202203011937.wMLpkfU3-lkp(a)intel.com/config)
> > > compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project d271fc04d5b97b12e6b797c6067d3c96a8d7470e)
> >
> > The same warning is emitted on clang for all existing definitions, so I can
> > respin with a fix for the warning like we do for GCC, otherwise it can also
> > be a follow up patch.
>
> Separate patch is fine.
> How do you plan on fixing it?
> What is __diag_ignore equivalent for clang?

Hmm, looks like I'll have to add those in include/linux/compiler-clang.h. Quick
local testing suggests it will work with _Pragma("clang diagnostic ignored ...").

--
Kartikeya

Re: [PATCH bpf-next v1 4/6] bpf: Harden register offset checks for release kfunc

[Alexei Starovoitov]

On Wed, Mar 2, 2022 at 3:00 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
> >
> > fwiw I like patches 1-3.
> > I think extra check here for release func is justified on its own.
> > Converting it into:
> >   fixed_off_ok = false;
> >   if (type == PTR_TO_BTF_ID && (!is_release_func || !reg->ref_obj_id))
> >           fixed_off_ok = true;
> > obfuscates the check to me.
>
> I was talking of putting this inside check_func_arg_reg_off. I think we should
> do the same check for BPF helpers as well (rn only one supports releasing
> PTR_TO_BTF_ID, soon we may have others). Just passing a bool to
> check_func_arg_reg_off to indicate we are checking for release func (helper or
> kfunc have same rules here) would allow putting this check inside it.

Hmm. check_func_arg() is called before we call
is_release_function(func_id).
Are you proposing to call it before and pass
another boolean into check_func_arg() or store the flag in meta?
Sounds ugly.
imo reg->off is a simple enough check to keep it open coded
where necessary.

Re: [PATCH bpf-next v1 6/6] selftests/bpf: Add tests for kfunc register offset checks

[Alexei Starovoitov]

On Wed, Mar 2, 2022 at 3:14 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
>
> On Thu, Mar 03, 2022 at 04:17:25AM IST, Alexei Starovoitov wrote:
> > On Tue, Mar 1, 2022 at 3:57 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
> > >
> > > On Tue, Mar 01, 2022 at 05:10:31PM IST, kernel test robot wrote:
> > > > Hi Kumar,
> > > >
> > > > Thank you for the patch! Perhaps something to improve:
> > > >
> > > > [auto build test WARNING on bpf-next/master]
> > > >
> > > > url:    https://github.com/0day-ci/linux/commits/Kumar-Kartikeya-Dwivedi/Fixes-for-bad-PTR_TO_BTF_ID-offset/20220301-150010
> > > > base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
> > > > config: s390-randconfig-r021-20220301 (https://download.01.org/0day-ci/archive/20220301/202203011937.wMLpkfU3-lkp@intel.com/config)
> > > > compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project d271fc04d5b97b12e6b797c6067d3c96a8d7470e)
> > >
> > > The same warning is emitted on clang for all existing definitions, so I can
> > > respin with a fix for the warning like we do for GCC, otherwise it can also
> > > be a follow up patch.
> >
> > Separate patch is fine.
> > How do you plan on fixing it?
> > What is __diag_ignore equivalent for clang?
>
> Hmm, looks like I'll have to add those in include/linux/compiler-clang.h. Quick
> local testing suggests it will work with _Pragma("clang diagnostic ignored ...").

Make a generic llvm/gcc #define for
__diag_ignore(GCC, 8, "-Wmissing-prototypes" ?
We need it in two places so far.

Re: [PATCH bpf-next v1 6/6] selftests/bpf: Add tests for kfunc register offset checks

[Alexei Starovoitov]

[-- Attachment #1: Type: text/plain, Size: 1578 bytes --]

On Wed, Mar 2, 2022 at 3:14 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
>
> On Thu, Mar 03, 2022 at 04:17:25AM IST, Alexei Starovoitov wrote:
> > On Tue, Mar 1, 2022 at 3:57 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
> > >
> > > On Tue, Mar 01, 2022 at 05:10:31PM IST, kernel test robot wrote:
> > > > Hi Kumar,
> > > >
> > > > Thank you for the patch! Perhaps something to improve:
> > > >
> > > > [auto build test WARNING on bpf-next/master]
> > > >
> > > > url:    https://github.com/0day-ci/linux/commits/Kumar-Kartikeya-Dwivedi/Fixes-for-bad-PTR_TO_BTF_ID-offset/20220301-150010
> > > > base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
> > > > config: s390-randconfig-r021-20220301 (https://download.01.org/0day-ci/archive/20220301/202203011937.wMLpkfU3-lkp(a)intel.com/config)
> > > > compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project d271fc04d5b97b12e6b797c6067d3c96a8d7470e)
> > >
> > > The same warning is emitted on clang for all existing definitions, so I can
> > > respin with a fix for the warning like we do for GCC, otherwise it can also
> > > be a follow up patch.
> >
> > Separate patch is fine.
> > How do you plan on fixing it?
> > What is __diag_ignore equivalent for clang?
>
> Hmm, looks like I'll have to add those in include/linux/compiler-clang.h. Quick
> local testing suggests it will work with _Pragma("clang diagnostic ignored ...").

Make a generic llvm/gcc #define for
__diag_ignore(GCC, 8, "-Wmissing-prototypes" ?
We need it in two places so far.

Re: [PATCH bpf-next v1 6/6] selftests/bpf: Add tests for kfunc register offset checks

[Nathan Chancellor]

On Thu, Mar 03, 2022 at 04:44:01AM +0530, Kumar Kartikeya Dwivedi wrote:
> On Thu, Mar 03, 2022 at 04:17:25AM IST, Alexei Starovoitov wrote:
> > On Tue, Mar 1, 2022 at 3:57 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
> > >
> > > On Tue, Mar 01, 2022 at 05:10:31PM IST, kernel test robot wrote:
> > > > Hi Kumar,
> > > >
> > > > Thank you for the patch! Perhaps something to improve:
> > > >
> > > > [auto build test WARNING on bpf-next/master]
> > > >
> > > > url:    https://github.com/0day-ci/linux/commits/Kumar-Kartikeya-Dwivedi/Fixes-for-bad-PTR_TO_BTF_ID-offset/20220301-150010
> > > > base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
> > > > config: s390-randconfig-r021-20220301 (https://download.01.org/0day-ci/archive/20220301/202203011937.wMLpkfU3-lkp@intel.com/config)
> > > > compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project d271fc04d5b97b12e6b797c6067d3c96a8d7470e)
> > >
> > > The same warning is emitted on clang for all existing definitions, so I can
> > > respin with a fix for the warning like we do for GCC, otherwise it can also
> > > be a follow up patch.
> >
> > Separate patch is fine.
> > How do you plan on fixing it?
> > What is __diag_ignore equivalent for clang?
> 
> Hmm, looks like I'll have to add those in include/linux/compiler-clang.h. Quick
> local testing suggests it will work with _Pragma("clang diagnostic ignored ...").

I have a diff that mirrors the GCC infrastructure, which should work for
this, feel free to copy it:

https://lore.kernel.org/r/20210310225240.4epj2mdmzt4vurr3@archlinux-ax161/

If you want to shut up the warning for all supported versions of clang,
switch 130000 for 110000 and __diag_clang_11() for __diag_clang_13().

Cheers,
Nathan

Re: [PATCH bpf-next v1 6/6] selftests/bpf: Add tests for kfunc register offset checks

[Nathan Chancellor]

[-- Attachment #1: Type: text/plain, Size: 1797 bytes --]

On Thu, Mar 03, 2022 at 04:44:01AM +0530, Kumar Kartikeya Dwivedi wrote:
> On Thu, Mar 03, 2022 at 04:17:25AM IST, Alexei Starovoitov wrote:
> > On Tue, Mar 1, 2022 at 3:57 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
> > >
> > > On Tue, Mar 01, 2022 at 05:10:31PM IST, kernel test robot wrote:
> > > > Hi Kumar,
> > > >
> > > > Thank you for the patch! Perhaps something to improve:
> > > >
> > > > [auto build test WARNING on bpf-next/master]
> > > >
> > > > url:    https://github.com/0day-ci/linux/commits/Kumar-Kartikeya-Dwivedi/Fixes-for-bad-PTR_TO_BTF_ID-offset/20220301-150010
> > > > base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
> > > > config: s390-randconfig-r021-20220301 (https://download.01.org/0day-ci/archive/20220301/202203011937.wMLpkfU3-lkp(a)intel.com/config)
> > > > compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project d271fc04d5b97b12e6b797c6067d3c96a8d7470e)
> > >
> > > The same warning is emitted on clang for all existing definitions, so I can
> > > respin with a fix for the warning like we do for GCC, otherwise it can also
> > > be a follow up patch.
> >
> > Separate patch is fine.
> > How do you plan on fixing it?
> > What is __diag_ignore equivalent for clang?
> 
> Hmm, looks like I'll have to add those in include/linux/compiler-clang.h. Quick
> local testing suggests it will work with _Pragma("clang diagnostic ignored ...").

I have a diff that mirrors the GCC infrastructure, which should work for
this, feel free to copy it:

https://lore.kernel.org/r/20210310225240.4epj2mdmzt4vurr3(a)archlinux-ax161/

If you want to shut up the warning for all supported versions of clang,
switch 130000 for 110000 and __diag_clang_11() for __diag_clang_13().

Cheers,
Nathan

Re: [PATCH bpf-next v1 4/6] bpf: Harden register offset checks for release kfunc

[Kumar Kartikeya Dwivedi]

On Thu, Mar 03, 2022 at 04:47:59AM IST, Alexei Starovoitov wrote:
> On Wed, Mar 2, 2022 at 3:00 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
> > >
> > > fwiw I like patches 1-3.
> > > I think extra check here for release func is justified on its own.
> > > Converting it into:
> > >   fixed_off_ok = false;
> > >   if (type == PTR_TO_BTF_ID && (!is_release_func || !reg->ref_obj_id))
> > >           fixed_off_ok = true;
> > > obfuscates the check to me.
> >
> > I was talking of putting this inside check_func_arg_reg_off. I think we should
> > do the same check for BPF helpers as well (rn only one supports releasing
> > PTR_TO_BTF_ID, soon we may have others). Just passing a bool to
> > check_func_arg_reg_off to indicate we are checking for release func (helper or
> > kfunc have same rules here) would allow putting this check inside it.
>
> Hmm. check_func_arg() is called before we call
> is_release_function(func_id).
> Are you proposing to call it before and pass
> another boolean into check_func_arg() or store the flag in meta?

We save meta.func_id before calling check_func_arg. Inside it we can do:
err = check_func_arg_reg_off(env, reg, regno, arg_type, is_release_function(meta->func_id));

I actually tried open coding it for BPF helpers, and it was more complicated. If
we delay this check until is_release_function call after check_func_arg, we need
to remember if reg for whom meta->ref_obj_id had off > 0 and type PTR_TO_BTF_ID.
If we put it inside check_reg_type or check_func_arg, you need to call
is_release_function anyway there.

Compared to these two options, doing it in check_func_arg_reg_off looks better
to me, but ymmv.

> Sounds ugly.
> imo reg->off is a simple enough check to keep it open coded
> where necessary.

--
Kartikeya

Re: [PATCH bpf-next v1 4/6] bpf: Harden register offset checks for release kfunc

[Martin KaFai Lau]

On Wed, Mar 02, 2022 at 02:44:18PM -0800, Alexei Starovoitov wrote:
> > So IIUC what you're saying is that once someone performs increment, we reset the
> > ref_obj_id to 0, then the reference state is still present so
> > check_reference_leak would complain, but releasing such modified register won't
> > work since ref_obj_id is 0 (so no ref state for that ref_obj_id).
I meant error similar to release_reference() should be used and it
should be treated as release-without-prior-acquire error.  The reg
is pointing at something that its reference has not been acquired
before.

You are right, reset ref_obj_id to 0 during increment won't work
as you have explained in the following.

> > 
> > But I think clang (or even user writing BPF ASM) would be well within its rights
> > to temporarily add an offset to the register, pass member pointer to some other
> > helper, or read some data, and then decrement it again to shift the pointer
> > backwards setting reg->off to 0. Then they should be able to again pass such
> > register to release helper or kfunc. I think it would be unlikely (you can save
> > original pointer to caller saved reg, or spill to stack, or use offset in LDX,
> > etc.) but certainly not impossible.
> 
> I don't think llvm will ever do such thing. Passing into a helper means
> that the register is scratched. It won't be reused after the call.
> Saving modified into a stack to restore later just to do a math on it
> goes against "optimization" goal of the compiler.
> 
> > I think the key point is that we want to make user pass the register as it was
> > when it was acquired, they can do any changes to off between acquire and
> > release, just that it should be set back to 0 when release function is called.
> 
> Correct and this patch is covering that.
> I'm not sure what is the contention point here.
> Sorry I'm behind the mailing list.
> 
> > > >
> > > > Again, given we can only pass one referenced reg, if we see release func and a
> > > > reg with ref_obj_id, it is the one being released.
> > > >
> > > > In the end, it's more of a preference thing, if you feel strongly about it I can
> > > > go with the __check_ptr_off_reg call too.
> > > Yeah, it is a preference thing and not feeling strongly.
> > > Without the need for the release-func/reg->off preemptive fix, adding
> > > one __check_ptr_off_reg() seems to be a cleaner fix to me but
> > > I won't insist.
> 
> fwiw I like patches 1-3.
> I think extra check here for release func is justified on its own.
> Converting it into:
>   fixed_off_ok = false;
>   if (type == PTR_TO_BTF_ID && (!is_release_func || !reg->ref_obj_id))
>           fixed_off_ok = true;
> obfuscates the check to me.
> if (rel && reg->off) check
> is pretty obvious.
Yeah, I am fine with an extra check and the "must have zero offset
message when passed to release kfunc".  The error is obvious enough
on what may be wrong in the bpf prog.

Re: [PATCH bpf-next v1 6/6] selftests/bpf: Add tests for kfunc register offset checks

[Kumar Kartikeya Dwivedi]

On Thu, Mar 03, 2022 at 04:56:37AM IST, Nathan Chancellor wrote:
> On Thu, Mar 03, 2022 at 04:44:01AM +0530, Kumar Kartikeya Dwivedi wrote:
> > On Thu, Mar 03, 2022 at 04:17:25AM IST, Alexei Starovoitov wrote:
> > > On Tue, Mar 1, 2022 at 3:57 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
> > > >
> > > > On Tue, Mar 01, 2022 at 05:10:31PM IST, kernel test robot wrote:
> > > > > Hi Kumar,
> > > > >
> > > > > Thank you for the patch! Perhaps something to improve:
> > > > >
> > > > > [auto build test WARNING on bpf-next/master]
> > > > >
> > > > > url:    https://github.com/0day-ci/linux/commits/Kumar-Kartikeya-Dwivedi/Fixes-for-bad-PTR_TO_BTF_ID-offset/20220301-150010
> > > > > base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
> > > > > config: s390-randconfig-r021-20220301 (https://download.01.org/0day-ci/archive/20220301/202203011937.wMLpkfU3-lkp@intel.com/config)
> > > > > compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project d271fc04d5b97b12e6b797c6067d3c96a8d7470e)
> > > >
> > > > The same warning is emitted on clang for all existing definitions, so I can
> > > > respin with a fix for the warning like we do for GCC, otherwise it can also
> > > > be a follow up patch.
> > >
> > > Separate patch is fine.
> > > How do you plan on fixing it?
> > > What is __diag_ignore equivalent for clang?
> >
> > Hmm, looks like I'll have to add those in include/linux/compiler-clang.h. Quick
> > local testing suggests it will work with _Pragma("clang diagnostic ignored ...").
>
> I have a diff that mirrors the GCC infrastructure, which should work for
> this, feel free to copy it:
>
> https://lore.kernel.org/r/20210310225240.4epj2mdmzt4vurr3@archlinux-ax161/
>
> If you want to shut up the warning for all supported versions of clang,
> switch 130000 for 110000 and __diag_clang_11() for __diag_clang_13().
>

That's great, Nathan! I'll add your Signed-off-by to it.

> Cheers,
> Nathan

--
Kartikeya

Re: [PATCH bpf-next v1 6/6] selftests/bpf: Add tests for kfunc register offset checks

[Kumar Kartikeya Dwivedi]

[-- Attachment #1: Type: text/plain, Size: 2011 bytes --]

On Thu, Mar 03, 2022 at 04:56:37AM IST, Nathan Chancellor wrote:
> On Thu, Mar 03, 2022 at 04:44:01AM +0530, Kumar Kartikeya Dwivedi wrote:
> > On Thu, Mar 03, 2022 at 04:17:25AM IST, Alexei Starovoitov wrote:
> > > On Tue, Mar 1, 2022 at 3:57 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
> > > >
> > > > On Tue, Mar 01, 2022 at 05:10:31PM IST, kernel test robot wrote:
> > > > > Hi Kumar,
> > > > >
> > > > > Thank you for the patch! Perhaps something to improve:
> > > > >
> > > > > [auto build test WARNING on bpf-next/master]
> > > > >
> > > > > url:    https://github.com/0day-ci/linux/commits/Kumar-Kartikeya-Dwivedi/Fixes-for-bad-PTR_TO_BTF_ID-offset/20220301-150010
> > > > > base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
> > > > > config: s390-randconfig-r021-20220301 (https://download.01.org/0day-ci/archive/20220301/202203011937.wMLpkfU3-lkp(a)intel.com/config)
> > > > > compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project d271fc04d5b97b12e6b797c6067d3c96a8d7470e)
> > > >
> > > > The same warning is emitted on clang for all existing definitions, so I can
> > > > respin with a fix for the warning like we do for GCC, otherwise it can also
> > > > be a follow up patch.
> > >
> > > Separate patch is fine.
> > > How do you plan on fixing it?
> > > What is __diag_ignore equivalent for clang?
> >
> > Hmm, looks like I'll have to add those in include/linux/compiler-clang.h. Quick
> > local testing suggests it will work with _Pragma("clang diagnostic ignored ...").
>
> I have a diff that mirrors the GCC infrastructure, which should work for
> this, feel free to copy it:
>
> https://lore.kernel.org/r/20210310225240.4epj2mdmzt4vurr3(a)archlinux-ax161/
>
> If you want to shut up the warning for all supported versions of clang,
> switch 130000 for 110000 and __diag_clang_11() for __diag_clang_13().
>

That's great, Nathan! I'll add your Signed-off-by to it.

> Cheers,
> Nathan

--
Kartikeya

Re: [PATCH bpf-next v1 4/6] bpf: Harden register offset checks for release kfunc

[Alexei Starovoitov]

On Wed, Mar 2, 2022 at 3:29 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
>
> On Thu, Mar 03, 2022 at 04:47:59AM IST, Alexei Starovoitov wrote:
> > On Wed, Mar 2, 2022 at 3:00 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
> > > >
> > > > fwiw I like patches 1-3.
> > > > I think extra check here for release func is justified on its own.
> > > > Converting it into:
> > > >   fixed_off_ok = false;
> > > >   if (type == PTR_TO_BTF_ID && (!is_release_func || !reg->ref_obj_id))
> > > >           fixed_off_ok = true;
> > > > obfuscates the check to me.
> > >
> > > I was talking of putting this inside check_func_arg_reg_off. I think we should
> > > do the same check for BPF helpers as well (rn only one supports releasing
> > > PTR_TO_BTF_ID, soon we may have others). Just passing a bool to
> > > check_func_arg_reg_off to indicate we are checking for release func (helper or
> > > kfunc have same rules here) would allow putting this check inside it.
> >
> > Hmm. check_func_arg() is called before we call
> > is_release_function(func_id).
> > Are you proposing to call it before and pass
> > another boolean into check_func_arg() or store the flag in meta?
>
> We save meta.func_id before calling check_func_arg. Inside it we can do:
> err = check_func_arg_reg_off(env, reg, regno, arg_type, is_release_function(meta->func_id));
>
> I actually tried open coding it for BPF helpers, and it was more complicated. If
> we delay this check until is_release_function call after check_func_arg, we need
> to remember if reg for whom meta->ref_obj_id had off > 0 and type PTR_TO_BTF_ID.
> If we put it inside check_reg_type or check_func_arg, you need to call
> is_release_function anyway there.
>
> Compared to these two options, doing it in check_func_arg_reg_off looks better
> to me, but ymmv.

I see. Yeah. You're right.
check_func_arg_reg_off(env, reg, regno, arg_type,
  is_release_function(meta->func_id))
is indeed cleaner.