[PATCH bpf v1 0/5] More fixes for crashes due to bad PTR_TO_BTF_ID reg->off

[PATCH bpf v1 0/5] More fixes for crashes due to bad PTR_TO_BTF_ID reg->off

[Kumar Kartikeya Dwivedi]

A few more fixes for bad PTR_TO_BTF_ID reg->off being accepted in places, that
can lead to the kernel crashing. Noticed while making sure my own series for BTF
ID pointer in map won't allow stores for pointers with incorrect offsets.

I include one example where d_path can crash even if you NULL check
PTR_TO_BTF_ID, and one example of how missing NULL check in helper taking
PTR_TO_BTF_ID (like bpf_sock_from_file) is a real problem, see the selftest
patch.

The &f->f_path becomes NULL + offset in case f is NULL, circumventing NULL
checks in existing helpers. The only thing needed to trigger this finding an
object that embeds the object of interest, and then somehow obtaining a NULL
PTR_TO_BTF_ID to it (not hard, esp. due to exception handler for PROBE_MEM loads
writing 0 to destination register).

However, for the case of patch 2, it is allowed in my series since the next load
of the bad pointer stored using:
  struct file *f = ...; // some pointer walking returning NULL pointer
  map_val->ptr = &f->f_path; // ptr being struct path *
... would be marked as PTR_UNTRUSTED, so it won't be allowed to be passed into
the kernel, and hence can be permitted. In referenced case, the PTR_TO_BTF_ID
should not be NULL anyway. kptr_get style helper takes PTR_TO_MAP_VALUE in
referenced ptr case only, so the load either yields NULL or RCU protected
pointer.

Tests for patch 1 depend on fixup_kfunc_btf_id in test_verifier, hence will be
sent after merge window opens, some other changes after bpf tree merges into
bpf-next, but all pending ones can be seen here [0]. Tests for patch 2 are
included, and try to trigger crash without the fix, but it's not 100% reliable.
We may need special testing helpers or kfuncs to make it thorough, but wanted to
wait before getting feedback.

Issue fixed by patch 2 is a bit more broader in scope, and would require proper
discussion (before being applied) on the correct way forward, as it is
technically backwards incompatible change, but hopefully never breaks real
programs, only malicious or already incorrect ones.

Also, please suggest the right "Fixes" tag for patch 2.

As for patch 3 (selftest), please suggest a better way to get a certain type of
PTR_TO_BTF_ID which can be NULL or NULL+offset. Can we add kfuncs for testing
that return such pointers and make them available to e.g. TC progs, if the fix
in patch 2 is acceptable?

  [0]: https://github.com/kkdwivedi/linux/commits/fixes-bpf-next

Kumar Kartikeya Dwivedi (5):
  bpf: Fix kfunc register offset check for PTR_TO_BTF_ID
  bpf: Restrict PTR_TO_BTF_ID offset to PAGE_SIZE when calling helpers
  bpf: Use bpf_ptr_is_invalid for all helpers taking PTR_TO_BTF_ID
  selftests/bpf: Add selftest for PTR_TO_BTF_ID NULL + off case
  selftests/bpf: Adjust verifier selftest for updated message

 include/linux/bpf.h                           | 19 ++++
 include/linux/bpf_verifier.h                  |  3 +
 kernel/bpf/bpf_inode_storage.c                |  4 +-
 kernel/bpf/bpf_lsm.c                          |  4 +-
 kernel/bpf/bpf_task_storage.c                 |  4 +-
 kernel/bpf/btf.c                              | 24 ++++-
 kernel/bpf/stackmap.c                         |  3 +
 kernel/bpf/task_iter.c                        |  2 +-
 kernel/bpf/verifier.c                         | 99 +++++++++++++------
 kernel/trace/bpf_trace.c                      | 12 +++
 net/core/bpf_sk_storage.c                     |  9 +-
 net/core/filter.c                             | 52 ++++++----
 net/ipv4/bpf_tcp_ca.c                         |  4 +-
 .../selftests/bpf/prog_tests/d_path_crash.c   | 19 ++++
 .../selftests/bpf/progs/d_path_crash.c        | 26 +++++
 .../selftests/bpf/verifier/bounds_deduction.c |  2 +-
 tools/testing/selftests/bpf/verifier/ctx.c    |  8 +-
 17 files changed, 226 insertions(+), 68 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/d_path_crash.c
 create mode 100644 tools/testing/selftests/bpf/progs/d_path_crash.c

-- 
2.35.1

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

[Kumar Kartikeya Dwivedi]

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

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

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

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

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

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

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

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

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

[PATCH bpf v1 2/5] bpf: Restrict PTR_TO_BTF_ID offset to PAGE_SIZE when calling helpers

[Kumar Kartikeya Dwivedi]

In the verifier, PTR_TO_BTF_ID is a bit special, as it can be NULL at
runtime. Helpers and kfuncs are expected to handle this case by NULL
checking even if not taking the OR_NULL variant of PTR_TO_BTF_ID, as it
can be obtained using pointer walking (btf_struct_walk) and be NULL.

The first job is to fix this assumption for all helpers, which
subsequent commits will do.

But a more serious issue occurs when the BTF ID that a helper or kfunc
is expecting may be an embedded object in another object whose
PTR_TO_BTF_ID program has access to. In that case, that parent pointer
may be NULL, and the pointer to member embedded in it would be formed by
adding member offset to the pointer.

Ultimately, verifier does a check using btf_struct_ids_match which takes
into account the non-zero reg->off and verifier the pointer to member
formed by pointer increment is same as the one helper is expecting.
However, it may have been formed using a parent pointer that is NULL.

This case would subvert the NULL check that (some) helpers correctly do,
hence some adjustment is needed, because the resulting pointer is NULL +
offset.

First, since all architectures fault in the first page size bytes for
NULL pointer dereference reliably (and consider any other faulting
address to be bad page fault), limit the offset that can be added and
passed to the helper function to PAGE_SIZE bytes, considering higher
address may be actually be valid.

This is a backwards incompatible change, as it restricts to forming
member pointers for structs whose size < PAGE_SIZE, or more precisely
member offset lies in first page, when passing to BPF helpers. The
kfuncs are already unstable so no concerns there. It could be possible
to bump it to higher limit as long as it doesn't overlap with valid
address range.

A test case in later commit is included which crashes the kernel by
forming a struct path * from a NULL struct file *, and then passes it to
bpf_d_path which uses struct path * without a NULL check, which causes
kernel crash without this fix.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 include/linux/bpf.h   | 19 +++++++++++++++++++
 kernel/bpf/btf.c      |  7 +++++++
 kernel/bpf/verifier.c | 10 ++++++++++
 3 files changed, 36 insertions(+)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index d0ad379d1e62..17d4bbf69cb6 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -494,6 +494,23 @@ enum bpf_reg_type {
 	 * assumptions about null or non-null when doing branch analysis.
 	 * Further, when passed into helpers the helpers can not, without
 	 * additional context, assume the value is non-null.
+	 *
+	 * All BPF helpers must use IS_PTR_TO_BTF_ID_ERR_OR_NULL when accepting
+	 * a PTR_TO_BTF_ID or PTR_TO_BTF_ID_OR_NULL from a BPF program.
+	 * Likewise, unstable kfuncs must do the same. This is because while
+	 * PTR_TO_BTF_ID may be NULL at runtime, a pointer to the embedded
+	 * object can be formed by adding the offset to the member, and then
+	 * passing verifier checks because verifier thinks that:
+	 *
+	 * (struct file *)ptr + offsetof(struct file, f_path) == (struct path *)
+	 *
+	 * This logic in BTF ID check is needed to pass pointer to objects
+	 * embedded in other objects user has pointer to, but in the NULL
+	 * pointer case for PTR_TO_BTF_ID reg state, it will allow passing
+	 * NULL + offset, which won't be rejected because it is not NULL.
+	 *
+	 * Hence, the IS_PTR_TO_BTF_ID_ERR_OR_NULL macro is needed to provide
+	 * safety for both NULL and this 'non-NULL but invalid pointer case'.
 	 */
 	PTR_TO_BTF_ID,
 	/* PTR_TO_BTF_ID_OR_NULL points to a kernel struct that has not
@@ -520,6 +537,8 @@ enum bpf_reg_type {
 };
 static_assert(__BPF_REG_TYPE_MAX <= BPF_BASE_TYPE_LIMIT);
 
+#define IS_PTR_TO_BTF_ID_ERR_OR_NULL(p) ((unsigned long)(p) < PAGE_SIZE)
+
 /* The information passed from prog-specific *_is_valid_access
  * back to the verifier.
  */
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 9c8c429aa4dc..9db92b60ea9e 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -5714,6 +5714,13 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 							    &reg_ref_id);
 			reg_ref_tname = btf_name_by_offset(reg_btf,
 							   reg_ref_t->name_off);
