| Message ID | 20220912154544.1398199-1-davemarchevsky@fb.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 47e34cb74d376ddfeaef94abb1d6dfb3c905ee51 |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next] bpf: Add verifier check for BPF_PTR_POISON retval and arg | expand |
On Mon, 12 Sept 2022 at 17:45, Dave Marchevsky <davemarchevsky@fb.com> wrote: > > BPF_PTR_POISON was added in commit c0a5a21c25f37 ("bpf: Allow storing > referenced kptr in map") to denote a bpf_func_proto btf_id which the > verifier will replace with a dynamically-determined btf_id at verification > time. > > This patch adds verifier 'poison' functionality to BPF_PTR_POISON in > order to prepare for expanded use of the value to poison ret- and > arg-btf_id in ongoing work, namely rbtree and linked list patchsets > [0, 1]. Specifically, when the verifier checks helper calls, it assumes > that BPF_PTR_POISON'ed ret type will be replaced with a valid type before > - or in lieu of - the default ret_btf_id logic. Similarly for arg btf_id. > > If poisoned btf_id reaches default handling block for either, consider > this a verifier internal error and fail verification. Otherwise a helper > w/ poisoned btf_id but no verifier logic replacing the type will cause a > crash as the invalid pointer is dereferenced. > > Also move BPF_PTR_POISON to existing include/linux/posion.h header and > remove unnecessary shift. > > [0]: lore.kernel.org/bpf/20220830172759.4069786-1-davemarchevsky@fb.com > [1]: lore.kernel.org/bpf/20220904204145.3089-1-memxor@gmail.com > > Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com> > --- Acked-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
Hello: This patch was applied to bpf/bpf-next.git (master) by Alexei Starovoitov <ast@kernel.org>: On Mon, 12 Sep 2022 08:45:44 -0700 you wrote: > BPF_PTR_POISON was added in commit c0a5a21c25f37 ("bpf: Allow storing > referenced kptr in map") to denote a bpf_func_proto btf_id which the > verifier will replace with a dynamically-determined btf_id at verification > time. > > This patch adds verifier 'poison' functionality to BPF_PTR_POISON in > order to prepare for expanded use of the value to poison ret- and > arg-btf_id in ongoing work, namely rbtree and linked list patchsets > [0, 1]. Specifically, when the verifier checks helper calls, it assumes > that BPF_PTR_POISON'ed ret type will be replaced with a valid type before > - or in lieu of - the default ret_btf_id logic. Similarly for arg btf_id. > > [...] Here is the summary with links: - [bpf-next] bpf: Add verifier check for BPF_PTR_POISON retval and arg https://git.kernel.org/bpf/bpf-next/c/47e34cb74d37 You are awesome, thank you!
diff --git a/include/linux/poison.h b/include/linux/poison.h index d62ef5a6b4e9..2d3249eb0e62 100644 --- a/include/linux/poison.h +++ b/include/linux/poison.h @@ -81,4 +81,7 @@ /********** net/core/page_pool.c **********/ #define PP_SIGNATURE (0x40 + POISON_POINTER_DELTA) +/********** kernel/bpf/ **********/ +#define BPF_PTR_POISON ((void *)(0xeB9FUL + POISON_POINTER_DELTA)) + #endif diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c index fc08035f14ed..41aeaf3862ec 100644 --- a/kernel/bpf/helpers.c +++ b/kernel/bpf/helpers.c @@ -15,6 +15,7 @@ #include <linux/ctype.h> #include <linux/jiffies.h> #include <linux/pid_namespace.h> +#include <linux/poison.h> #include <linux/proc_ns.h> #include <linux/security.h> #include <linux/btf_ids.h> @@ -1376,10 +1377,9 @@ BPF_CALL_2(bpf_kptr_xchg, void *, map_value, void *, ptr) } /* Unlike other PTR_TO_BTF_ID helpers the btf_id in bpf_kptr_xchg() - * helper is determined dynamically by the verifier. + * helper is determined dynamically by the verifier. Use BPF_PTR_POISON to + * denote type that verifier will determine. */ -#define BPF_PTR_POISON ((void *)((0xeB9FUL << 2) + POISON_POINTER_DELTA)) - static const struct bpf_func_proto bpf_kptr_xchg_proto = { .func = bpf_kptr_xchg, .gpl_only = false, diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index c259d734f863..8c6fbcd0afaf 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -23,6 +23,7 @@ #include <linux/error-injection.h> #include <linux/bpf_lsm.h> #include <linux/btf_ids.h> +#include <linux/poison.h> #include "disasm.h" @@ -5782,13 +5783,22 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno, if (meta->func_id == BPF_FUNC_kptr_xchg) { if (map_kptr_match_type(env, meta->kptr_off_desc, reg, regno)) return -EACCES; - } else if (!btf_struct_ids_match(&env->log, reg->btf, reg->btf_id, reg->off, - btf_vmlinux, *arg_btf_id, - strict_type_match)) { - verbose(env, "R%d is of type %s but %s is expected\n", - regno, kernel_type_name(reg->btf, reg->btf_id), - kernel_type_name(btf_vmlinux, *arg_btf_id)); - return -EACCES; + } else { + if (arg_btf_id == BPF_PTR_POISON) { + verbose(env, "verifier internal error:"); + verbose(env, "R%d has non-overwritten BPF_PTR_POISON type\n", + regno); + return -EACCES; + } + + if (!btf_struct_ids_match(&env->log, reg->btf, reg->btf_id, reg->off, + btf_vmlinux, *arg_btf_id, + strict_type_match)) { + verbose(env, "R%d is of type %s but %s is expected\n", + regno, kernel_type_name(reg->btf, reg->btf_id), + kernel_type_name(btf_vmlinux, *arg_btf_id)); + return -EACCES; + } } } @@ -7457,6 +7467,12 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn ret_btf = meta.kptr_off_desc->kptr.btf; ret_btf_id = meta.kptr_off_desc->kptr.btf_id; } else { + if (fn->ret_btf_id == BPF_PTR_POISON) { + verbose(env, "verifier internal error:"); + verbose(env, "func %s has non-overwritten BPF_PTR_POISON return type\n", + func_id_name(func_id)); + return -EINVAL; + } ret_btf = btf_vmlinux; ret_btf_id = *fn->ret_btf_id; }
BPF_PTR_POISON was added in commit c0a5a21c25f37 ("bpf: Allow storing referenced kptr in map") to denote a bpf_func_proto btf_id which the verifier will replace with a dynamically-determined btf_id at verification time. This patch adds verifier 'poison' functionality to BPF_PTR_POISON in order to prepare for expanded use of the value to poison ret- and arg-btf_id in ongoing work, namely rbtree and linked list patchsets [0, 1]. Specifically, when the verifier checks helper calls, it assumes that BPF_PTR_POISON'ed ret type will be replaced with a valid type before - or in lieu of - the default ret_btf_id logic. Similarly for arg btf_id. If poisoned btf_id reaches default handling block for either, consider this a verifier internal error and fail verification. Otherwise a helper w/ poisoned btf_id but no verifier logic replacing the type will cause a crash as the invalid pointer is dereferenced. Also move BPF_PTR_POISON to existing include/linux/posion.h header and remove unnecessary shift. [0]: lore.kernel.org/bpf/20220830172759.4069786-1-davemarchevsky@fb.com [1]: lore.kernel.org/bpf/20220904204145.3089-1-memxor@gmail.com Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com> --- This patch was originally part of [0] and is used in [1]. Pulling out and submitting separately as discussed in patch 18's thread in [1]. To validate, comment out the 'if' test and block preceding the 'else's changed by this patch. (Either or both will work). The poisoned ret and arg types for bpf_kptr_xchg are overwritten in these 'if's, so commenting out the overwriting will result in the added behavior causing progs which use bpf_kptr_xchg to fail to pass verifier. Can confirm by doing './test_progs -t map_kptr' . include/linux/poison.h | 3 +++ kernel/bpf/helpers.c | 6 +++--- kernel/bpf/verifier.c | 30 +++++++++++++++++++++++------- 3 files changed, 29 insertions(+), 10 deletions(-)