Re: [PATCH 18/27] bpf: Restrict kernel image access functions when the kernel is locked down

Re: [PATCH 18/27] bpf: Restrict kernel image access functions when the kernel is locked down

[Alexei Starovoitov]

On Thu, Oct 19, 2017 at 03:52:49PM +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 dc498b605d5d..35e85a3fdb37 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("BPF")) {
> +		memset(dst, 0, size);
> +		return -EPERM;
> +	}

That doesn't help the lockdown purpose.
If you don't trust the root the only way to prevent bpf read
memory is to disable the whole thing.
Have a single check in sys_bpf() to disallow everything if kernel_is_locked_down()
and don't add overhead to critical path like bpf_probe_read().

Re: [PATCH 18/27] bpf: Restrict kernel image access functions when the kernel is locked down

[joeyli]

Hi Alexei,

Thanks for your review! 

On Thu, Oct 19, 2017 at 03:18:30PM -0700, Alexei Starovoitov wrote:
> On Thu, Oct 19, 2017 at 03:52:49PM +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 dc498b605d5d..35e85a3fdb37 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("BPF")) {
> > +		memset(dst, 0, size);
> > +		return -EPERM;
> > +	}
> 
> That doesn't help the lockdown purpose.
> If you don't trust the root the only way to prevent bpf read
> memory is to disable the whole thing.

Not totally untrust root, I don't want that root reads
arbitrary memory address through bpf.

Is it not enough to lock down bpf_probe_read, bpf_probe_write_user
and bpf_trace_printk?

> Have a single check in sys_bpf() to disallow everything if kernel_is_locked_down()
> and don't add overhead to critical path like bpf_probe_read().
>

Yes, it give overhead to bpf_probe_read but it prevents arbitrary
memory read.

Another idea is signing bpf bytecode then verifying signture when
loading to kernel. 

Thanks a lot!
Joey Lee 

Re: [PATCH 18/27] bpf: Restrict kernel image access functions when the kernel is locked down

[David Howells]

Hi Joey,

Should I just lock down sys_bpf() entirely for now?  We can always free it up
somewhat later.

David

Re: [PATCH 18/27] bpf: Restrict kernel image access functions when the kernel is locked down

[jlee]

On Fri, Oct 20, 2017 at 09:08:48AM +0100, David Howells wrote:
> Hi Joey,
> 
> Should I just lock down sys_bpf() entirely for now?  We can always free it up
> somewhat later.
> 
> David

OK~~ Please just remove my patch until we find out a way to
verify bpf code or protect sensitive data in memory.

I think that we don't need to lock down sys_bpf() now because
we didn't lock down other interfaces for reading arbitrary
address like /dev/mem and /dev/kmem.

Thanks a lot!
Joey Lee

Re: [PATCH 18/27] bpf: Restrict kernel image access functions when the kernel is locked down

[Alexei Starovoitov]

On Fri, Oct 20, 2017 at 11:57:48PM +0800, jlee@suse.com wrote:
> On Fri, Oct 20, 2017 at 09:08:48AM +0100, David Howells wrote:
> > Hi Joey,
> > 
> > Should I just lock down sys_bpf() entirely for now?  We can always free it up
> > somewhat later.
> > 
> > David
> 
> OK~~ Please just remove my patch until we find out a way to
> verify bpf code or protect sensitive data in memory.
> 
> I think that we don't need to lock down sys_bpf() now because
> we didn't lock down other interfaces for reading arbitrary
> address like /dev/mem and /dev/kmem.

If you want to lock down read access you'd need to disable
not only bpf, but all of kprobe and likey ftrace, since
untrusted root can infer kernel data by observing function
execution even if it cannot load modules and bpf progs.

Re: [PATCH 18/27] bpf: Restrict kernel image access functions when the kernel is locked down

[David Howells]

Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:

> If you want to lock down read access you'd need to disable
> not only bpf, but all of kprobe and likey ftrace, since
> untrusted root can infer kernel data by observing function
> execution even if it cannot load modules and bpf progs.

Okay.

David

Re: [PATCH 18/27] bpf: Restrict kernel image access functions when the kernel is locked down

[David Howells]

jlee-IBi9RG/b67k@public.gmane.org wrote:

