SSB extra v2 0

This is still highly RFC. I would mostly like feedback from folks as to whether the overall method of plugging into BPF is more acceptable now. This has not been tested on real hardware yet.

--

BPF is a potential source of gadgets that can be used for memory diambiguation-based attacks. To help mitigate these, we enable the bit in SPEC_CTRL which enables the reduced (memory) speculation mode on the processor when runing BPF code.

This improves on the last version quite a bit: there is no overhead for unmitigated BPF programs. Instead of calling the mitigation code unconditionally, we stick it in a wrapper function which then calls the real BPF payload. In the unmitigated case, the function pointer just points to the payload directly.

This also does not do any of the arch-specific bits. Thomas has some code to do that.

Big caveat: I've only been looking at BPF for a couple of days. I'm not deepy familiar with the code and I'm positive there are bugs and tons of ways to optimize this.

Thomas Gleixner(1):
bpf: Add speculation control interface

Dave Hansen (4):
bpf: install mitigation wrapper for untrusted programs
bpf: set prog->need_mitigation when calling other programs
bpf: populate prog->need_mitigation for unprivileged programs
bpf: only mitigate programs that write to memory

 include/linux/bpf_verifier.h |  1 +
 include/linux/filter.h       |  7 ++++++-
 include/linux/nospec.h       |  4 ++++
 kernel/bpf/core.c            | 14 ++++++++++++++
 kernel/bpf/verifier.c        | 13 +++++++++++++
 net/core/filter.c            | 25 +++++++++++++++++++++++++
 6 files changed, 63 insertions(+), 1 deletion(-)

2.9.5

From: Thomas Gleixner <[email protected]>

---
 include/linux/nospec.h | 4 ++++
 kernel/bpf/core.c      | 5 +++++
 2 files changed, 9 insertions(+)

index a908c95..d04ca98 100644
@@ -63,4 +63,8 @@ int arch_prctl_spec_ctrl_get(struct task_struct *task, unsigned long which);
 int arch_prctl_spec_ctrl_set(struct task_struct *task, unsigned long which,
 			     unsigned long ctrl);
 
+/* Speculation control for BPF enforced mitigation */
+void arch_bpf_spec_ctrl_disable(void);
+void arch_bpf_spec_ctrl_enable(void);
+
 #endif /* _LINUX_NOSPEC_H */

index ba03ec3..dd7caa1 100644
@@ -31,6 +31,7 @@
 #include <linux/rbtree_latch.h>
 #include <linux/kallsyms.h>
 #include <linux/rcupdate.h>
+#include <linux/nospec.h>
 
 #include <asm/unaligned.h>
 

@@ -1805,6 +1806,10 @@ const struct bpf_func_proto bpf_tail_call_proto = {
 	.arg3_type	= ARG_ANYTHING,
 };
 
+/* Stubs for architectures which do not implement BPF speculation control */
+void __weak arch_bpf_spec_ctrl_disable(void) { }
+void __weak arch_bpf_spec_ctrl_enable(void) { }
+
 /* Stub for JITs that only support cBPF. eBPF programs are interpreted.
  * It is encouraged to implement bpf_int_jit_compile() instead, so that
  * eBPF and implicitly also cBPF can get JITed!

2.9.5

From: Dave Hansen <[email protected]>

BPF programs can potentially do bad things, especially where side-channels are concerned. This introduces some infrastructure to help deal with them.

Each BPF program has a function pointer (->bpf_func) which either points to an interpreter or to the actual JIT'd code. We override it when running untrusted programs (defined by having CAP_SYS_ADMIN at program load time), but leave it in place for trusted ones. This way, the runtime overhead is basically zero for programs which receive no
mitigation: it just calls directly into the original code.

The function we override it to (bpf_untrusted_wrapper()) just turns on mitigations when entering the program and turns them off before returning. Ideally, we would change the ->bpf_func call to actually pass 'bpf_prog' itself instead of deriving it via container_of(), but this means less churn for now.

Why not use the BPF machine itself to do the mitigation by patching in instructions?

We could attempt to do the mitigation in the eBPF instructions themselves, but that gets messy, fast. We would need something like bpf__gen_prologue(), which I ran away from in terror. We would also need to ensure that offloaded programs did not receive the mitigation instructions.

eBPF programs can also "call" other programs, but those calls never return, so they are more similar to execve(). That means that the mitigations at the beginning of programs *have* to be conditional in some way if implemented inside the eBPF machine. It also means that we do not get a nice, clean call/return pair.

---
 include/linux/filter.h |  7 ++++++-
 kernel/bpf/core.c      |  9 +++++++++
 net/core/filter.c      | 25 +++++++++++++++++++++++++
 3 files changed, 40 insertions(+), 1 deletion(-)

index fc4e8f9..ac10038 100644
@@ -467,7 +467,8 @@ struct bpf_prog {
 				dst_needed:1,	/* Do we need dst entry? */
 				blinded:1,	/* Was blinded */
 				is_func:1,	/* program is a bpf function */
