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

[David Howells]

Alexei Starovoitov  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 
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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:
> j...@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.
>

Thank you for pointing out!

Joey Lee
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[David Howells]

j...@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
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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:
> j...@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
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[David Howells]

j...@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
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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  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 
For the current patch.
Unnecessary checks for no good reason in performance critical
functions are not acceptable.

--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[David Howells]

Alexei Starovoitov  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
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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 
> 
> 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 
> Signed-off-by: David Howells 
> cc: net...@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().

--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html