| Message ID | 20230330145203.80506-1-void@manifault.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | e4c2acab95a5947fe7948140a83e4f4918c6c048 |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next,1/2] bpf: Handle PTR_MAYBE_NULL case in PTR_TO_BTF_ID helper call arg | expand |
Hello: This series was applied to bpf/bpf-next.git (master) by Andrii Nakryiko <andrii@kernel.org>: On Thu, 30 Mar 2023 09:52:02 -0500 you wrote: > When validating a helper function argument, we use check_reg_type() to > ensure that the register containing the argument is of the correct type. > When the register's base type is PTR_TO_BTF_ID, there is some > supplemental logic where we do extra checks for various combinations of > PTR_TO_BTF_ID type modifiers. For example, for PTR_TO_BTF_ID, > PTR_TO_BTF_ID | PTR_TRUSTED, and PTR_TO_BTF_ID | MEM_RCU, we call > map_kptr_match_type() for bpf_kptr_xchg() calls, and > btf_struct_ids_match() for other helper calls. > > [...] Here is the summary with links: - [bpf-next,1/2] bpf: Handle PTR_MAYBE_NULL case in PTR_TO_BTF_ID helper call arg https://git.kernel.org/bpf/bpf-next/c/e4c2acab95a5 - [bpf-next,2/2] selftests/bpf: Add testcases for ptr_*_or_null_ in bpf_kptr_xchg https://git.kernel.org/bpf/bpf-next/c/67efbd57bc6e You are awesome, thank you!
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 20eb2015842f..52738f9dcb15 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -7204,6 +7204,10 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno, } break; } + case PTR_TO_BTF_ID | PTR_MAYBE_NULL: + case PTR_TO_BTF_ID | PTR_MAYBE_NULL | MEM_RCU: + verbose(env, "Possibly NULL pointer passed to helper arg%d\n", regno); + return -EACCES; case PTR_TO_BTF_ID | MEM_ALLOC: if (meta->func_id != BPF_FUNC_spin_lock && meta->func_id != BPF_FUNC_spin_unlock && meta->func_id != BPF_FUNC_kptr_xchg) {
When validating a helper function argument, we use check_reg_type() to ensure that the register containing the argument is of the correct type. When the register's base type is PTR_TO_BTF_ID, there is some supplemental logic where we do extra checks for various combinations of PTR_TO_BTF_ID type modifiers. For example, for PTR_TO_BTF_ID, PTR_TO_BTF_ID | PTR_TRUSTED, and PTR_TO_BTF_ID | MEM_RCU, we call map_kptr_match_type() for bpf_kptr_xchg() calls, and btf_struct_ids_match() for other helper calls. When an unhandled PTR_TO_BTF_ID type modifier combination is passed to check_reg_type(), the verifier fails with an internal verifier error message. This can currently be triggered by passing a PTR_MAYBE_NULL pointer to helper functions (currently just bpf_kptr_xchg()) with an ARG_PTR_TO_BTF_ID_OR_NULL arg type. For example, by callin bpf_kptr_xchg(&v->kptr, bpf_cpumask_create()). Whether or not passing a PTR_MAYBE_NULL arg to an ARG_PTR_TO_BTF_ID_OR_NULL argument is valid is an interesting question. In a vacuum, it seems fine. A helper function with an ARG_PTR_TO_BTF_ID_OR_NULL arg would seem to be implying that it can handle either a NULL or non-NULL arg, and has logic in place to detect and gracefully handle each. This is the case for bpf_kptr_xchg(), which of course simply does an xchg(). On the other hand, bpf_kptr_xchg() also specifies OBJ_RELEASE, and refcounting semantics for a PTR_MAYBE_NULL pointer is different than handling it for a NULL _OR_ non-NULL pointer. For example, with a non-NULL arg, we should always fail if there was not a nonzero refcount for the value in the register being passed to the helper. For PTR_MAYBE_NULL on the other hand, it's unclear. If the pointer is NULL it would be fine, but if it's not NULL, it would be incorrect to load the program. The current solution to this is to just fail if PTR_MAYBE_NULL is passed, and to instead require programs to have a NULL check to explicitly handle the NULL and non-NULL cases. This seems reasonable. Not only would it possibly be quite complicated to correctly handle PTR_MAYBE_NULL refcounting in the verifier, but it's also an arguably odd programming pattern in general to not explicitly handle the NULL case anyways. For example, it seems odd to not care about whether a pointer you're passing to bpf_kptr_xchg() was successfully allocated in a program such as the following: private(MASK) static struct bpf_cpumask __kptr * global_mask; SEC("tp_btf/task_newtask") int BPF_PROG(example, struct task_struct *task, u64 clone_flags) { struct bpf_cpumask *prev; /* bpf_cpumask_create() returns PTR_MAYBE_NULL */ prev = bpf_kptr_xchg(&global_mask, bpf_cpumask_create()); if (prev) bpf_cpumask_release(prev); return 0; } This patch therefore updates the verifier to explicitly check for PTR_MAYBE_NULL in check_reg_type(), and fail gracefully if it's observed. This isn't really "fixing" anything unsafe or incorrect. We're just updating the verifier to fail gracefully, and explicitly handle this pattern rather than unintentionally falling back to an internal verifier error path. A subsequent patch will update selftests. Signed-off-by: David Vernet <void@manifault.com> --- kernel/bpf/verifier.c | 4 ++++ 1 file changed, 4 insertions(+)