-				kprobe_override:1; /* Do we override a kprobe? */
+				kprobe_override:1, /* Do we override a kprobe? */
+				need_mitigation:1; /* Need speculation fixes? */
 	enum bpf_prog_type	type;		/* Type of BPF program */
 	enum bpf_attach_type	expected_attach_type; /* For some prog types */
 	u32			len;		/* Number of filter blocks */

@@ -477,6 +478,8 @@ struct bpf_prog {
 	struct sock_fprog_kern	*orig_prog;	/* Original BPF program */
 	unsigned int		(*bpf_func)(const void *ctx,
 					    const struct bpf_insn *insn);
+	unsigned int		(*bpf_orig_func)(const void *ctx,
+					    const struct bpf_insn *insn);
 	/* Instructions for interpreter */
 	union {
 		struct sock_filter	insns[0];

@@ -1051,4 +1054,6 @@ struct bpf_sock_ops_kern {
 					 */
 };
 
+unsigned int bpf_untrusted_wrapper(const void *ctx, const struct bpf_insn *insn);
+
 #endif /* __LINUX_FILTER_H__ */

index dd7caa1..15e4865 100644
@@ -1501,6 +1501,15 @@ struct bpf_prog *bpf_prog_select_runtime(struct bpf_prog *fp, int *err)
 		if (*err)
 			return fp;
 	}
+
+	/* Must be done after JIT has set fp->bpf_func: */
+	if (fp->need_mitigation) {
+		/* Stash the original function: */
+		fp->bpf_orig_func = fp->bpf_func;
+		/* Replace it with the wrapper for untrusted programs: */
+		fp->bpf_func = bpf_untrusted_wrapper;
+	}
+
 	bpf_prog_lock_ro(fp);
 
 	/* The tail call compatibility check can only be done at

index d31aff9..f8b8099 100644
@@ -48,6 +48,7 @@
 #include <linux/filter.h>
 #include <linux/ratelimit.h>
 #include <linux/seccomp.h>
+#include <linux/nospec.h>
 #include <linux/if_vlan.h>
 #include <linux/bpf.h>
 #include <net/sch_generic.h>

@@ -5649,3 +5650,27 @@ int sk_get_filter(struct sock *sk, struct sock_filter __user *ubuf,
 	release_sock(sk);
 	return ret;
 }
+
+/*
+ * This function is installed as the ->bpf_func in all bpf_prog's that
+ * are not trusted.  They might have been loaded by an untrusted user
+ * or call potentially untrusted code.  They might be doing something
+ * that reduces kernel hardening, so add mitigations while they are
+ * running.
+ *
+ * This is nice for things like offloaded BPF programs because they
+ * do not use ->bpf_func.
+ */
+unsigned int bpf_untrusted_wrapper(const void *ctx, const struct bpf_insn *insn)
+{
+	struct bpf_prog *prog = container_of(insn, struct bpf_prog, insnsi[0]);
+	unsigned int ret;
+
+	arch_bpf_spec_ctrl_enable();
+	ret = prog->bpf_orig_func(ctx, insn);
+	arch_bpf_spec_ctrl_disable();
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(bpf_untrusted_wrapper);
+

2.9.5

From: Dave Hansen <[email protected]>

When calling other programs, assume that those programs are not trusted and mark the current program as needing mitigation.

This situation can obviously be improved by checking the calling program to see if it needs mitigation. That step is left as an exercise to the reader (the folks who have actually looked at the BPF code for more than two days in their life).

---
 kernel/bpf/verifier.c | 2 ++
 1 file changed, 2 insertions(+)

index 5dd1dcb..ee454b6 100644
@@ -5481,6 +5481,8 @@ static int fixup_bpf_calls(struct bpf_verifier_env *env)
 			 * the program array.
 			 */
 			prog->cb_access = 1;
+			prog->need_mitigation = 1;
+
 			env->prog->aux->stack_depth = MAX_BPF_STACK;
 
 			/* mark bpf_tail_call as different opcode to avoid

2.9.5

From: Dave Hansen <[email protected]>

For now, assume that all programs loaded by unpriviliged users are untrusted and need mitigation.

Again, there are lots of optimizations we can do here, but this is obviously a conservative place to start.

---
 kernel/bpf/syscall.c | 2 ++
 1 file changed, 2 insertions(+)

index 4ca46df..fa4d9e3 100644
@@ -1292,6 +1292,8 @@ static int bpf_prog_load(union bpf_attr *attr)
 			   bpf_prog_insn_size(prog)) != 0)
 		goto free_prog;
 
+	/* Mitigate all programs loaded without CAP_SYS_ADMIN: */
+	prog->need_mitigation = !capable(CAP_SYS_ADMIN);
 	prog->orig_prog = NULL;
 	prog->jited = 0;
 

2.9.5

From: Dave Hansen <[email protected]>

Think of this more as an example of what we can do than a full-blown mitigation. Since we go through and verify BFP programs and we also know that speculative-store-bypass requires stores (aka writes), we have the verifier record if it saw a memory write. If not, like if the program operates on registers alone, then do not bother with mitigation.

---
 include/linux/bpf_verifier.h |  1 +
 kernel/bpf/syscall.c         |  2 --
 kernel/bpf/verifier.c        | 11 +++++++++++
 3 files changed, 12 insertions(+), 2 deletions(-)

index 7e61c39..396b5f1 100644
@@ -189,6 +189,7 @@ struct bpf_verifier_env {
 	u32 id_gen;			/* used to generate unique reg IDs */
 	bool allow_ptr_leaks;
 	bool seen_direct_write;
+	bool saw_memory_write;		/* Did the program write to memory? */
 	struct bpf_insn_aux_data *insn_aux_data; /* array of per-insn state */
 	struct bpf_verifier_log log;
 	u32 subprog_starts[BPF_MAX_SUBPROGS];

index fa4d9e3..4ca46df 100644
@@ -1292,8 +1292,6 @@ static int bpf_prog_load(union bpf_attr *attr)
 			   bpf_prog_insn_size(prog)) != 0)
 		goto free_prog;
 
-	/* Mitigate all programs loaded without CAP_SYS_ADMIN: */
-	prog->need_mitigation = !capable(CAP_SYS_ADMIN);
 	prog->orig_prog = NULL;
 	prog->jited = 0;
 

index ee454b6..83fc611 100644
@@ -1601,6 +1601,9 @@ static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, u32 regn
 	struct bpf_func_state *state;
 	int size, err = 0;
 
+	if (t == BPF_WRITE)
+		env->saw_memory_write = true;
+
 	size = bpf_size_to_bytes(bpf_size);
 	if (size < 0)
 		return size;

@@ -5739,6 +5742,14 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr)
 		convert_pseudo_ld_imm64(env);
 	}
 
