netdev - [PATCH bpf] bpf: Use pointer type whitelist for XADD

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-Id: <20200415204743.206086-1-jannh@google.com>
Date:   Wed, 15 Apr 2020 22:47:43 +0200
From:   Jann Horn <jannh@...gle.com>
To:     bpf@...r.kernel.org, Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>
Cc:     Martin KaFai Lau <kafai@...com>, Song Liu <songliubraving@...com>,
        Yonghong Song <yhs@...com>, Andrii Nakryiko <andriin@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...omium.org>, netdev@...r.kernel.org
Subject: [PATCH bpf] bpf: Use pointer type whitelist for XADD

At the moment, check_xadd() uses a blacklist to decide whether a given
pointer type should be usable with the XADD instruction. Out of all the
pointer types that check_mem_access() accepts, only four are currently let
through by check_xadd():

PTR_TO_MAP_VALUE
PTR_TO_CTX           rejected
PTR_TO_STACK
PTR_TO_PACKET        rejected
PTR_TO_PACKET_META   rejected
PTR_TO_FLOW_KEYS     rejected
PTR_TO_SOCKET        rejected
PTR_TO_SOCK_COMMON   rejected
PTR_TO_TCP_SOCK      rejected
PTR_TO_XDP_SOCK      rejected
PTR_TO_TP_BUFFER
PTR_TO_BTF_ID

Looking at the currently permitted ones:

 - PTR_TO_MAP_VALUE: This makes sense and is the primary usecase for XADD.
 - PTR_TO_STACK: This doesn't make much sense, there is no concurrency on
   the BPF stack. It also causes confusion further down, because the first
   check_mem_access() won't check whether the stack slot being read from is
   STACK_SPILL and the second check_mem_access() assumes in
   check_stack_write() that the value being written is a normal scalar.
   This means that unprivileged users can leak kernel pointers.
 - PTR_TO_TP_BUFFER: This is a local output buffer without concurrency.
 - PTR_TO_BTF_ID: This is read-only, XADD can't work. When the verifier
   tries to verify XADD on such memory, the first check_ptr_to_btf_access()
   invocation gets confused by value_regno not being a valid array index
   and writes to out-of-bounds memory.

Limit XADD to PTR_TO_MAP_VALUE, since everything else at least doesn't make
sense, and is sometimes broken on top of that.

Fixes: 17a5267067f3 ("bpf: verifier (add verifier core)")
Signed-off-by: Jann Horn <jannh@...gle.com>
---
I'm just sending this on the public list, since the worst-case impact for
non-root users is leaking kernel pointers to userspace. In a context where
you can reach BPF (no sandboxing), I don't think that kernel ASLR is very
effective at the moment anyway.

This breaks ten unit tests that assume that XADD is possible on the stack,
and I'm not sure how all of them should be fixed up; I'd appreciate it if
someone else could figure out how to fix them. I think some of them might
be using XADD to cast pointers to numbers, or something like that? But I'm
not sure.

Or is XADD on the stack actually something you want to support for some
reason, meaning that that part would have to be fixed differently?

 kernel/bpf/verifier.c | 27 +--------------------------
 1 file changed, 1 insertion(+), 26 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 38cfcf701eeb7..397c17a2e970f 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -2699,28 +2699,6 @@ static bool is_ctx_reg(struct bpf_verifier_env *env, int regno)
 	return reg->type == PTR_TO_CTX;
 }
 
-static bool is_sk_reg(struct bpf_verifier_env *env, int regno)
-{
-	const struct bpf_reg_state *reg = reg_state(env, regno);
-
-	return type_is_sk_pointer(reg->type);
-}
-
-static bool is_pkt_reg(struct bpf_verifier_env *env, int regno)
-{
-	const struct bpf_reg_state *reg = reg_state(env, regno);
-
-	return type_is_pkt_pointer(reg->type);
-}
-
-static bool is_flow_key_reg(struct bpf_verifier_env *env, int regno)
-{
-	const struct bpf_reg_state *reg = reg_state(env, regno);
-
-	/* Separate to is_ctx_reg() since we still want to allow BPF_ST here. */
-	return reg->type == PTR_TO_FLOW_KEYS;
-}
-
 static int check_pkt_ptr_alignment(struct bpf_verifier_env *env,
 				   const struct bpf_reg_state *reg,
 				   int off, int size, bool strict)
@@ -3298,10 +3276,7 @@ static int check_xadd(struct bpf_verifier_env *env, int insn_idx, struct bpf_ins
 		return -EACCES;
 	}
 
-	if (is_ctx_reg(env, insn->dst_reg) ||
-	    is_pkt_reg(env, insn->dst_reg) ||
-	    is_flow_key_reg(env, insn->dst_reg) ||
-	    is_sk_reg(env, insn->dst_reg)) {
+	if (reg_state(env, insn->dst_reg)->type != PTR_TO_MAP_VALUE) {
 		verbose(env, "BPF_XADD stores into R%d %s is not allowed\n",
 			insn->dst_reg,
 			reg_type_str[reg_state(env, insn->dst_reg)->type]);

base-commit: 87b0f983f66f23762921129fd35966eddc3f2dae
-- 
2.26.0.110.g2183baf09c-goog