| Message ID | 149142344547.5101.4518618716303032193.stgit@warthog.procyon.org.uk |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
On Wed, Apr 05, 2017 at 09:17:25PM +0100, David Howells wrote: > From: Chun-Yi Lee <jlee@suse.com> > > There are some bpf functions can be used to read kernel memory: > bpf_probe_read, bpf_probe_write_user and bpf_trace_printk. These allow > private keys in kernel memory (e.g. the hibernation image signing key) to > be read by an eBPF program. Prohibit those functions when the kernel is > locked down. > > Signed-off-by: Chun-Yi Lee <jlee@suse.com> > Signed-off-by: David Howells <dhowells@redhat.com> > cc: netdev@vger.kernel.org > --- > > kernel/trace/bpf_trace.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > index cee9802cf3e0..7fde851f207b 100644 > --- a/kernel/trace/bpf_trace.c > +++ b/kernel/trace/bpf_trace.c > @@ -65,6 +65,11 @@ BPF_CALL_3(bpf_probe_read, void *, dst, u32, size, const void *, unsafe_ptr) > { > int ret; > > + if (kernel_is_locked_down()) { > + memset(dst, 0, size); > + return -EPERM; > + } this will obviously break the program. How about disabling loading tracing programs during the lockdown completely? Also is there a description of what this lockdown trying to accomplish? The cover letter is scarce in details.
On 6 April 2017 at 13:29, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > On Wed, Apr 05, 2017 at 09:17:25PM +0100, David Howells wrote: >> From: Chun-Yi Lee <jlee@suse.com> >> >> There are some bpf functions can be used to read kernel memory: >> bpf_probe_read, bpf_probe_write_user and bpf_trace_printk. These allow >> private keys in kernel memory (e.g. the hibernation image signing key) to >> be read by an eBPF program. Prohibit those functions when the kernel is >> locked down. >> >> Signed-off-by: Chun-Yi Lee <jlee@suse.com> >> Signed-off-by: David Howells <dhowells@redhat.com> >> cc: netdev@vger.kernel.org >> --- >> >> kernel/trace/bpf_trace.c | 11 +++++++++++ >> 1 file changed, 11 insertions(+) >> >> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c >> index cee9802cf3e0..7fde851f207b 100644 >> --- a/kernel/trace/bpf_trace.c >> +++ b/kernel/trace/bpf_trace.c >> @@ -65,6 +65,11 @@ BPF_CALL_3(bpf_probe_read, void *, dst, u32, size, const void *, unsafe_ptr) >> { >> int ret; >> >> + if (kernel_is_locked_down()) { >> + memset(dst, 0, size); >> + return -EPERM; >> + } > > this will obviously break the program. How about disabling loading tracing > programs during the lockdown completely? > > Also is there a description of what this lockdown trying to accomplish? > The cover letter is scarce in details. > This is a very good point, and this is actually feedback that was given (by Alan Cox, iirc) the last time this series was circulated. This series is a mixed bag of patches that all look like they improve 'security' in one way or the other. But what is lacking is a coherent view on the threat model, and to what extent all these patches reduce the vulnerability to such threats. Without that, these patches do very little beyond giving a false sense of security, imo.
Hi David, First, thanks for your help to send out this series. On Wed, Apr 05, 2017 at 09:17:25PM +0100, David Howells wrote: > From: Chun-Yi Lee <jlee@suse.com> > > There are some bpf functions can be used to read kernel memory: > bpf_probe_read, bpf_probe_write_user and bpf_trace_printk. These allow > private keys in kernel memory (e.g. the hibernation image signing key) to > be read by an eBPF program. Prohibit those functions when the kernel is > locked down. > > Signed-off-by: Chun-Yi Lee <jlee@suse.com> > Signed-off-by: David Howells <dhowells@redhat.com> > cc: netdev@vger.kernel.org This patch is used with hibernation signature verification. I suggest that we can remove this patch from your series because we just lock down the hibernation function until hibernation verification get accepted. On the other hand, we are trying to enhance the bpf verifier to prevent bpf print reads specific memory addresses that have sensitive data. Thanks a lot! Joey Lee
Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > this will obviously break the program. Yeah. But if it allows one to twiddle the kernel image or gain access to crypto material... > How about disabling loading tracing programs during the lockdown completely? Interesting thought. I'm not sure how much would actually need locking down here. Turning on tracepoints in the kernel and reading out of the trace buffer, for example, ought to be okay, though if there are any tracepoints that leak crypto information, they may need locking down also. Basically, I think it boils down to: if it can be used to modify the kernel image or read arbitrary data from the kernel image then should probably be forbidden. There have to be exceptions, though, such as loading authenticated kernel modules. David
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c index cee9802cf3e0..7fde851f207b 100644 --- a/kernel/trace/bpf_trace.c +++ b/kernel/trace/bpf_trace.c @@ -65,6 +65,11 @@ BPF_CALL_3(bpf_probe_read, void *, dst, u32, size, const void *, unsafe_ptr) { int ret; + if (kernel_is_locked_down()) { + memset(dst, 0, size); + return -EPERM; + } + ret = probe_kernel_read(dst, unsafe_ptr, size); if (unlikely(ret < 0)) memset(dst, 0, size); @@ -84,6 +89,9 @@ static const struct bpf_func_proto bpf_probe_read_proto = { BPF_CALL_3(bpf_probe_write_user, void *, unsafe_ptr, const void *, src, u32, size) { + if (kernel_is_locked_down()) + return -EPERM; + /* * Ensure we're in user context which is safe for the helper to * run. This helper has no business in a kthread. @@ -143,6 +151,9 @@ BPF_CALL_5(bpf_trace_printk, char *, fmt, u32, fmt_size, u64, arg1, if (fmt[--fmt_size] != 0) return -EINVAL; + if (kernel_is_locked_down()) + return __trace_printk(1, fmt, 0, 0, 0); + /* check format string for allowed specifiers */ for (i = 0; i < fmt_size; i++) { if ((!isprint(fmt[i]) && !isspace(fmt[i])) || !isascii(fmt[i]))