+	/* To mitigate speculative-store-bypass, we only need
+	 * mitigation for programs that write to memory.  Mark that
+	 * the program needs mitigation if loaded without
+	 * CAP_SYS_ADMIN:
+	 */
+	if (env->saw_memory_write && !capable(CAP_SYS_ADMIN))
+		env->prog->need_mitigation = true;
+
 err_release_maps:
 	if (!env->prog->aux->used_maps)
 		/* if we didn't copy map pointers into bpf_prog_info, release

2.9.5

For now, it *is* generic, though. It's whether the mitigation function gets injected into the call path or not.

Once we have multiple things we have to mitigate for at entry/exit, we may need to add some other flags to the bpf_prog that say exactly what mitigations we need, like a "bfp_prog->does_memory_write" that explicitly tells us if we need the SSB mitigation.

Now looking at the overhead of this SSB hardware mitigation stuff I really wonder what's the state on investigating software mitigations for BPF.

The magic PDF talks about that, but has anyone looked at that?

Thanks,

tglx

I don't know of anyone that has. I'm fairly sure no one at Intel has looked in detail, at least in the post-Spectre timeframe.

Frankly, I think it's mostly a waste of time to do it without having the BPF folks deeply involved from day one. They were not fans of the way we (Intel) did Spectre V1 mitigations and I don't expect this to be any different.

They were deeply opposed to LFENCE getting used, for instance.

Great, why can't we drag them into this then? We obviously need their help, as anything we come up with is going to just be made fun of by them :)

thanks,

greg k-h

I've asked specifically that we get Alexei Starovoitov on here. I've been told that it's in the works, but it's a bit opaque to me what's actually going on.

I asked (admittedly on this list only) Jon to bring Daniel in here as well, but no response to that, unfortunately.

Jiri Kosina
SUSE Labs

I just asked about this, at least from Intel's perspective: Daniel is a bit harder to do because he's not attached to a big company that folks have existing relationships with.

How about we do Alexei first, and then we can work on Daniel if he really is critical?

Just to explicitly followup (because I missed your second mail, sorry Jiri). I saw the original ask but then your next mail said it was ok. I missed some email last week due to Red Hat Summit and patch scrambling. I'm currently going over everything I might have missed but also flying to a funeral in the UK - will you let me know if you'd still like me to push for additional folks to be read in?
Jon.

Computer Architect | Sent from my Fedora powered laptop

If the main worry is using the store buffer bypass to basically bypass type safety (by loading a pointer that was prevously stored as something else), I would assume that it could be done with something much more targeted than an lfence.

But I do suspect that once they see a MSR write on each entry/exit, they'll say "we'll do the lfence". You can probably have some simple greedy model where you only do an lfence for pointer loads, and only if you did a store before, and maybe it won't be too painful.

But yes, we definitely should have Alexei involved. For some reason I thought he already was.

Linus