+
+			if (reg->type == PTR_TO_BTF_ID && (reg->off < 0 || reg->off >= PAGE_SIZE)) {
+				bpf_log(log,
+					"R%d type=ptr_%s off=%d must be in range [0, %lu) when passing into kfunc\n",
+					regno, reg_ref_tname, reg->off, PAGE_SIZE);
+			}
+
 			/* In case of PTR_TO_SOCKET, PTR_TO_SOCK_COMMON,
 			 * PTR_TO_TCP_SOCK, we do type check using BTF IDs of
 			 * in-kernel types they point to, but
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 732dcba85ce5..90972bff1c54 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -5257,6 +5257,16 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno,
 			arg_btf_id = compatible->btf_id;
 		}
 
+		/* reg->off may be < 0 here, as check_func_arg_reg_off is
+		 * called later.
+		 */
+		if (reg->off < 0 || reg->off >= PAGE_SIZE) {
+			verbose(env,
+				"R%d type=%s%s off=%d must be in range [0, %lu) when passing into helper\n",
+				regno, reg_type_str(env, reg->type), kernel_type_name(reg->btf, reg->btf_id),
+				reg->off, PAGE_SIZE);
+		}
+
 		if (!btf_struct_ids_match(&env->log, reg->btf, reg->btf_id, reg->off,
 					  btf_vmlinux, *arg_btf_id)) {
 			verbose(env, "R%d is of type %s but %s is expected\n",
-- 
2.35.1

[PATCH bpf v1 3/5] bpf: Use bpf_ptr_is_invalid for all helpers taking PTR_TO_BTF_ID

[Kumar Kartikeya Dwivedi]

Switch all helpers accepting ARG_PTR_TO_BTF_ID to use bpf_ptr_is_invalid
to check NULL or invalid pointers formed in the program.

Some helpers missed checking for NULL when taking PTR_TO_BTF_ID, those
are also marked as fixed along with others that need updating due to the
new convention.

Fixes: 492e639f0c22 ("bpf: Add bpf_seq_printf and bpf_seq_write helpers")
Fixes: eb411377aed9 ("bpf: Add bpf_seq_printf_btf helper")
Fixes: 6e22ab9da793 ("bpf: Add d_path helper")
Fixes: dd6e10fbd9fb ("bpf: Add bpf_task_pt_regs() helper")
Fixes: f307fa2cb4c9 ("bpf: Introduce bpf_sk_{, ancestor_}cgroup_id helpers")
Fixes: 4de16969523c ("bpf: enable event output helper also for xdp types")
Fixes: c5dbb89fc2ac ("bpf: Expose bpf_get_socket_cookie to tracing programs")
Fixes: 3cee6fb8e69e ("bpf: tcp: Support bpf_(get|set)sockopt in bpf tcp iter")
Fixes: a5fa25adf03d ("bpf: Change bpf_sk_release and bpf_sk_*cgroup_id to accept ARG_PTR_TO_BTF_ID_SOCK_COMMON")
Fixes: c0df236e1394 ("bpf: Change bpf_tcp_*_syncookie to accept ARG_PTR_TO_BTF_ID_SOCK_COMMON")
Fixes: 27e5203bd9c5 ("bpf: Change bpf_sk_assign to accept ARG_PTR_TO_BTF_ID_SOCK_COMMON")
Fixes: af7ec1383361 ("bpf: Add bpf_skc_to_tcp6_sock() helper")
Fixes: 478cfbdf5f13 ("bpf: Add bpf_skc_to_{tcp, tcp_timewait, tcp_request}_sock() helpers")
Fixes: 0d4fad3e57df ("bpf: Add bpf_skc_to_udp6_sock() helper")
Fixes: 9eeb3aa33ae0 ("bpf: Add bpf_skc_to_unix_sock() helper")
Fixes: 4f19cab76136 ("bpf: Add a bpf_sock_from_file helper")
Fixes: fa28dcb82a38 ("bpf: Introduce helper bpf_get_task_stack()")
Fixes: 7c7e3d31e785 ("bpf: Introduce helper bpf_find_vma")
Fixes: 4cf1bc1f1045 ("bpf: Implement task local storage")
Fixes: 3f6719c7b62f ("bpf: Add bpf_bprm_opts_set helper")
Fixes: 27672f0d280a ("bpf: Add a BPF helper for getting the IMA hash of an inode")
Fixes: 8ea636848aca ("bpf: Implement bpf_local_storage for inodes")
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 include/linux/bpf.h            | 18 ++++++------
 kernel/bpf/bpf_inode_storage.c |  4 +--
 kernel/bpf/bpf_lsm.c           |  4 ++-
 kernel/bpf/bpf_task_storage.c  |  4 +--
 kernel/bpf/stackmap.c          |  3 ++
 kernel/bpf/task_iter.c         |  2 +-
 kernel/trace/bpf_trace.c       | 12 ++++++++
 net/core/bpf_sk_storage.c      |  9 +++---
 net/core/filter.c              | 52 +++++++++++++++++++++-------------
 net/ipv4/bpf_tcp_ca.c          |  4 +--
 10 files changed, 72 insertions(+), 40 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 17d4bbf69cb6..83deca7afc88 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -495,12 +495,12 @@ enum bpf_reg_type {
 	 * Further, when passed into helpers the helpers can not, without
 	 * additional context, assume the value is non-null.
 	 *
-	 * All BPF helpers must use IS_PTR_TO_BTF_ID_ERR_OR_NULL when accepting
-	 * a PTR_TO_BTF_ID or PTR_TO_BTF_ID_OR_NULL from a BPF program.
-	 * Likewise, unstable kfuncs must do the same. This is because while
-	 * PTR_TO_BTF_ID may be NULL at runtime, a pointer to the embedded
-	 * object can be formed by adding the offset to the member, and then
-	 * passing verifier checks because verifier thinks that:
+	 * All BPF helpers must use 'bpf_ptr_is_invalid' when accepting a
+	 * PTR_TO_BTF_ID or PTR_TO_BTF_ID_OR_NULL from a BPF program. Likewise,
+	 * unstable kfuncs must do the same. This is because while PTR_TO_BTF_ID
+	 * may be NULL at runtime, a pointer to the embedded object can be
+	 * formed by adding the offset to the member, and then passing verifier
+	 * checks because verifier thinks that:
 	 *
 	 * (struct file *)ptr + offsetof(struct file, f_path) == (struct path *)
 	 *
@@ -509,8 +509,8 @@ enum bpf_reg_type {
 	 * pointer case for PTR_TO_BTF_ID reg state, it will allow passing
 	 * NULL + offset, which won't be rejected because it is not NULL.
 	 *
-	 * Hence, the IS_PTR_TO_BTF_ID_ERR_OR_NULL macro is needed to provide
-	 * safety for both NULL and this 'non-NULL but invalid pointer case'.
+	 * Hence, the 'bpf_ptr_is_invalid' macro is needed to provide safety for
+	 * both NULL and this 'non-NULL but invalid pointer case'.
 	 */
 	PTR_TO_BTF_ID,
 	/* PTR_TO_BTF_ID_OR_NULL points to a kernel struct that has not
@@ -537,7 +537,7 @@ enum bpf_reg_type {
 };
 static_assert(__BPF_REG_TYPE_MAX <= BPF_BASE_TYPE_LIMIT);
 
-#define IS_PTR_TO_BTF_ID_ERR_OR_NULL(p) ((unsigned long)(p) < PAGE_SIZE)
+#define bpf_ptr_is_invalid(p) (unlikely((unsigned long)(p) < PAGE_SIZE))
 
 /* The information passed from prog-specific *_is_valid_access
  * back to the verifier.
diff --git a/kernel/bpf/bpf_inode_storage.c b/kernel/bpf/bpf_inode_storage.c
index e29d9e3d853e..b36b1609789e 100644
--- a/kernel/bpf/bpf_inode_storage.c
+++ b/kernel/bpf/bpf_inode_storage.c
@@ -183,7 +183,7 @@ BPF_CALL_4(bpf_inode_storage_get, struct bpf_map *, map, struct inode *, inode,
 	 * bpf_local_storage_update expects the owner to have a
 	 * valid storage pointer.
 	 */
-	if (!inode || !inode_storage_ptr(inode))
+	if (bpf_ptr_is_invalid(inode) || !inode_storage_ptr(inode))
 		return (unsigned long)NULL;
 
 	sdata = inode_storage_lookup(inode, map, true);
@@ -208,7 +208,7 @@ BPF_CALL_2(bpf_inode_storage_delete,
 	   struct bpf_map *, map, struct inode *, inode)
 {
 	WARN_ON_ONCE(!bpf_rcu_lock_held());
-	if (!inode)
+	if (bpf_ptr_is_invalid(inode))
 		return -EINVAL;
 
 	/* This helper must only called from where the inode is guaranteed
diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
index 9e4ecc990647..c60e06a5b79d 100644
--- a/kernel/bpf/bpf_lsm.c
+++ b/kernel/bpf/bpf_lsm.c
@@ -58,7 +58,7 @@ int bpf_lsm_verify_prog(struct bpf_verifier_log *vlog,
 
 BPF_CALL_2(bpf_bprm_opts_set, struct linux_binprm *, bprm, u64, flags)
 {
-	if (flags & ~BPF_F_BRPM_OPTS_MASK)
+	if (bpf_ptr_is_invalid(bprm) || flags & ~BPF_F_BRPM_OPTS_MASK)
 		return -EINVAL;
 
 	bprm->secureexec = (flags & BPF_F_BPRM_SECUREEXEC);
@@ -78,6 +78,8 @@ static const struct bpf_func_proto bpf_bprm_opts_set_proto = {
 
 BPF_CALL_3(bpf_ima_inode_hash, struct inode *, inode, void *, dst, u32, size)
 {
+	if (bpf_ptr_is_invalid(inode))
+		return -EINVAL;
 	return ima_inode_hash(inode, dst, size);
 }
 
diff --git a/kernel/bpf/bpf_task_storage.c b/kernel/bpf/bpf_task_storage.c
index 5da7bed0f5f6..85bca62fc1e1 100644
--- a/kernel/bpf/bpf_task_storage.c
+++ b/kernel/bpf/bpf_task_storage.c
@@ -235,7 +235,7 @@ BPF_CALL_4(bpf_task_storage_get, struct bpf_map *, map, struct task_struct *,
 	if (flags & ~(BPF_LOCAL_STORAGE_GET_F_CREATE))
 		return (unsigned long)NULL;
 
-	if (!task)
+	if (bpf_ptr_is_invalid(task))
 		return (unsigned long)NULL;
 
 	if (!bpf_task_storage_trylock())
@@ -264,7 +264,7 @@ BPF_CALL_2(bpf_task_storage_delete, struct bpf_map *, map, struct task_struct *,
 	int ret;
 
 	WARN_ON_ONCE(!bpf_rcu_lock_held());
-	if (!task)
+	if (bpf_ptr_is_invalid(task))
 		return -EINVAL;
 
 	if (!bpf_task_storage_trylock())
diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c
index 22c8ae94e4c1..6eb4be83f04d 100644
--- a/kernel/bpf/stackmap.c
+++ b/kernel/bpf/stackmap.c
@@ -474,6 +474,9 @@ BPF_CALL_4(bpf_get_task_stack, struct task_struct *, task, void *, buf,
 	struct pt_regs *regs;
 	long res = -EINVAL;
 
+	if (bpf_ptr_is_invalid(task))
+		return -EINVAL;
+
 	if (!try_get_task_stack(task))
 		return -EFAULT;
 
diff --git a/kernel/bpf/task_iter.c b/kernel/bpf/task_iter.c
index d94696198ef8..ec27de5e75f4 100644
--- a/kernel/bpf/task_iter.c
+++ b/kernel/bpf/task_iter.c
@@ -595,7 +595,7 @@ BPF_CALL_5(bpf_find_vma, struct task_struct *, task, u64, start,
 	if (flags)
 		return -EINVAL;
 
-	if (!task)
+	if (bpf_ptr_is_invalid(task))
 		return -ENOENT;
 
 	mm = task->mm;
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 21aa30644219..6ea6ded8f9fc 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -468,6 +468,9 @@ BPF_CALL_5(bpf_seq_printf, struct seq_file *, m, char *, fmt, u32, fmt_size,
 	int err, num_args;
 	u32 *bin_args;
 
+	if (bpf_ptr_is_invalid(m))
+		return -EINVAL;
+
 	if (data_len & 7 || data_len > MAX_BPRINTF_VARARGS * 8 ||
 	    (data_len && !data))
 		return -EINVAL;
@@ -500,6 +503,8 @@ static const struct bpf_func_proto bpf_seq_printf_proto = {
 
 BPF_CALL_3(bpf_seq_write, struct seq_file *, m, const void *, data, u32, len)
 {
+	if (bpf_ptr_is_invalid(m))
+		return -EINVAL;
 	return seq_write(m, data, len) ? -EOVERFLOW : 0;
 }
 
@@ -520,6 +525,9 @@ BPF_CALL_4(bpf_seq_printf_btf, struct seq_file *, m, struct btf_ptr *, ptr,
 	s32 btf_id;
 	int ret;
 
+	if (bpf_ptr_is_invalid(m))
+		return -EINVAL;
+
 	ret = bpf_btf_printf_prepare(ptr, btf_ptr_size, flags, &btf, &btf_id);
 	if (ret)
 		return ret;
@@ -769,6 +777,8 @@ const struct bpf_func_proto bpf_get_current_task_btf_proto = {
 
 BPF_CALL_1(bpf_task_pt_regs, struct task_struct *, task)
 {
+	if (bpf_ptr_is_invalid(task))
+		return -EINVAL;
 	return (unsigned long) task_pt_regs(task);
 }
 
@@ -894,6 +904,8 @@ BPF_CALL_3(bpf_d_path, struct path *, path, char *, buf, u32, sz)
 	long len;
 	char *p;
 
+	if (bpf_ptr_is_invalid(path))
+		return -EINVAL;
 	if (!sz)
 		return 0;
 
diff --git a/net/core/bpf_sk_storage.c b/net/core/bpf_sk_storage.c
index d9c37fd10809..836f43b160a8 100644
--- a/net/core/bpf_sk_storage.c
+++ b/net/core/bpf_sk_storage.c
@@ -261,7 +261,8 @@ BPF_CALL_4(bpf_sk_storage_get, struct bpf_map *, map, struct sock *, sk,
 	struct bpf_local_storage_data *sdata;
 
 	WARN_ON_ONCE(!bpf_rcu_lock_held());
-	if (!sk || !sk_fullsock(sk) || flags > BPF_SK_STORAGE_GET_F_CREATE)
+	if (bpf_ptr_is_invalid(sk) ||
+	    !sk_fullsock(sk) || flags > BPF_SK_STORAGE_GET_F_CREATE)
 		return (unsigned long)NULL;
 
 	sdata = bpf_sk_storage_lookup(sk, map, true);
@@ -292,7 +293,7 @@ BPF_CALL_4(bpf_sk_storage_get, struct bpf_map *, map, struct sock *, sk,
 BPF_CALL_2(bpf_sk_storage_delete, struct bpf_map *, map, struct sock *, sk)
 {
 	WARN_ON_ONCE(!bpf_rcu_lock_held());
-	if (!sk || !sk_fullsock(sk))
+	if (bpf_ptr_is_invalid(sk) || !sk_fullsock(sk))
 		return -EINVAL;
 
 	if (refcount_inc_not_zero(&sk->sk_refcnt)) {
@@ -421,7 +422,7 @@ BPF_CALL_4(bpf_sk_storage_get_tracing, struct bpf_map *, map, struct sock *, sk,
 	   void *, value, u64, flags)
 {
 	WARN_ON_ONCE(!bpf_rcu_lock_held());
-	if (in_hardirq() || in_nmi())
+	if (bpf_ptr_is_invalid(sk) || in_hardirq() || in_nmi())
 		return (unsigned long)NULL;
 
 	return (unsigned long)____bpf_sk_storage_get(map, sk, value, flags);
@@ -431,7 +432,7 @@ BPF_CALL_2(bpf_sk_storage_delete_tracing, struct bpf_map *, map,
 	   struct sock *, sk)
 {
 	WARN_ON_ONCE(!bpf_rcu_lock_held());
-	if (in_hardirq() || in_nmi())
+	if (bpf_ptr_is_invalid(sk) || in_hardirq() || in_nmi())
 		return -EPERM;
 
 	return ____bpf_sk_storage_delete(map, sk);
diff --git a/net/core/filter.c b/net/core/filter.c
index 9eb785842258..a578df364273 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -4516,7 +4516,7 @@ static inline u64 __bpf_sk_cgroup_id(struct sock *sk)
 	struct cgroup *cgrp;
 
 	sk = sk_to_full_sk(sk);
-	if (!sk || !sk_fullsock(sk))
+	if (bpf_ptr_is_invalid(sk) || !sk_fullsock(sk))
 		return 0;
 
 	cgrp = sock_cgroup_ptr(&sk->sk_cgrp_data);
@@ -4542,7 +4542,7 @@ static inline u64 __bpf_sk_ancestor_cgroup_id(struct sock *sk,
 	struct cgroup *cgrp;
 
 	sk = sk_to_full_sk(sk);
-	if (!sk || !sk_fullsock(sk))
+	if (bpf_ptr_is_invalid(sk) || !sk_fullsock(sk))
 		return 0;
 
 	cgrp = sock_cgroup_ptr(&sk->sk_cgrp_data);
@@ -4569,6 +4569,7 @@ static const struct bpf_func_proto bpf_skb_ancestor_cgroup_id_proto = {
 
 BPF_CALL_1(bpf_sk_cgroup_id, struct sock *, sk)
 {
+	/* __bpf_sk_cgroup_id does bpf_ptr_is_invalid check */
 	return __bpf_sk_cgroup_id(sk);
 }
 
@@ -4581,6 +4582,7 @@ static const struct bpf_func_proto bpf_sk_cgroup_id_proto = {
 
 BPF_CALL_2(bpf_sk_ancestor_cgroup_id, struct sock *, sk, int, ancestor_level)
 {
+	/* __bpf_sk_ancestor_cgroup_id does bpf_ptr_is_invalid check */
 	return __bpf_sk_ancestor_cgroup_id(sk, ancestor_level);
 }
 
@@ -4607,7 +4609,7 @@ BPF_CALL_5(bpf_xdp_event_output, struct xdp_buff *, xdp, struct bpf_map *, map,
 
 	if (unlikely(flags & ~(BPF_F_CTXLEN_MASK | BPF_F_INDEX_MASK)))
 		return -EINVAL;
-	if (unlikely(!xdp ||
+	if (unlikely(bpf_ptr_is_invalid(xdp) ||
 		     xdp_size > (unsigned long)(xdp->data_end - xdp->data)))
 		return -EFAULT;
 
@@ -4678,7 +4680,7 @@ static const struct bpf_func_proto bpf_get_socket_cookie_sock_proto = {
 
 BPF_CALL_1(bpf_get_socket_ptr_cookie, struct sock *, sk)
 {
-	return sk ? sock_gen_cookie(sk) : 0;
+	return !bpf_ptr_is_invalid(sk) ? sock_gen_cookie(sk) : 0;
 }
 
 const struct bpf_func_proto bpf_get_socket_ptr_cookie_proto = {
@@ -5015,7 +5017,7 @@ static int _bpf_setsockopt(struct sock *sk, int level, int optname,
 static int _bpf_getsockopt(struct sock *sk, int level, int optname,
 			   char *optval, int optlen)
 {
-	if (!sk_fullsock(sk))
+	if (bpf_ptr_is_invalid(sk) || !sk_fullsock(sk))
 		goto err_clear;
 
 	sock_owned_by_me(sk);
@@ -5114,6 +5116,9 @@ static int _bpf_getsockopt(struct sock *sk, int level, int optname,
 BPF_CALL_5(bpf_sk_setsockopt, struct sock *, sk, int, level,
 	   int, optname, char *, optval, int, optlen)
 {
+	if (bpf_ptr_is_invalid(sk))
+		return -EINVAL;
+
 	if (level == SOL_TCP && optname == TCP_CONGESTION) {
 		if (optlen >= sizeof("cdg") - 1 &&
 		    !strncmp("cdg", optval, optlen))
@@ -5137,6 +5142,7 @@ const struct bpf_func_proto bpf_sk_setsockopt_proto = {
 BPF_CALL_5(bpf_sk_getsockopt, struct sock *, sk, int, level,
 	   int, optname, char *, optval, int, optlen)
 {
+	/* _bpf_getsockopt does bpf_ptr_is_invalid check */
 	return _bpf_getsockopt(sk, level, optname, optval, optlen);
 }
 
@@ -6373,7 +6379,7 @@ static const struct bpf_func_proto bpf_sk_lookup_udp_proto = {
 
 BPF_CALL_1(bpf_sk_release, struct sock *, sk)
 {
-	if (sk && sk_is_refcounted(sk))
+	if (!bpf_ptr_is_invalid(sk) && sk_is_refcounted(sk))
 		sock_gen_put(sk);
 	return 0;
 }
@@ -6764,7 +6770,7 @@ BPF_CALL_5(bpf_tcp_check_syncookie, struct sock *, sk, void *, iph, u32, iph_len
 	u32 cookie;
 	int ret;
 
-	if (unlikely(!sk || th_len < sizeof(*th)))
+	if (unlikely(bpf_ptr_is_invalid(sk) || th_len < sizeof(*th)))
 		return -EINVAL;
 
 	/* sk_listener() allows TCP_NEW_SYN_RECV, which makes no sense here. */
@@ -6831,7 +6837,8 @@ BPF_CALL_5(bpf_tcp_gen_syncookie, struct sock *, sk, void *, iph, u32, iph_len,
 	u32 cookie;
 	u16 mss;
 
-	if (unlikely(!sk || th_len < sizeof(*th) || th_len != th->doff * 4))
+	if (unlikely(bpf_ptr_is_invalid(sk) || th_len < sizeof(*th) ||
+		     th_len != th->doff * 4))
 		return -EINVAL;
 
 	if (sk->sk_protocol != IPPROTO_TCP || sk->sk_state != TCP_LISTEN)
@@ -6895,7 +6902,7 @@ static const struct bpf_func_proto bpf_tcp_gen_syncookie_proto = {
 
 BPF_CALL_3(bpf_sk_assign, struct sk_buff *, skb, struct sock *, sk, u64, flags)
 {
-	if (!sk || flags != 0)
+	if (bpf_ptr_is_invalid(sk) || flags != 0)
 		return -EINVAL;
 	if (!skb_at_tc_ingress(skb))
 		return -EOPNOTSUPP;
@@ -10737,8 +10744,8 @@ BPF_CALL_1(bpf_skc_to_tcp6_sock, struct sock *, sk)
 	 * trigger an explicit type generation here.
 	 */
 	BTF_TYPE_EMIT(struct tcp6_sock);
-	if (sk && sk_fullsock(sk) && sk->sk_protocol == IPPROTO_TCP &&
-	    sk->sk_family == AF_INET6)
+	if (!bpf_ptr_is_invalid(sk) && sk_fullsock(sk) &&
+	    sk->sk_protocol == IPPROTO_TCP && sk->sk_family == AF_INET6)
 		return (unsigned long)sk;
 
 	return (unsigned long)NULL;
@@ -10754,7 +10761,7 @@ const struct bpf_func_proto bpf_skc_to_tcp6_sock_proto = {
 
 BPF_CALL_1(bpf_skc_to_tcp_sock, struct sock *, sk)
 {
-	if (sk && sk_fullsock(sk) && sk->sk_protocol == IPPROTO_TCP)
+	if (!bpf_ptr_is_invalid(sk) && sk_fullsock(sk) && sk->sk_protocol == IPPROTO_TCP)
 		return (unsigned long)sk;
 
 	return (unsigned long)NULL;
@@ -10776,13 +10783,15 @@ BPF_CALL_1(bpf_skc_to_tcp_timewait_sock, struct sock *, sk)
 	BTF_TYPE_EMIT(struct inet_timewait_sock);
 	BTF_TYPE_EMIT(struct tcp_timewait_sock);
 
+	if (bpf_ptr_is_invalid(sk))
+		return (unsigned long)NULL;
 #ifdef CONFIG_INET
-	if (sk && sk->sk_prot == &tcp_prot && sk->sk_state == TCP_TIME_WAIT)
+	if (sk->sk_prot == &tcp_prot && sk->sk_state == TCP_TIME_WAIT)
 		return (unsigned long)sk;
 #endif
 
 #if IS_BUILTIN(CONFIG_IPV6)
-	if (sk && sk->sk_prot == &tcpv6_prot && sk->sk_state == TCP_TIME_WAIT)
+	if (sk->sk_prot == &tcpv6_prot && sk->sk_state == TCP_TIME_WAIT)
 		return (unsigned long)sk;
 #endif
 
@@ -10799,13 +10808,15 @@ const struct bpf_func_proto bpf_skc_to_tcp_timewait_sock_proto = {
 
 BPF_CALL_1(bpf_skc_to_tcp_request_sock, struct sock *, sk)
 {
+	if (bpf_ptr_is_invalid(sk))
+		return (unsigned long)NULL;
 #ifdef CONFIG_INET
-	if (sk && sk->sk_prot == &tcp_prot && sk->sk_state == TCP_NEW_SYN_RECV)
+	if (sk->sk_prot == &tcp_prot && sk->sk_state == TCP_NEW_SYN_RECV)
 		return (unsigned long)sk;
 #endif
 
 #if IS_BUILTIN(CONFIG_IPV6)
-	if (sk && sk->sk_prot == &tcpv6_prot && sk->sk_state == TCP_NEW_SYN_RECV)
+	if (sk->sk_prot == &tcpv6_prot && sk->sk_state == TCP_NEW_SYN_RECV)
 		return (unsigned long)sk;
 #endif
 
@@ -10826,8 +10837,9 @@ BPF_CALL_1(bpf_skc_to_udp6_sock, struct sock *, sk)
 	 * trigger an explicit type generation here.
 	 */
 	BTF_TYPE_EMIT(struct udp6_sock);
-	if (sk && sk_fullsock(sk) && sk->sk_protocol == IPPROTO_UDP &&
-	    sk->sk_type == SOCK_DGRAM && sk->sk_family == AF_INET6)
+	if (!bpf_ptr_is_invalid(sk) && sk_fullsock(sk) &&
+	    sk->sk_protocol == IPPROTO_UDP && sk->sk_type == SOCK_DGRAM &&
+	    sk->sk_family == AF_INET6)
 		return (unsigned long)sk;
 
 	return (unsigned long)NULL;
@@ -10847,7 +10859,7 @@ BPF_CALL_1(bpf_skc_to_unix_sock, struct sock *, sk)
 	 * trigger an explicit type generation here.
 	 */
 	BTF_TYPE_EMIT(struct unix_sock);
-	if (sk && sk_fullsock(sk) && sk->sk_family == AF_UNIX)
+	if (!bpf_ptr_is_invalid(sk) && sk_fullsock(sk) && sk->sk_family == AF_UNIX)
 		return (unsigned long)sk;
 
 	return (unsigned long)NULL;
@@ -10863,6 +10875,8 @@ const struct bpf_func_proto bpf_skc_to_unix_sock_proto = {
 
 BPF_CALL_1(bpf_sock_from_file, struct file *, file)
 {
+	if (bpf_ptr_is_invalid(file))
+		return (unsigned long)NULL;
 	return (unsigned long)sock_from_file(file);
 }
 
diff --git a/net/ipv4/bpf_tcp_ca.c b/net/ipv4/bpf_tcp_ca.c
index de610cb83694..e5f2f0fe46be 100644
--- a/net/ipv4/bpf_tcp_ca.c
+++ b/net/ipv4/bpf_tcp_ca.c
@@ -144,7 +144,8 @@ static int bpf_tcp_ca_btf_struct_access(struct bpf_verifier_log *log,
 
 BPF_CALL_2(bpf_tcp_send_ack, struct tcp_sock *, tp, u32, rcv_nxt)
 {
-	/* bpf_tcp_ca prog cannot have NULL tp */
+	if (bpf_ptr_is_invalid(tp))
+		return -EINVAL;
 	__tcp_send_ack((struct sock *)tp, rcv_nxt);
 	return 0;
 }
@@ -152,7 +153,6 @@ BPF_CALL_2(bpf_tcp_send_ack, struct tcp_sock *, tp, u32, rcv_nxt)
 static const struct bpf_func_proto bpf_tcp_send_ack_proto = {
 	.func		= bpf_tcp_send_ack,
 	.gpl_only	= false,
-	/* In case we want to report error later */
 	.ret_type	= RET_INTEGER,
 	.arg1_type	= ARG_PTR_TO_BTF_ID,
 	.arg1_btf_id	= &tcp_sock_id,
-- 
2.35.1

[PATCH bpf v1 4/5] selftests/bpf: Add selftest for PTR_TO_BTF_ID NULL + off case

[Kumar Kartikeya Dwivedi]

While at it, also try breaking bpf_sock_from_file, since it doesn't
check its argument for NULL in the first place. With our fix, both
shouldn't crash.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 .../selftests/bpf/prog_tests/d_path_crash.c   | 19 ++++++++++++++
 .../selftests/bpf/progs/d_path_crash.c        | 26 +++++++++++++++++++
 2 files changed, 45 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/d_path_crash.c
 create mode 100644 tools/testing/selftests/bpf/progs/d_path_crash.c

diff --git a/tools/testing/selftests/bpf/prog_tests/d_path_crash.c b/tools/testing/selftests/bpf/prog_tests/d_path_crash.c
new file mode 100644
index 000000000000..b1ee705d2108
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/d_path_crash.c
@@ -0,0 +1,19 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <test_progs.h>
+#include <fcntl.h>
+#include <unistd.h>
+
+#include "d_path_crash.skel.h"
+
+void test_d_path_crash(void)
+{
+	struct d_path_crash *skel;
+
+	skel = d_path_crash__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "d_path_crash__open_and_load"))
+		return;
+	skel->bss->pid = getpid();
+	ASSERT_OK(d_path_crash__attach(skel), "d_path__attach");
+	close(open("/dev/null", O_RDONLY));
+	d_path_crash__destroy(skel);
+}
diff --git a/tools/testing/selftests/bpf/progs/d_path_crash.c b/tools/testing/selftests/bpf/progs/d_path_crash.c
new file mode 100644
index 000000000000..a4b1a8b200f3
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/d_path_crash.c
@@ -0,0 +1,26 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <vmlinux.h>
+#include <bpf/bpf_tracing.h>
+#include <bpf/bpf_helpers.h>
+
+int pid = 0;
+
+SEC("lsm/file_open")
+int BPF_PROG(lsm_file_open, struct file *file)
+{
+	struct task_struct *current = bpf_get_current_task_btf();
+	unsigned long *val, l;
+	char buf[64] = {};
+	struct file *f;
+
+	if (current->tgid != pid)
+		return 0;
+
+	f = current->files->fd_array[63];
+	bpf_d_path(&f->f_path, buf, sizeof(buf));
+	/* If we survived, let's try our luck here */
+	bpf_sock_from_file(f);
+	return 0;
+}
+
+char _license[] SEC("license") = "GPL";
-- 
2.35.1

[PATCH bpf v1 5/5] selftests/bpf: Adjust verifier selftest for updated message

[Kumar Kartikeya Dwivedi]

Verifier now prints a different message on seeing negative offset for
ctx reg (and others as well), change the errstr expected by test.

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

Re: [PATCH bpf v1 0/5] More fixes for crashes due to bad PTR_TO_BTF_ID reg->off

[Kumar Kartikeya Dwivedi]

On Sat, Feb 19, 2022 at 05:07:39PM IST, Kumar Kartikeya Dwivedi wrote:
> A few more fixes for bad PTR_TO_BTF_ID reg->off being accepted in places, that
> can lead to the kernel crashing. Noticed while making sure my own series for BTF
> ID pointer in map won't allow stores for pointers with incorrect offsets.
>
> I include one example where d_path can crash even if you NULL check
> PTR_TO_BTF_ID, and one example of how missing NULL check in helper taking
> PTR_TO_BTF_ID (like bpf_sock_from_file) is a real problem, see the selftest
> patch.
>
> The &f->f_path becomes NULL + offset in case f is NULL, circumventing NULL
> checks in existing helpers. The only thing needed to trigger this finding an
> object that embeds the object of interest, and then somehow obtaining a NULL
> PTR_TO_BTF_ID to it (not hard, esp. due to exception handler for PROBE_MEM loads
> writing 0 to destination register).
>
> However, for the case of patch 2, it is allowed in my series since the next load
> of the bad pointer stored using:
>   struct file *f = ...; // some pointer walking returning NULL pointer
>   map_val->ptr = &f->f_path; // ptr being struct path *
> ... would be marked as PTR_UNTRUSTED, so it won't be allowed to be passed into
> the kernel, and hence can be permitted. In referenced case, the PTR_TO_BTF_ID
> should not be NULL anyway. kptr_get style helper takes PTR_TO_MAP_VALUE in
> referenced ptr case only, so the load either yields NULL or RCU protected
> pointer.
>
> Tests for patch 1 depend on fixup_kfunc_btf_id in test_verifier, hence will be
> sent after merge window opens, some other changes after bpf tree merges into
> bpf-next, but all pending ones can be seen here [0]. Tests for patch 2 are
> included, and try to trigger crash without the fix, but it's not 100% reliable.
> We may need special testing helpers or kfuncs to make it thorough, but wanted to
> wait before getting feedback.
>
> Issue fixed by patch 2 is a bit more broader in scope, and would require proper
> discussion (before being applied) on the correct way forward, as it is
> technically backwards incompatible change, but hopefully never breaks real
> programs, only malicious or already incorrect ones.
>
> Also, please suggest the right "Fixes" tag for patch 2.
>
> As for patch 3 (selftest), please suggest a better way to get a certain type of
> PTR_TO_BTF_ID which can be NULL or NULL+offset. Can we add kfuncs for testing
> that return such pointers and make them available to e.g. TC progs, if the fix
> in patch 2 is acceptable?
>
>   [0]: https://github.com/kkdwivedi/linux/commits/fixes-bpf-next
>

Looking at BPF CI [1], it seems it surfaces another problem I was seeing locally
but couldn't craft a reliable test case for, that it forms a non-NULL but
invalid pointer using pointer walking, in some cases RCU read lock provides
protection for those cases, but not all (esp. if kernel doesn't clear the old
pointer that was in use before, and has it sitting in some location). RDI (arg1)
seems to be pointing somewhere behind the faulting address, which means the
&f->f_path is bad.

But this requires a larger discussion.

In that case, PAGE_SIZE thing won't help. We may have to introduce a PTR_BPF_REF
flag (e.g. set for ctx args of tracing progs, set based on BTF tagging in other
places) which tells the verifier that the pointer for the duration of program
will be valid, so it can be passed into helpers.

So for cases like &f->f_path it will work, since file + off will still have
PTR_BPF_REF set in reg state. In case of pointer walking, where dst_reg state
is updated on walk, we may have to explicitly tag members where PTR_BPF_REF can
be inherited if parent object has PTR_BPF_REF (i.e. ref to parent implies ref to
child cases).

Something like:

struct foo {
	...
	struct bar __bpf_ref *p;
	struct baz *q;
	...
}

... then if getting:

	struct foo *f = ...; // PTR_TO_BTF_ID | PTR_BPF_REF
	struct bar *p = f->p; // Inherits PTR_BPF_REF
	struct baz *q = f->q; // Does not inherit PTR_BPF_REF

Thoughts?

  [1]: https://github.com/kernel-patches/bpf/runs/5258413028?check_suite_focus=true

> Kumar Kartikeya Dwivedi (5):
>   bpf: Fix kfunc register offset check for PTR_TO_BTF_ID
>   bpf: Restrict PTR_TO_BTF_ID offset to PAGE_SIZE when calling helpers
>   bpf: Use bpf_ptr_is_invalid for all helpers taking PTR_TO_BTF_ID
>   selftests/bpf: Add selftest for PTR_TO_BTF_ID NULL + off case
>   selftests/bpf: Adjust verifier selftest for updated message
>
>  include/linux/bpf.h                           | 19 ++++
>  include/linux/bpf_verifier.h                  |  3 +
>  kernel/bpf/bpf_inode_storage.c                |  4 +-
>  kernel/bpf/bpf_lsm.c                          |  4 +-
>  kernel/bpf/bpf_task_storage.c                 |  4 +-
>  kernel/bpf/btf.c                              | 24 ++++-
>  kernel/bpf/stackmap.c                         |  3 +
>  kernel/bpf/task_iter.c                        |  2 +-
>  kernel/bpf/verifier.c                         | 99 +++++++++++++------
>  kernel/trace/bpf_trace.c                      | 12 +++
>  net/core/bpf_sk_storage.c                     |  9 +-
>  net/core/filter.c                             | 52 ++++++----
>  net/ipv4/bpf_tcp_ca.c                         |  4 +-
>  .../selftests/bpf/prog_tests/d_path_crash.c   | 19 ++++
>  .../selftests/bpf/progs/d_path_crash.c        | 26 +++++
>  .../selftests/bpf/verifier/bounds_deduction.c |  2 +-
>  tools/testing/selftests/bpf/verifier/ctx.c    |  8 +-
>  17 files changed, 226 insertions(+), 68 deletions(-)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/d_path_crash.c
>  create mode 100644 tools/testing/selftests/bpf/progs/d_path_crash.c
>
> --
> 2.35.1
>

--
Kartikeya

Re: [PATCH bpf v1 0/5] More fixes for crashes due to bad PTR_TO_BTF_ID reg->off

[Alexei Starovoitov]

On Sat, Feb 19, 2022 at 05:40:35PM +0530, Kumar Kartikeya Dwivedi wrote:
> On Sat, Feb 19, 2022 at 05:07:39PM IST, Kumar Kartikeya Dwivedi wrote:
> > A few more fixes for bad PTR_TO_BTF_ID reg->off being accepted in places, that
> > can lead to the kernel crashing. Noticed while making sure my own series for BTF
> > ID pointer in map won't allow stores for pointers with incorrect offsets.
> >
> > I include one example where d_path can crash even if you NULL check
> > PTR_TO_BTF_ID, and one example of how missing NULL check in helper taking
> > PTR_TO_BTF_ID (like bpf_sock_from_file) is a real problem, see the selftest
> > patch.
> >
> > The &f->f_path becomes NULL + offset in case f is NULL, circumventing NULL
> > checks in existing helpers. The only thing needed to trigger this finding an
> > object that embeds the object of interest, and then somehow obtaining a NULL
> > PTR_TO_BTF_ID to it (not hard, esp. due to exception handler for PROBE_MEM loads
> > writing 0 to destination register).
> >
> > However, for the case of patch 2, it is allowed in my series since the next load
> > of the bad pointer stored using:
> >   struct file *f = ...; // some pointer walking returning NULL pointer
> >   map_val->ptr = &f->f_path; // ptr being struct path *
> > ... would be marked as PTR_UNTRUSTED, so it won't be allowed to be passed into
> > the kernel, and hence can be permitted. In referenced case, the PTR_TO_BTF_ID
> > should not be NULL anyway. kptr_get style helper takes PTR_TO_MAP_VALUE in
> > referenced ptr case only, so the load either yields NULL or RCU protected
> > pointer.
> >
> > Tests for patch 1 depend on fixup_kfunc_btf_id in test_verifier, hence will be
> > sent after merge window opens, some other changes after bpf tree merges into
> > bpf-next, but all pending ones can be seen here [0]. Tests for patch 2 are
> > included, and try to trigger crash without the fix, but it's not 100% reliable.
> > We may need special testing helpers or kfuncs to make it thorough, but wanted to
> > wait before getting feedback.
> >
> > Issue fixed by patch 2 is a bit more broader in scope, and would require proper
> > discussion (before being applied) on the correct way forward, as it is
> > technically backwards incompatible change, but hopefully never breaks real
> > programs, only malicious or already incorrect ones.
> >
> > Also, please suggest the right "Fixes" tag for patch 2.
> >
> > As for patch 3 (selftest), please suggest a better way to get a certain type of
> > PTR_TO_BTF_ID which can be NULL or NULL+offset. Can we add kfuncs for testing
> > that return such pointers and make them available to e.g. TC progs, if the fix
> > in patch 2 is acceptable?
> >
> >   [0]: https://github.com/kkdwivedi/linux/commits/fixes-bpf-next
> >
> 
> Looking at BPF CI [1], it seems it surfaces another problem I was seeing locally
> but couldn't craft a reliable test case for, that it forms a non-NULL but
> invalid pointer using pointer walking, in some cases RCU read lock provides
> protection for those cases, but not all (esp. if kernel doesn't clear the old
> pointer that was in use before, and has it sitting in some location). RDI (arg1)
> seems to be pointing somewhere behind the faulting address, which means the
> &f->f_path is bad.
> 
> But this requires a larger discussion.
> 
> In that case, PAGE_SIZE thing won't help. We may have to introduce a PTR_BPF_REF
> flag (e.g. set for ctx args of tracing progs, set based on BTF tagging in other
> places) which tells the verifier that the pointer for the duration of program
> will be valid, so it can be passed into helpers.
> 
> So for cases like &f->f_path it will work, since file + off will still have
> PTR_BPF_REF set in reg state. In case of pointer walking, where dst_reg state
> is updated on walk, we may have to explicitly tag members where PTR_BPF_REF can
> be inherited if parent object has PTR_BPF_REF (i.e. ref to parent implies ref to
> child cases).
> 
> Something like:
> 
> struct foo {
> 	...
> 	struct bar __bpf_ref *p;
> 	struct baz *q;
> 	...
> }
> 
> ... then if getting:
> 
> 	struct foo *f = ...; // PTR_TO_BTF_ID | PTR_BPF_REF
> 	struct bar *p = f->p; // Inherits PTR_BPF_REF
> 	struct baz *q = f->q; // Does not inherit PTR_BPF_REF
> 
> Thoughts?
> 
>   [1]: https://github.com/kernel-patches/bpf/runs/5258413028?check_suite_focus=true

fd_array wasn't zero initialized at alloc time, so it contains garbage.
fd_array[63] read that garbage.
So patches 2 and 3 don't help.
The 'fixes' list for patch 3 is ridiculous. No need.
Pls drop patch 2 and in instead of
+#define bpf_ptr_is_invalid(p) (unlikely((unsigned long)(p) < PAGE_SIZE))
do static inline function that checks
if ((unsigned long)p < user_addr_max())
and bails out.
bpf-next is fine.
Not sure whether patch 1 is strictly necessary after above change.

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

[Alexei Starovoitov]

On Sat, Feb 19, 2022 at 05:07:40PM +0530, Kumar Kartikeya Dwivedi wrote:
>  
> +/* Caller ensures reg->type does not have PTR_MAYBE_NULL */
> +int check_func_arg_reg_off(struct bpf_verifier_env *env,
> +			   const struct bpf_reg_state *reg, int regno,
> +			   bool arg_alloc_mem)
> +{
> +	enum bpf_reg_type type = reg->type;
> +	int err;
> +
> +	WARN_ON_ONCE(type & PTR_MAYBE_NULL);

So the warn was added and made things more difficult and check had to be moved
into check_mem_reg to clear that flag?
Why add that warn in the first place then?
The logic get convoluted because of that.

> +	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;
> +	}

Out of the whole patch this part is useful. The rest seems to dealing
with self inflicted pain.
Just call check_ptr_off_reg() for kfunc ?
The patch seems to be doing several things at once. Please split.

Re: [PATCH bpf v1 0/5] More fixes for crashes due to bad PTR_TO_BTF_ID reg->off

[Alexei Starovoitov]

On Sat, Feb 19, 2022 at 05:07:39PM +0530, Kumar Kartikeya Dwivedi wrote:
> 
> Tests for patch 1 depend on fixup_kfunc_btf_id in test_verifier, hence will be
> sent after merge window opens, some other changes after bpf tree merges into
> bpf-next, but all pending ones can be seen here [0].

Let's get everything through bpf-next. There is no urgency in any of these fixes.
Please post it all including ptr_to_btf_id-in-map set.

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

[Kumar Kartikeya Dwivedi]

On Sun, Feb 20, 2022 at 07:54:09AM IST, Alexei Starovoitov wrote:
> On Sat, Feb 19, 2022 at 05:07:40PM +0530, Kumar Kartikeya Dwivedi wrote:
> >
> > +/* Caller ensures reg->type does not have PTR_MAYBE_NULL */
> > +int check_func_arg_reg_off(struct bpf_verifier_env *env,
> > +			   const struct bpf_reg_state *reg, int regno,
> > +			   bool arg_alloc_mem)
> > +{
> > +	enum bpf_reg_type type = reg->type;
> > +	int err;
> > +
> > +	WARN_ON_ONCE(type & PTR_MAYBE_NULL);
>
> So the warn was added and made things more difficult and check had to be moved
> into check_mem_reg to clear that flag?
> Why add that warn in the first place then?
> The logic get convoluted because of that.
>

Ok, will drop.

> > +	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;
> > +	}
>
> Out of the whole patch this part is useful. The rest seems to dealing
> with self inflicted pain.
> Just call check_ptr_off_reg() for kfunc ?

I still think we should call a common helper. For kfunc there are also reg->type
PTR_TO_SOCK etc., for them fixed offset should be rejected. So we can either
have a common helper like this for both kfunc and BPF helpers, or exposing
fixed_off_ok parameter in check_ptr_off_reg. Your wish.

> The patch seems to be doing several things at once. Please split.

--
Kartikeya

Re: [PATCH bpf v1 0/5] More fixes for crashes due to bad PTR_TO_BTF_ID reg->off

[Kumar Kartikeya Dwivedi]

On Sun, Feb 20, 2022 at 07:48:08AM IST, Alexei Starovoitov wrote:
> On Sat, Feb 19, 2022 at 05:40:35PM +0530, Kumar Kartikeya Dwivedi wrote:
> > On Sat, Feb 19, 2022 at 05:07:39PM IST, Kumar Kartikeya Dwivedi wrote:
> > > A few more fixes for bad PTR_TO_BTF_ID reg->off being accepted in places, that
> > > can lead to the kernel crashing. Noticed while making sure my own series for BTF
> > > ID pointer in map won't allow stores for pointers with incorrect offsets.
> > >
> > > I include one example where d_path can crash even if you NULL check
> > > PTR_TO_BTF_ID, and one example of how missing NULL check in helper taking
> > > PTR_TO_BTF_ID (like bpf_sock_from_file) is a real problem, see the selftest
> > > patch.
> > >
> > > The &f->f_path becomes NULL + offset in case f is NULL, circumventing NULL
> > > checks in existing helpers. The only thing needed to trigger this finding an
> > > object that embeds the object of interest, and then somehow obtaining a NULL
> > > PTR_TO_BTF_ID to it (not hard, esp. due to exception handler for PROBE_MEM loads
> > > writing 0 to destination register).
> > >
> > > However, for the case of patch 2, it is allowed in my series since the next load
> > > of the bad pointer stored using:
> > >   struct file *f = ...; // some pointer walking returning NULL pointer
> > >   map_val->ptr = &f->f_path; // ptr being struct path *
> > > ... would be marked as PTR_UNTRUSTED, so it won't be allowed to be passed into
> > > the kernel, and hence can be permitted. In referenced case, the PTR_TO_BTF_ID
> > > should not be NULL anyway. kptr_get style helper takes PTR_TO_MAP_VALUE in
> > > referenced ptr case only, so the load either yields NULL or RCU protected
> > > pointer.
> > >
> > > Tests for patch 1 depend on fixup_kfunc_btf_id in test_verifier, hence will be
> > > sent after merge window opens, some other changes after bpf tree merges into
> > > bpf-next, but all pending ones can be seen here [0]. Tests for patch 2 are
> > > included, and try to trigger crash without the fix, but it's not 100% reliable.
> > > We may need special testing helpers or kfuncs to make it thorough, but wanted to
> > > wait before getting feedback.
> > >
> > > Issue fixed by patch 2 is a bit more broader in scope, and would require proper
> > > discussion (before being applied) on the correct way forward, as it is
> > > technically backwards incompatible change, but hopefully never breaks real
> > > programs, only malicious or already incorrect ones.
> > >
> > > Also, please suggest the right "Fixes" tag for patch 2.
> > >
> > > As for patch 3 (selftest), please suggest a better way to get a certain type of
> > > PTR_TO_BTF_ID which can be NULL or NULL+offset. Can we add kfuncs for testing
> > > that return such pointers and make them available to e.g. TC progs, if the fix
> > > in patch 2 is acceptable?
> > >
> > >   [0]: https://github.com/kkdwivedi/linux/commits/fixes-bpf-next
> > >
> >
> > Looking at BPF CI [1], it seems it surfaces another problem I was seeing locally
> > but couldn't craft a reliable test case for, that it forms a non-NULL but
> > invalid pointer using pointer walking, in some cases RCU read lock provides
> > protection for those cases, but not all (esp. if kernel doesn't clear the old
> > pointer that was in use before, and has it sitting in some location). RDI (arg1)
> > seems to be pointing somewhere behind the faulting address, which means the
> > &f->f_path is bad.
> >
> > But this requires a larger discussion.
> >
> > In that case, PAGE_SIZE thing won't help. We may have to introduce a PTR_BPF_REF
> > flag (e.g. set for ctx args of tracing progs, set based on BTF tagging in other
> > places) which tells the verifier that the pointer for the duration of program
> > will be valid, so it can be passed into helpers.
> >
> > So for cases like &f->f_path it will work, since file + off will still have
> > PTR_BPF_REF set in reg state. In case of pointer walking, where dst_reg state
> > is updated on walk, we may have to explicitly tag members where PTR_BPF_REF can
> > be inherited if parent object has PTR_BPF_REF (i.e. ref to parent implies ref to
> > child cases).
> >
> > Something like:
> >
> > struct foo {
> > 	...
> > 	struct bar __bpf_ref *p;
> > 	struct baz *q;
> > 	...
> > }
> >
> > ... then if getting:
> >
> > 	struct foo *f = ...; // PTR_TO_BTF_ID | PTR_BPF_REF
> > 	struct bar *p = f->p; // Inherits PTR_BPF_REF
> > 	struct baz *q = f->q; // Does not inherit PTR_BPF_REF
> >
> > Thoughts?
> >
> >   [1]: https://github.com/kernel-patches/bpf/runs/5258413028?check_suite_focus=true
>
> fd_array wasn't zero initialized at alloc time, so it contains garbage.
> fd_array[63] read that garbage.
> So patches 2 and 3 don't help.
> The 'fixes' list for patch 3 is ridiculous. No need.
> Pls drop patch 2 and in instead of
> +#define bpf_ptr_is_invalid(p) (unlikely((unsigned long)(p) < PAGE_SIZE))
> do static inline function that checks
> if ((unsigned long)p < user_addr_max())

This prevents this specific case, but what happens when PTR_TO_BTF_ID can be
formed to an already freed object (which seems even more likely to me in
sleepable progs, because typical RCU grace period won't wait for us)? Or even
just reading from structures which don't clear pointers they have freed, but are
themselves not freed (so exception handling zeroing dst reg won't kick in).

> and bails out.
> bpf-next is fine.
> Not sure whether patch 1 is strictly necessary after above change.

It is still needed to prevent the var_off case. See [0]. I think it allows you
to pass PTR_TO_BTF_ID while not actually pointing to the actual object, as long
as reg->off is 0 (so that btf_struct_ids_match doesn't fail).

  [0]: https://github.com/kkdwivedi/linux/commit/51981fd301ff55edb72b6cf346c7ac302b3f1d7d#diff-98f6e99b9859bb0525d345f6b962f028d50a605b2397518ddd77b134b5a98977R124
--
Kartikeya

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

[Alexei Starovoitov]

On Sat, Feb 19, 2022 at 6:49 PM Kumar Kartikeya Dwivedi
<memxor@gmail.com> wrote:
>
> On Sun, Feb 20, 2022 at 07:54:09AM IST, Alexei Starovoitov wrote:
> > On Sat, Feb 19, 2022 at 05:07:40PM +0530, Kumar Kartikeya Dwivedi wrote:
> > >
> > > +/* Caller ensures reg->type does not have PTR_MAYBE_NULL */
> > > +int check_func_arg_reg_off(struct bpf_verifier_env *env,
> > > +                      const struct bpf_reg_state *reg, int regno,
> > > +                      bool arg_alloc_mem)
> > > +{
> > > +   enum bpf_reg_type type = reg->type;
> > > +   int err;
> > > +
> > > +   WARN_ON_ONCE(type & PTR_MAYBE_NULL);
> >
> > So the warn was added and made things more difficult and check had to be moved
> > into check_mem_reg to clear that flag?
> > Why add that warn in the first place then?
> > The logic get convoluted because of that.
> >
>
> Ok, will drop.
>
> > > +   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;
> > > +   }
> >
> > Out of the whole patch this part is useful. The rest seems to dealing
> > with self inflicted pain.
> > Just call check_ptr_off_reg() for kfunc ?
>
> I still think we should call a common helper.

What is the point of "common helper" when types
with explicit allow of reg offset like PTR_TO_PACKET cannot
be passed into kfuncs?
A common helper would mislead the reader that such checks are necessary.

>  For kfunc there are also reg->type
> PTR_TO_SOCK etc., for them fixed offset should be rejected. So we can either
> have a common helper like this for both kfunc and BPF helpers, or exposing
> fixed_off_ok parameter in check_ptr_off_reg. Your wish.

Are you saying that we should allow PTR_TO_SOCKET+fixed_off ?
I guess than it's better to convert this line
                err = __check_ptr_off_reg(env, reg, regno,
                                          type == PTR_TO_BTF_ID);
into a helper.
And convert this line:
reg->type == PTR_TO_BTF_ID ||
   (reg2btf_ids[base_type(reg->type)] && !type_flag(reg->type))

into another helper.
Like:
static inline bool is_ptr_to_btf_id(type)
{
  return type == PTR_TO_BTF_ID ||
   (reg2btf_ids[base_type(type)] && !type_flag(type));
}
and
int check_ptr_off_reg(struct bpf_verifier_env *env,
                      const struct bpf_reg_state *reg, int regno)
{
  return __check_ptr_off_reg(env, reg, regno, is_ptr_to_btf_id(reg->type));
}

and call check_ptr_off_reg() directly from check_func_arg()
instead of __check_ptr_off_reg.

and call check_ptr_off_reg() from btf_check_func_arg_match() too.

Re: [PATCH bpf v1 0/5] More fixes for crashes due to bad PTR_TO_BTF_ID reg->off

[Alexei Starovoitov]

On Sat, Feb 19, 2022 at 6:59 PM Kumar Kartikeya Dwivedi
<memxor@gmail.com> wrote:
>
> On Sun, Feb 20, 2022 at 07:48:08AM IST, Alexei Starovoitov wrote:
> > On Sat, Feb 19, 2022 at 05:40:35PM +0530, Kumar Kartikeya Dwivedi wrote:
> > > On Sat, Feb 19, 2022 at 05:07:39PM IST, Kumar Kartikeya Dwivedi wrote:
> > > > A few more fixes for bad PTR_TO_BTF_ID reg->off being accepted in places, that
> > > > can lead to the kernel crashing. Noticed while making sure my own series for BTF
> > > > ID pointer in map won't allow stores for pointers with incorrect offsets.
> > > >
> > > > I include one example where d_path can crash even if you NULL check
> > > > PTR_TO_BTF_ID, and one example of how missing NULL check in helper taking
> > > > PTR_TO_BTF_ID (like bpf_sock_from_file) is a real problem, see the selftest
> > > > patch.
> > > >
> > > > The &f->f_path becomes NULL + offset in case f is NULL, circumventing NULL
> > > > checks in existing helpers. The only thing needed to trigger this finding an
> > > > object that embeds the object of interest, and then somehow obtaining a NULL
> > > > PTR_TO_BTF_ID to it (not hard, esp. due to exception handler for PROBE_MEM loads
> > > > writing 0 to destination register).
> > > >
> > > > However, for the case of patch 2, it is allowed in my series since the next load
> > > > of the bad pointer stored using:
> > > >   struct file *f = ...; // some pointer walking returning NULL pointer
> > > >   map_val->ptr = &f->f_path; // ptr being struct path *
> > > > ... would be marked as PTR_UNTRUSTED, so it won't be allowed to be passed into
> > > > the kernel, and hence can be permitted. In referenced case, the PTR_TO_BTF_ID
> > > > should not be NULL anyway. kptr_get style helper takes PTR_TO_MAP_VALUE in
> > > > referenced ptr case only, so the load either yields NULL or RCU protected
> > > > pointer.
> > > >
> > > > Tests for patch 1 depend on fixup_kfunc_btf_id in test_verifier, hence will be
> > > > sent after merge window opens, some other changes after bpf tree merges into
> > > > bpf-next, but all pending ones can be seen here [0]. Tests for patch 2 are
> > > > included, and try to trigger crash without the fix, but it's not 100% reliable.
> > > > We may need special testing helpers or kfuncs to make it thorough, but wanted to
> > > > wait before getting feedback.
> > > >
> > > > Issue fixed by patch 2 is a bit more broader in scope, and would require proper
> > > > discussion (before being applied) on the correct way forward, as it is
> > > > technically backwards incompatible change, but hopefully never breaks real
> > > > programs, only malicious or already incorrect ones.
> > > >
> > > > Also, please suggest the right "Fixes" tag for patch 2.
> > > >
> > > > As for patch 3 (selftest), please suggest a better way to get a certain type of
> > > > PTR_TO_BTF_ID which can be NULL or NULL+offset. Can we add kfuncs for testing
> > > > that return such pointers and make them available to e.g. TC progs, if the fix
> > > > in patch 2 is acceptable?
> > > >
> > > >   [0]: https://github.com/kkdwivedi/linux/commits/fixes-bpf-next
> > > >
> > >
> > > Looking at BPF CI [1], it seems it surfaces another problem I was seeing locally
> > > but couldn't craft a reliable test case for, that it forms a non-NULL but
> > > invalid pointer using pointer walking, in some cases RCU read lock provides
> > > protection for those cases, but not all (esp. if kernel doesn't clear the old
> > > pointer that was in use before, and has it sitting in some location). RDI (arg1)
> > > seems to be pointing somewhere behind the faulting address, which means the
> > > &f->f_path is bad.
> > >
> > > But this requires a larger discussion.
> > >
> > > In that case, PAGE_SIZE thing won't help. We may have to introduce a PTR_BPF_REF
> > > flag (e.g. set for ctx args of tracing progs, set based on BTF tagging in other
> > > places) which tells the verifier that the pointer for the duration of program
> > > will be valid, so it can be passed into helpers.
> > >
> > > So for cases like &f->f_path it will work, since file + off will still have
> > > PTR_BPF_REF set in reg state. In case of pointer walking, where dst_reg state
> > > is updated on walk, we may have to explicitly tag members where PTR_BPF_REF can
> > > be inherited if parent object has PTR_BPF_REF (i.e. ref to parent implies ref to
> > > child cases).
> > >
> > > Something like:
> > >
> > > struct foo {
> > >     ...
> > >     struct bar __bpf_ref *p;
> > >     struct baz *q;
> > >     ...
> > > }
> > >
> > > ... then if getting:
> > >
> > >     struct foo *f = ...; // PTR_TO_BTF_ID | PTR_BPF_REF
> > >     struct bar *p = f->p; // Inherits PTR_BPF_REF
> > >     struct baz *q = f->q; // Does not inherit PTR_BPF_REF
> > >
> > > Thoughts?
> > >
> > >   [1]: https://github.com/kernel-patches/bpf/runs/5258413028?check_suite_focus=true
> >
> > fd_array wasn't zero initialized at alloc time, so it contains garbage.
> > fd_array[63] read that garbage.
> > So patches 2 and 3 don't help.
> > The 'fixes' list for patch 3 is ridiculous. No need.
> > Pls drop patch 2 and in instead of
> > +#define bpf_ptr_is_invalid(p) (unlikely((unsigned long)(p) < PAGE_SIZE))
> > do static inline function that checks
> > if ((unsigned long)p < user_addr_max())
>
> This prevents this specific case, but what happens when PTR_TO_BTF_ID can be
> formed to an already freed object (which seems even more likely to me in
> sleepable progs, because typical RCU grace period won't wait for us)? Or even
> just reading from structures which don't clear pointers they have freed, but are
> themselves not freed (so exception handling zeroing dst reg won't kick in).

Right. The above check would prevent a class of issues, but not all of them.
If kernel code doesn't clear the pointer inside a struct after freeing it
it would lead to uaf.
Thankfully most of the code is rcu and typical rcu protected pointer
usage would replace the pointer before doing call_rcu() to clean it.
There are corner cases and we need to gradually close such holes.
My point is that it's not a bpf tree material. No need to
create panic and do a patch with gadzillion 'fixes' tags.
With bpf progs calling kfuncs there is a possibility of a crash.
We need to eliminate such possibilities when we can.
But we're not going to backport hundred patches to old kernels.
We will close one hole today and a year later another hole could be found.
We'll close it too, but we are not going to backport a year worth of
verifier patches.

> > and bails out.
> > bpf-next is fine.
> > Not sure whether patch 1 is strictly necessary after above change.
>
> It is still needed to prevent the var_off case.

Right. var_off should be prevented.

Re: [PATCH bpf v1 0/5] More fixes for crashes due to bad PTR_TO_BTF_ID reg->off

[Kumar Kartikeya Dwivedi]

On Tue, Feb 22, 2022 at 02:15:37AM IST, Alexei Starovoitov wrote:
> On Sat, Feb 19, 2022 at 6:59 PM Kumar Kartikeya Dwivedi
> <memxor@gmail.com> wrote:
> >
> > On Sun, Feb 20, 2022 at 07:48:08AM IST, Alexei Starovoitov wrote:
> > > On Sat, Feb 19, 2022 at 05:40:35PM +0530, Kumar Kartikeya Dwivedi wrote:
> > > > On Sat, Feb 19, 2022 at 05:07:39PM IST, Kumar Kartikeya Dwivedi wrote:
> > > > > A few more fixes for bad PTR_TO_BTF_ID reg->off being accepted in places, that
> > > > > can lead to the kernel crashing. Noticed while making sure my own series for BTF
> > > > > ID pointer in map won't allow stores for pointers with incorrect offsets.
> > > > >
> > > > > I include one example where d_path can crash even if you NULL check
> > > > > PTR_TO_BTF_ID, and one example of how missing NULL check in helper taking
> > > > > PTR_TO_BTF_ID (like bpf_sock_from_file) is a real problem, see the selftest
> > > > > patch.
> > > > >
> > > > > The &f->f_path becomes NULL + offset in case f is NULL, circumventing NULL
> > > > > checks in existing helpers. The only thing needed to trigger this finding an
> > > > > object that embeds the object of interest, and then somehow obtaining a NULL
> > > > > PTR_TO_BTF_ID to it (not hard, esp. due to exception handler for PROBE_MEM loads
> > > > > writing 0 to destination register).
> > > > >
> > > > > However, for the case of patch 2, it is allowed in my series since the next load
> > > > > of the bad pointer stored using:
> > > > >   struct file *f = ...; // some pointer walking returning NULL pointer
> > > > >   map_val->ptr = &f->f_path; // ptr being struct path *
> > > > > ... would be marked as PTR_UNTRUSTED, so it won't be allowed to be passed into
> > > > > the kernel, and hence can be permitted. In referenced case, the PTR_TO_BTF_ID
> > > > > should not be NULL anyway. kptr_get style helper takes PTR_TO_MAP_VALUE in
> > > > > referenced ptr case only, so the load either yields NULL or RCU protected
> > > > > pointer.
> > > > >
> > > > > Tests for patch 1 depend on fixup_kfunc_btf_id in test_verifier, hence will be
> > > > > sent after merge window opens, some other changes after bpf tree merges into
> > > > > bpf-next, but all pending ones can be seen here [0]. Tests for patch 2 are
> > > > > included, and try to trigger crash without the fix, but it's not 100% reliable.
> > > > > We may need special testing helpers or kfuncs to make it thorough, but wanted to
> > > > > wait before getting feedback.
> > > > >
> > > > > Issue fixed by patch 2 is a bit more broader in scope, and would require proper
> > > > > discussion (before being applied) on the correct way forward, as it is
> > > > > technically backwards incompatible change, but hopefully never breaks real
> > > > > programs, only malicious or already incorrect ones.
> > > > >
> > > > > Also, please suggest the right "Fixes" tag for patch 2.
> > > > >
> > > > > As for patch 3 (selftest), please suggest a better way to get a certain type of
> > > > > PTR_TO_BTF_ID which can be NULL or NULL+offset. Can we add kfuncs for testing
> > > > > that return such pointers and make them available to e.g. TC progs, if the fix
> > > > > in patch 2 is acceptable?
> > > > >
> > > > >   [0]: https://github.com/kkdwivedi/linux/commits/fixes-bpf-next
> > > > >
> > > >
> > > > Looking at BPF CI [1], it seems it surfaces another problem I was seeing locally
> > > > but couldn't craft a reliable test case for, that it forms a non-NULL but
> > > > invalid pointer using pointer walking, in some cases RCU read lock provides
> > > > protection for those cases, but not all (esp. if kernel doesn't clear the old
> > > > pointer that was in use before, and has it sitting in some location). RDI (arg1)
> > > > seems to be pointing somewhere behind the faulting address, which means the
> > > > &f->f_path is bad.
> > > >
> > > > But this requires a larger discussion.
> > > >
> > > > In that case, PAGE_SIZE thing won't help. We may have to introduce a PTR_BPF_REF
> > > > flag (e.g. set for ctx args of tracing progs, set based on BTF tagging in other
> > > > places) which tells the verifier that the pointer for the duration of program
> > > > will be valid, so it can be passed into helpers.
> > > >
> > > > So for cases like &f->f_path it will work, since file + off will still have
> > > > PTR_BPF_REF set in reg state. In case of pointer walking, where dst_reg state
> > > > is updated on walk, we may have to explicitly tag members where PTR_BPF_REF can
> > > > be inherited if parent object has PTR_BPF_REF (i.e. ref to parent implies ref to
> > > > child cases).
> > > >
> > > > Something like:
> > > >
> > > > struct foo {
> > > >     ...
> > > >     struct bar __bpf_ref *p;
> > > >     struct baz *q;
> > > >     ...
> > > > }
> > > >
> > > > ... then if getting:
> > > >
> > > >     struct foo *f = ...; // PTR_TO_BTF_ID | PTR_BPF_REF
> > > >     struct bar *p = f->p; // Inherits PTR_BPF_REF
> > > >     struct baz *q = f->q; // Does not inherit PTR_BPF_REF
> > > >
> > > > Thoughts?
> > > >
> > > >   [1]: https://github.com/kernel-patches/bpf/runs/5258413028?check_suite_focus=true
> > >
> > > fd_array wasn't zero initialized at alloc time, so it contains garbage.
> > > fd_array[63] read that garbage.
> > > So patches 2 and 3 don't help.
> > > The 'fixes' list for patch 3 is ridiculous. No need.
> > > Pls drop patch 2 and in instead of
> > > +#define bpf_ptr_is_invalid(p) (unlikely((unsigned long)(p) < PAGE_SIZE))
> > > do static inline function that checks
> > > if ((unsigned long)p < user_addr_max())
> >
> > This prevents this specific case, but what happens when PTR_TO_BTF_ID can be
> > formed to an already freed object (which seems even more likely to me in
> > sleepable progs, because typical RCU grace period won't wait for us)? Or even
> > just reading from structures which don't clear pointers they have freed, but are
> > themselves not freed (so exception handling zeroing dst reg won't kick in).
>
> Right. The above check would prevent a class of issues, but not all of them.
> If kernel code doesn't clear the pointer inside a struct after freeing it
> it would lead to uaf.
> Thankfully most of the code is rcu and typical rcu protected pointer
> usage would replace the pointer before doing call_rcu() to clean it.
> There are corner cases and we need to gradually close such holes.
> My point is that it's not a bpf tree material. No need to
> create panic and do a patch with gadzillion 'fixes' tags.

Ok, I'll drop them, but some stable helpers did miss checking NULL too, e.g.
bpf_sock_from_file.

> With bpf progs calling kfuncs there is a possibility of a crash.

This is not just limited to kfuncs.

Apart from BPF helpers, just as an example, I was looking at bpf_inode_storage
last week, that looks suspect to me now; after it has been enabled in sleepable
progs, __destroy_inode etc. no longer wait for the rcu_read_unlock_trace. So
many more cases where inode pointer is obtained by following pointer chains
(e.g. we can just consider fd_array case, where valid file is closed after we
obtain file->f_inode) which would be prevented due to RCU before, would now be
problematic.

> We need to eliminate such possibilities when we can.
> But we're not going to backport hundred patches to old kernels.
> We will close one hole today and a year later another hole could be found.
> We'll close it too, but we are not going to backport a year worth of
> verifier patches.

Ok, understood.

I will target bpf-next going forward, but since you agree this needs to be
fixed, it seems to me we need to bring in a hammer and mark all PTR_TO_BTF_ID
not obtained from known 'safe' contexts as e.g. PTR_UNTRUSTED, so that they can
only be dereferenced. But we can clear the untrusted flag for cases where we
know it is guaranteed to be a valid pointer, e.g. say LSM args, atleast for the
duration of the program lifetime.

Martin also floated a similar idea when sleepable local storage was being
discussed [0] (the paragraph describing PTR_TO_RDONLY_BTF_ID).

We also need to handle cases where pointer walking is used, to decide whether to
set PTR_UNTRUSTED or not. I guess we can use BTF tagging here to annotate
members and say 'if valid pointer to parent exists, valid pointer to this object
can be formed too', but not all cases are as simple.

WDYT?

  [0]: https://lore.kernel.org/bpf/20210901063217.5zpvnltvfmctrkum@kafai-mbp.dhcp.thefacebook.com

>
> > > and bails out.
> > > bpf-next is fine.
> > > Not sure whether patch 1 is strictly necessary after above change.
> >
> > It is still needed to prevent the var_off case.
>
> Right. var_off should be prevented.

--
Kartikeya

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

[Kumar Kartikeya Dwivedi]

On Tue, Feb 22, 2022 at 02:06:15AM IST, Alexei Starovoitov wrote:
> On Sat, Feb 19, 2022 at 6:49 PM Kumar Kartikeya Dwivedi
> <memxor@gmail.com> wrote:
> >
> > On Sun, Feb 20, 2022 at 07:54:09AM IST, Alexei Starovoitov wrote:
> > > On Sat, Feb 19, 2022 at 05:07:40PM +0530, Kumar Kartikeya Dwivedi wrote:
> > > >
> > > > +/* Caller ensures reg->type does not have PTR_MAYBE_NULL */
> > > > +int check_func_arg_reg_off(struct bpf_verifier_env *env,
> > > > +                      const struct bpf_reg_state *reg, int regno,
> > > > +                      bool arg_alloc_mem)
> > > > +{
> > > > +   enum bpf_reg_type type = reg->type;
> > > > +   int err;
> > > > +
> > > > +   WARN_ON_ONCE(type & PTR_MAYBE_NULL);
> > >
> > > So the warn was added and made things more difficult and check had to be moved
> > > into check_mem_reg to clear that flag?
> > > Why add that warn in the first place then?
> > > The logic get convoluted because of that.
> > >
> >
> > Ok, will drop.
> >
> > > > +   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;
> > > > +   }
> > >
> > > Out of the whole patch this part is useful. The rest seems to dealing
> > > with self inflicted pain.
> > > Just call check_ptr_off_reg() for kfunc ?
> >
> > I still think we should call a common helper.
>
> What is the point of "common helper" when types
> with explicit allow of reg offset like PTR_TO_PACKET cannot
> be passed into kfuncs?
> A common helper would mislead the reader that such checks are necessary.
>

PTR_TO_PACKET is certainly allowed to be passed to kfunc, and not just that,
PTR_TO_STACK, PTR_TO_BUF, PTR_TO_MEM, PTR_TO_MAP_VALUE, PTR_TO_MAP_KEY, all are
permited after we set ptr_to_mem_ok = true for kfunc. And these can have fixed
off and sometimes var_off to be set. They are also allowed for global functions.

Which is why I thought having a single list in the entire verifier would be more
easier to maintain, then we can update it in one place and ensure both BPF
helpers and kfunc are covered by the same checks and expectations for fixed and
variable offsets. It isn't 'misleading' because all those types are also
permitted for kfuncs.

> >  For kfunc there are also reg->type
> > PTR_TO_SOCK etc., for them fixed offset should be rejected. So we can either
> > have a common helper like this for both kfunc and BPF helpers, or exposing
> > fixed_off_ok parameter in check_ptr_off_reg. Your wish.
>
> Are you saying that we should allow PTR_TO_SOCKET+fixed_off ?

No, I said we need to allow fixed off for PTR_TO_BTF_ID, but also prevent
var_off for it, but just using check_ptr_off_reg would not help because it
prevents fixed_off, and using __check_ptr_off_reg with fixed_off_ok == true
would be wrong for PTR_TO_SOCKET etc. Hence some refactoring is needed.

And using check_ptr_off_reg ultimately (through the common check or directly)
also rejects var_off for PTR_TO_BTF_ID, which was the actual problem that
started this whole patch.

> I guess than it's better to convert this line
>                 err = __check_ptr_off_reg(env, reg, regno,
>                                           type == PTR_TO_BTF_ID);
> into a helper.
> And convert this line:
> reg->type == PTR_TO_BTF_ID ||
>    (reg2btf_ids[base_type(reg->type)] && !type_flag(reg->type))
>
> into another helper.
> Like:
> static inline bool is_ptr_to_btf_id(type)
> {
>   return type == PTR_TO_BTF_ID ||
>    (reg2btf_ids[base_type(type)] && !type_flag(type));
> }
> and
> int check_ptr_off_reg(struct bpf_verifier_env *env,
>                       const struct bpf_reg_state *reg, int regno)
> {
>   return __check_ptr_off_reg(env, reg, regno, is_ptr_to_btf_id(reg->type));
> }
>
> and call check_ptr_off_reg() directly from check_func_arg()
> instead of __check_ptr_off_reg.
>
> and call check_ptr_off_reg() from btf_check_func_arg_match() too.

--
Kartikeya

Re: [PATCH bpf v1 0/5] More fixes for crashes due to bad PTR_TO_BTF_ID reg->off

[Alexei Starovoitov]

On Mon, Feb 21, 2022 at 7:21 PM Kumar Kartikeya Dwivedi
<memxor@gmail.com> wrote:
>
> On Tue, Feb 22, 2022 at 02:15:37AM IST, Alexei Starovoitov wrote:
> > On Sat, Feb 19, 2022 at 6:59 PM Kumar Kartikeya Dwivedi
> > <memxor@gmail.com> wrote:
> > >
> > > On Sun, Feb 20, 2022 at 07:48:08AM IST, Alexei Starovoitov wrote:
> > > > On Sat, Feb 19, 2022 at 05:40:35PM +0530, Kumar Kartikeya Dwivedi wrote:
> > > > > On Sat, Feb 19, 2022 at 05:07:39PM IST, Kumar Kartikeya Dwivedi wrote:
> > > > > > A few more fixes for bad PTR_TO_BTF_ID reg->off being accepted in places, that
> > > > > > can lead to the kernel crashing. Noticed while making sure my own series for BTF
> > > > > > ID pointer in map won't allow stores for pointers with incorrect offsets.
> > > > > >
> > > > > > I include one example where d_path can crash even if you NULL check
> > > > > > PTR_TO_BTF_ID, and one example of how missing NULL check in helper taking
> > > > > > PTR_TO_BTF_ID (like bpf_sock_from_file) is a real problem, see the selftest
> > > > > > patch.
> > > > > >
> > > > > > The &f->f_path becomes NULL + offset in case f is NULL, circumventing NULL
> > > > > > checks in existing helpers. The only thing needed to trigger this finding an
> > > > > > object that embeds the object of interest, and then somehow obtaining a NULL
> > > > > > PTR_TO_BTF_ID to it (not hard, esp. due to exception handler for PROBE_MEM loads
> > > > > > writing 0 to destination register).
> > > > > >
> > > > > > However, for the case of patch 2, it is allowed in my series since the next load
> > > > > > of the bad pointer stored using:
> > > > > >   struct file *f = ...; // some pointer walking returning NULL pointer
> > > > > >   map_val->ptr = &f->f_path; // ptr being struct path *
> > > > > > ... would be marked as PTR_UNTRUSTED, so it won't be allowed to be passed into
> > > > > > the kernel, and hence can be permitted. In referenced case, the PTR_TO_BTF_ID
> > > > > > should not be NULL anyway. kptr_get style helper takes PTR_TO_MAP_VALUE in
> > > > > > referenced ptr case only, so the load either yields NULL or RCU protected
> > > > > > pointer.
> > > > > >
> > > > > > Tests for patch 1 depend on fixup_kfunc_btf_id in test_verifier, hence will be
> > > > > > sent after merge window opens, some other changes after bpf tree merges into
> > > > > > bpf-next, but all pending ones can be seen here [0]. Tests for patch 2 are
> > > > > > included, and try to trigger crash without the fix, but it's not 100% reliable.
> > > > > > We may need special testing helpers or kfuncs to make it thorough, but wanted to
> > > > > > wait before getting feedback.
> > > > > >
> > > > > > Issue fixed by patch 2 is a bit more broader in scope, and would require proper
> > > > > > discussion (before being applied) on the correct way forward, as it is
> > > > > > technically backwards incompatible change, but hopefully never breaks real
> > > > > > programs, only malicious or already incorrect ones.
> > > > > >
> > > > > > Also, please suggest the right "Fixes" tag for patch 2.
> > > > > >
> > > > > > As for patch 3 (selftest), please suggest a better way to get a certain type of
> > > > > > PTR_TO_BTF_ID which can be NULL or NULL+offset. Can we add kfuncs for testing
> > > > > > that return such pointers and make them available to e.g. TC progs, if the fix
> > > > > > in patch 2 is acceptable?
> > > > > >
> > > > > >   [0]: https://github.com/kkdwivedi/linux/commits/fixes-bpf-next
> > > > > >
> > > > >
> > > > > Looking at BPF CI [1], it seems it surfaces another problem I was seeing locally
> > > > > but couldn't craft a reliable test case for, that it forms a non-NULL but
> > > > > invalid pointer using pointer walking, in some cases RCU read lock provides
> > > > > protection for those cases, but not all (esp. if kernel doesn't clear the old
> > > > > pointer that was in use before, and has it sitting in some location). RDI (arg1)
> > > > > seems to be pointing somewhere behind the faulting address, which means the
> > > > > &f->f_path is bad.
> > > > >
> > > > > But this requires a larger discussion.
> > > > >
> > > > > In that case, PAGE_SIZE thing won't help. We may have to introduce a PTR_BPF_REF
> > > > > flag (e.g. set for ctx args of tracing progs, set based on BTF tagging in other
> > > > > places) which tells the verifier that the pointer for the duration of program
> > > > > will be valid, so it can be passed into helpers.
> > > > >
> > > > > So for cases like &f->f_path it will work, since file + off will still have
> > > > > PTR_BPF_REF set in reg state. In case of pointer walking, where dst_reg state
> > > > > is updated on walk, we may have to explicitly tag members where PTR_BPF_REF can
> > > > > be inherited if parent object has PTR_BPF_REF (i.e. ref to parent implies ref to
> > > > > child cases).
> > > > >
> > > > > Something like:
> > > > >
> > > > > struct foo {
> > > > >     ...
> > > > >     struct bar __bpf_ref *p;
> > > > >     struct baz *q;
> > > > >     ...
> > > > > }
> > > > >
> > > > > ... then if getting:
> > > > >
> > > > >     struct foo *f = ...; // PTR_TO_BTF_ID | PTR_BPF_REF
> > > > >     struct bar *p = f->p; // Inherits PTR_BPF_REF
> > > > >     struct baz *q = f->q; // Does not inherit PTR_BPF_REF
> > > > >
> > > > > Thoughts?
> > > > >
> > > > >   [1]: https://github.com/kernel-patches/bpf/runs/5258413028?check_suite_focus=true
> > > >
> > > > fd_array wasn't zero initialized at alloc time, so it contains garbage.
> > > > fd_array[63] read that garbage.
> > > > So patches 2 and 3 don't help.
> > > > The 'fixes' list for patch 3 is ridiculous. No need.
> > > > Pls drop patch 2 and in instead of
> > > > +#define bpf_ptr_is_invalid(p) (unlikely((unsigned long)(p) < PAGE_SIZE))
> > > > do static inline function that checks
> > > > if ((unsigned long)p < user_addr_max())
> > >
> > > This prevents this specific case, but what happens when PTR_TO_BTF_ID can be
> > > formed to an already freed object (which seems even more likely to me in
> > > sleepable progs, because typical RCU grace period won't wait for us)? Or even
> > > just reading from structures which don't clear pointers they have freed, but are
> > > themselves not freed (so exception handling zeroing dst reg won't kick in).
> >
> > Right. The above check would prevent a class of issues, but not all of them.
> > If kernel code doesn't clear the pointer inside a struct after freeing it
> > it would lead to uaf.
> > Thankfully most of the code is rcu and typical rcu protected pointer
> > usage would replace the pointer before doing call_rcu() to clean it.
> > There are corner cases and we need to gradually close such holes.
> > My point is that it's not a bpf tree material. No need to
> > create panic and do a patch with gadzillion 'fixes' tags.
>
> Ok, I'll drop them, but some stable helpers did miss checking NULL too, e.g.
> bpf_sock_from_file.
>
> > With bpf progs calling kfuncs there is a possibility of a crash.
>
> This is not just limited to kfuncs.
>
> Apart from BPF helpers, just as an example, I was looking at bpf_inode_storage
> last week, that looks suspect to me now; after it has been enabled in sleepable
> progs, __destroy_inode etc. no longer wait for the rcu_read_unlock_trace. So
> many more cases where inode pointer is obtained by following pointer chains
> (e.g. we can just consider fd_array case, where valid file is closed after we
> obtain file->f_inode) which would be prevented due to RCU before, would now be
> problematic.
>
> > We need to eliminate such possibilities when we can.
> > But we're not going to backport hundred patches to old kernels.
> > We will close one hole today and a year later another hole could be found.
> > We'll close it too, but we are not going to backport a year worth of
> > verifier patches.
>
> Ok, understood.
>
> I will target bpf-next going forward, but since you agree this needs to be
> fixed, it seems to me we need to bring in a hammer and mark all PTR_TO_BTF_ID
> not obtained from known 'safe' contexts as e.g. PTR_UNTRUSTED, so that they can
> only be dereferenced. But we can clear the untrusted flag for cases where we
> know it is guaranteed to be a valid pointer, e.g. say LSM args, atleast for the
> duration of the program lifetime.

That could be an option if it doesn't break existing progs and
doesn't limit usability of pointer walks.
Let's get to it when PTR_UNTRUSTED lands as part of ptr_to_btf_id in a
map series.

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

[Alexei Starovoitov]

On Mon, Feb 21, 2022 at 7:31 PM Kumar Kartikeya Dwivedi
<memxor@gmail.com> wrote:
>
> On Tue, Feb 22, 2022 at 02:06:15AM IST, Alexei Starovoitov wrote:
> > On Sat, Feb 19, 2022 at 6:49 PM Kumar Kartikeya Dwivedi
> > <memxor@gmail.com> wrote:
> > >
> > > On Sun, Feb 20, 2022 at 07:54:09AM IST, Alexei Starovoitov wrote:
> > > > On Sat, Feb 19, 2022 at 05:07:40PM +0530, Kumar Kartikeya Dwivedi wrote:
> > > > >
> > > > > +/* Caller ensures reg->type does not have PTR_MAYBE_NULL */
> > > > > +int check_func_arg_reg_off(struct bpf_verifier_env *env,
> > > > > +                      const struct bpf_reg_state *reg, int regno,
> > > > > +                      bool arg_alloc_mem)
> > > > > +{
> > > > > +   enum bpf_reg_type type = reg->type;
> > > > > +   int err;
> > > > > +
> > > > > +   WARN_ON_ONCE(type & PTR_MAYBE_NULL);
> > > >
> > > > So the warn was added and made things more difficult and check had to be moved
> > > > into check_mem_reg to clear that flag?
> > > > Why add that warn in the first place then?
> > > > The logic get convoluted because of that.
> > > >
> > >
> > > Ok, will drop.
> > >
> > > > > +   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;
> > > > > +   }
> > > >
> > > > Out of the whole patch this part is useful. The rest seems to dealing
> > > > with self inflicted pain.
> > > > Just call check_ptr_off_reg() for kfunc ?
> > >
> > > I still think we should call a common helper.
> >
> > What is the point of "common helper" when types
> > with explicit allow of reg offset like PTR_TO_PACKET cannot
> > be passed into kfuncs?
> > A common helper would mislead the reader that such checks are necessary.
> >
>
> PTR_TO_PACKET is certainly allowed to be passed to kfunc, and not just that,
> PTR_TO_STACK, PTR_TO_BUF, PTR_TO_MEM, PTR_TO_MAP_VALUE, PTR_TO_MAP_KEY, all are
> permited after we set ptr_to_mem_ok = true for kfunc. And these can have fixed
> off and sometimes var_off to be set. They are also allowed for global functions.

Ahh. Right. The whole check inside check_mem_reg dance confused me.

> Which is why I thought having a single list in the entire verifier would be more
> easier to maintain, then we can update it in one place and ensure both BPF
> helpers and kfunc are covered by the same checks and expectations for fixed and
> variable offsets. It isn't 'misleading' because all those types are also
> permitted for kfuncs.
>
> > >  For kfunc there are also reg->type
> > > PTR_TO_SOCK etc., for them fixed offset should be rejected. So we can either
> > > have a common helper like this for both kfunc and BPF helpers, or exposing
> > > fixed_off_ok parameter in check_ptr_off_reg. Your wish.
> >
> > Are you saying that we should allow PTR_TO_SOCKET+fixed_off ?
>
> No, I said we need to allow fixed off for PTR_TO_BTF_ID, but also prevent
> var_off for it, but just using check_ptr_off_reg would not help because it
> prevents fixed_off, and using __check_ptr_off_reg with fixed_off_ok == true
> would be wrong for PTR_TO_SOCKET etc. Hence some refactoring is needed.
>
> And using check_ptr_off_reg ultimately (through the common check or directly)
> also rejects var_off for PTR_TO_BTF_ID, which was the actual problem that
> started this whole patch.
>
> > I guess than it's better to convert this line
> >                 err = __check_ptr_off_reg(env, reg, regno,
> >                                           type == PTR_TO_BTF_ID);
> > into a helper.
> > And convert this line:
> > reg->type == PTR_TO_BTF_ID ||
> >    (reg2btf_ids[base_type(reg->type)] && !type_flag(reg->type))
> >
> > into another helper.
> > Like:
> > static inline bool is_ptr_to_btf_id(type)
> > {
> >   return type == PTR_TO_BTF_ID ||
> >    (reg2btf_ids[base_type(type)] && !type_flag(type));
> > }
> > and
> > int check_ptr_off_reg(struct bpf_verifier_env *env,
> >                       const struct bpf_reg_state *reg, int regno)
> > {
> >   return __check_ptr_off_reg(env, reg, regno, is_ptr_to_btf_id(reg->type));
> > }
> >
> > and call check_ptr_off_reg() directly from check_func_arg()
> > instead of __check_ptr_off_reg.
> >
> > and call check_ptr_off_reg() from btf_check_func_arg_match() too.

Thoughts about the above proposal?
In addition to above we can have check_func_arg_reg_off()
and call it early and once in btf_check_func_arg_match()
instead of hiding deep in a call chain.
I don't like 'bool arg_alloc_mem' part though
and would like to get rid of 'bool ptr_to_mem_ok' eventually as well.
Such flags make the code harder to follow,
since the action on the flag value is done outside
of the main part of the code.
For example reading btf_check_func_arg_match() on its own is complicated.
The developer has to examine all call sites to see how they pass that flag.
Same thing with 'bool arg_alloc_mem'.
Please pass arg_type instead.

This patch should be split into three.
p1 - refactor into check_func_arg_reg_off helper.
p2 - call it form btf_check_func_arg_match
p3 - add off < 0 check.

If you're adding "while at it" to the commit log it means that
it shouldn't be a single patch.

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

[Kumar Kartikeya Dwivedi]

On Tue, Feb 22, 2022 at 09:51:43AM IST, Alexei Starovoitov wrote:
> On Mon, Feb 21, 2022 at 7:31 PM Kumar Kartikeya Dwivedi
> <memxor@gmail.com> wrote:
> >
> > On Tue, Feb 22, 2022 at 02:06:15AM IST, Alexei Starovoitov wrote:
> > > On Sat, Feb 19, 2022 at 6:49 PM Kumar Kartikeya Dwivedi
> > > <memxor@gmail.com> wrote:
> > > >
> > > > On Sun, Feb 20, 2022 at 07:54:09AM IST, Alexei Starovoitov wrote:
> > > > > On Sat, Feb 19, 2022 at 05:07:40PM +0530, Kumar Kartikeya Dwivedi wrote:
> > > > > >
> > > > > > +/* Caller ensures reg->type does not have PTR_MAYBE_NULL */
> > > > > > +int check_func_arg_reg_off(struct bpf_verifier_env *env,
> > > > > > +                      const struct bpf_reg_state *reg, int regno,
> > > > > > +                      bool arg_alloc_mem)
> > > > > > +{
> > > > > > +   enum bpf_reg_type type = reg->type;
> > > > > > +   int err;
> > > > > > +
> > > > > > +   WARN_ON_ONCE(type & PTR_MAYBE_NULL);
> > > > >
> > > > > So the warn was added and made things more difficult and check had to be moved
> > > > > into check_mem_reg to clear that flag?
> > > > > Why add that warn in the first place then?
> > > > > The logic get convoluted because of that.
> > > > >
> > > >
> > > > Ok, will drop.
> > > >
> > > > > > +   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;
> > > > > > +   }
> > > > >
> > > > > Out of the whole patch this part is useful. The rest seems to dealing
> > > > > with self inflicted pain.
> > > > > Just call check_ptr_off_reg() for kfunc ?
> > > >
> > > > I still think we should call a common helper.
> > >
> > > What is the point of "common helper" when types
> > > with explicit allow of reg offset like PTR_TO_PACKET cannot
> > > be passed into kfuncs?
> > > A common helper would mislead the reader that such checks are necessary.
> > >
> >
> > PTR_TO_PACKET is certainly allowed to be passed to kfunc, and not just that,
> > PTR_TO_STACK, PTR_TO_BUF, PTR_TO_MEM, PTR_TO_MAP_VALUE, PTR_TO_MAP_KEY, all are
> > permited after we set ptr_to_mem_ok = true for kfunc. And these can have fixed
> > off and sometimes var_off to be set. They are also allowed for global functions.
>
> Ahh. Right. The whole check inside check_mem_reg dance confused me.
>
> > Which is why I thought having a single list in the entire verifier would be more
> > easier to maintain, then we can update it in one place and ensure both BPF
> > helpers and kfunc are covered by the same checks and expectations for fixed and
> > variable offsets. It isn't 'misleading' because all those types are also
> > permitted for kfuncs.
> >
> > > >  For kfunc there are also reg->type
> > > > PTR_TO_SOCK etc., for them fixed offset should be rejected. So we can either
> > > > have a common helper like this for both kfunc and BPF helpers, or exposing
> > > > fixed_off_ok parameter in check_ptr_off_reg. Your wish.
> > >
> > > Are you saying that we should allow PTR_TO_SOCKET+fixed_off ?
> >
> > No, I said we need to allow fixed off for PTR_TO_BTF_ID, but also prevent
> > var_off for it, but just using check_ptr_off_reg would not help because it
> > prevents fixed_off, and using __check_ptr_off_reg with fixed_off_ok == true
> > would be wrong for PTR_TO_SOCKET etc. Hence some refactoring is needed.
> >
> > And using check_ptr_off_reg ultimately (through the common check or directly)
> > also rejects var_off for PTR_TO_BTF_ID, which was the actual problem that
> > started this whole patch.
> >
> > > I guess than it's better to convert this line
> > >                 err = __check_ptr_off_reg(env, reg, regno,
> > >                                           type == PTR_TO_BTF_ID);
> > > into a helper.
> > > And convert this line:
> > > reg->type == PTR_TO_BTF_ID ||
> > >    (reg2btf_ids[base_type(reg->type)] && !type_flag(reg->type))
> > >
> > > into another helper.
> > > Like:
> > > static inline bool is_ptr_to_btf_id(type)
> > > {
> > >   return type == PTR_TO_BTF_ID ||
> > >    (reg2btf_ids[base_type(type)] && !type_flag(type));
> > > }
> > > and
> > > int check_ptr_off_reg(struct bpf_verifier_env *env,
> > >                       const struct bpf_reg_state *reg, int regno)
> > > {
> > >   return __check_ptr_off_reg(env, reg, regno, is_ptr_to_btf_id(reg->type));
> > > }
> > >
> > > and call check_ptr_off_reg() directly from check_func_arg()
> > > instead of __check_ptr_off_reg.
> > >
> > > and call check_ptr_off_reg() from btf_check_func_arg_match() too.
>
> Thoughts about the above proposal?
> In addition to above we can have check_func_arg_reg_off()
> and call it early and once in btf_check_func_arg_match()
> instead of hiding deep in a call chain.
> I don't like 'bool arg_alloc_mem' part though
> and would like to get rid of 'bool ptr_to_mem_ok' eventually as well.
> Such flags make the code harder to follow,
> since the action on the flag value is done outside
> of the main part of the code.
> For example reading btf_check_func_arg_match() on its own is complicated.
> The developer has to examine all call sites to see how they pass that flag.
> Same thing with 'bool arg_alloc_mem'.
> Please pass arg_type instead.
>
> This patch should be split into three.
> p1 - refactor into check_func_arg_reg_off helper.
> p2 - call it form btf_check_func_arg_match
> p3 - add off < 0 check.
>
> If you're adding "while at it" to the commit log it means that
> it shouldn't be a single patch.

Agree with everything, will split and respin. Thanks.

--
Kartikeya