> I think that we don't need to lock down sys_bpf() now because
> we didn't lock down other interfaces for reading arbitrary
> address like /dev/mem and /dev/kmem.

Ummm...  See patch 4.  You even gave me a Reviewed-by for it ;-)

David

Re: [PATCH 18/27] bpf: Restrict kernel image access functions when the kernel is locked down

[jlee]

On Fri, Oct 20, 2017 at 05:03:22PM +0100, David Howells wrote:
> jlee@suse.com wrote:
> 
> > I think that we don't need to lock down sys_bpf() now because
> > we didn't lock down other interfaces for reading arbitrary
> > address like /dev/mem and /dev/kmem.
> 
> Ummm...  See patch 4.  You even gave me a Reviewed-by for it ;-)
> 
> David

hm... patch 4 only prevents write_mem() but not read_mem().
Or I missed anything?

Thanks
Joey Lee

Re: [PATCH 18/27] bpf: Restrict kernel image access functions when the kernel is locked down

[David Howells]

jlee@suse.com wrote:

> hm... patch 4 only prevents write_mem() but not read_mem().
> Or I missed anything?

Actually, yes, as it happens, patch 11 prevents you from even opening /dev/mem
and /dev/kmem by locking down open of /dev/port.  So I've moved this bit to
patch 4, simplified and posted a new variant for patch 4.

David

Re: [PATCH 18/27] bpf: Restrict kernel image access functions when the kernel is locked down

[joeyli]

On Mon, Oct 23, 2017 at 03:53:00PM +0100, David Howells wrote:
> jlee-IBi9RG/b67k@public.gmane.org wrote:
> 
> > hm... patch 4 only prevents write_mem() but not read_mem().
> > Or I missed anything?
> 
> Actually, yes, as it happens, patch 11 prevents you from even opening /dev/mem
> and /dev/kmem by locking down open of /dev/port.  So I've moved this bit to
> patch 4, simplified and posted a new variant for patch 4.
>

Thank you for pointing out!

Joey Lee

Re: [PATCH 18/27] bpf: Restrict kernel image access functions when the kernel is locked down

[David Howells]

Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:

> > @@ -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("BPF")) {
> > +		memset(dst, 0, size);
> > +		return -EPERM;
> > +	}
>
> That doesn't help the lockdown purpose.
> If you don't trust the root the only way to prevent bpf read
> memory is to disable the whole thing.
> Have a single check in sys_bpf() to disallow everything if kernel_is_locked_down()
> and don't add overhead to critical path like bpf_probe_read().

TBH, I've no idea how bpf does anything, so I can't say whether this is
better, overkill or insufficient.

David

Re: [PATCH 18/27] bpf: Restrict kernel image access functions when the kernel is locked down

[Alexei Starovoitov]

On Thu, Oct 19, 2017 at 11:48:34PM +0100, David Howells wrote:
> Alexei Starovoitov <alexei.starovoitov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 
> > > @@ -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("BPF")) {
> > > +		memset(dst, 0, size);
> > > +		return -EPERM;
> > > +	}
> >
> > That doesn't help the lockdown purpose.
> > If you don't trust the root the only way to prevent bpf read
> > memory is to disable the whole thing.
> > Have a single check in sys_bpf() to disallow everything if kernel_is_locked_down()
> > and don't add overhead to critical path like bpf_probe_read().
> 
> TBH, I've no idea how bpf does anything, so I can't say whether this is
> better, overkill or insufficient.

ok. To make it clear:
Nacked-by: Alexei Starovoitov <ast-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
For the current patch.
Unnecessary checks for no good reason in performance critical
functions are not acceptable.

Re: [PATCH 18/27] bpf: Restrict kernel image access functions when the kernel is locked down

[David Howells]

Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:

> > TBH, I've no idea how bpf does anything, so I can't say whether this is
> > better, overkill or insufficient.
> 
> ok. To make it clear:
> Nacked-by: Alexei Starovoitov <ast@kernel.org>
> For the current patch.
> Unnecessary checks for no good reason in performance critical
> functions are not acceptable.

They aren't unnecessary checks.

Can you please suggest if there's some way more suitable than just killing bpf
entirely?  I don't know the code, and I presume you do.

David