linux-security-module.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH bpf] security: Fix hook iteration for secid_to_secctx
@ 2020-05-20 12:56 KP Singh
  2020-05-20 15:15 ` Casey Schaufler
  2020-06-19 12:49 ` Ondrej Mosnacek
  0 siblings, 2 replies; 9+ messages in thread
From: KP Singh @ 2020-05-20 12:56 UTC (permalink / raw)
  To: linux-kernel, bpf, linux-security-module
  Cc: Alexei Starovoitov, Daniel Borkmann, James Morris, Anders Roxell,
	Casey Schaufler

From: KP Singh <kpsingh@google.com>

secid_to_secctx is not stackable, and since the BPF LSM registers this
hook by default, the call_int_hook logic is not suitable which
"bails-on-fail" and casues issues when other LSMs register this hook and
eventually breaks Audit.

In order to fix this, directly iterate over the security hooks instead
of using call_int_hook as suggested in:

https: //lore.kernel.org/bpf/9d0eb6c6-803a-ff3a-5603-9ad6d9edfc00@schaufler-ca.com/#t

Fixes: 98e828a0650f ("security: Refactor declaration of LSM hooks")
Fixes: 625236ba3832 ("security: Fix the default value of secid_to_secctx hook"
Reported-by: Alexei Starovoitov <ast@kernel.org>
Signed-off-by: KP Singh <kpsingh@google.com>
---
 security/security.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/security/security.c b/security/security.c
index 7fed24b9d57e..51de970fbb1e 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1965,8 +1965,20 @@ EXPORT_SYMBOL(security_ismaclabel);
 
 int security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen)
 {
-	return call_int_hook(secid_to_secctx, -EOPNOTSUPP, secid, secdata,
-				seclen);
+	struct security_hook_list *hp;
+	int rc;
+
+	/*
+	 * Currently, only one LSM can implement secid_to_secctx (i.e this
+	 * LSM hook is not "stackable").
+	 */
+	hlist_for_each_entry(hp, &security_hook_heads.secid_to_secctx, list) {
+		rc = hp->hook.secid_to_secctx(secid, secdata, seclen);
+		if (rc != LSM_RET_DEFAULT(secid_to_secctx))
+			return rc;
+	}
+
+	return LSM_RET_DEFAULT(secid_to_secctx);
 }
 EXPORT_SYMBOL(security_secid_to_secctx);
 
-- 
2.26.2.761.g0e0b3e54be-goog


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH bpf] security: Fix hook iteration for secid_to_secctx
  2020-05-20 12:56 [PATCH bpf] security: Fix hook iteration for secid_to_secctx KP Singh
@ 2020-05-20 15:15 ` Casey Schaufler
  2020-05-21  1:35   ` Alexei Starovoitov
  2020-06-19 12:49 ` Ondrej Mosnacek
  1 sibling, 1 reply; 9+ messages in thread
From: Casey Schaufler @ 2020-05-20 15:15 UTC (permalink / raw)
  To: KP Singh, linux-kernel, bpf, linux-security-module
  Cc: Alexei Starovoitov, Daniel Borkmann, James Morris, Anders Roxell


On 5/20/2020 5:56 AM, KP Singh wrote:
> From: KP Singh <kpsingh@google.com>
>
> secid_to_secctx is not stackable, and since the BPF LSM registers this
> hook by default, the call_int_hook logic is not suitable which
> "bails-on-fail" and casues issues when other LSMs register this hook and
> eventually breaks Audit.
>
> In order to fix this, directly iterate over the security hooks instead
> of using call_int_hook as suggested in:
>
> https: //lore.kernel.org/bpf/9d0eb6c6-803a-ff3a-5603-9ad6d9edfc00@schaufler-ca.com/#t
>
> Fixes: 98e828a0650f ("security: Refactor declaration of LSM hooks")
> Fixes: 625236ba3832 ("security: Fix the default value of secid_to_secctx hook"
> Reported-by: Alexei Starovoitov <ast@kernel.org>
> Signed-off-by: KP Singh <kpsingh@google.com>

This looks fine.

> ---
>  security/security.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/security/security.c b/security/security.c
> index 7fed24b9d57e..51de970fbb1e 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1965,8 +1965,20 @@ EXPORT_SYMBOL(security_ismaclabel);
>  
>  int security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen)
>  {
> -	return call_int_hook(secid_to_secctx, -EOPNOTSUPP, secid, secdata,
> -				seclen);
> +	struct security_hook_list *hp;
> +	int rc;
> +
> +	/*
> +	 * Currently, only one LSM can implement secid_to_secctx (i.e this
> +	 * LSM hook is not "stackable").
> +	 */
> +	hlist_for_each_entry(hp, &security_hook_heads.secid_to_secctx, list) {
> +		rc = hp->hook.secid_to_secctx(secid, secdata, seclen);
> +		if (rc != LSM_RET_DEFAULT(secid_to_secctx))
> +			return rc;
> +	}
> +
> +	return LSM_RET_DEFAULT(secid_to_secctx);
>  }
>  EXPORT_SYMBOL(security_secid_to_secctx);
>  

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH bpf] security: Fix hook iteration for secid_to_secctx
  2020-05-20 15:15 ` Casey Schaufler
@ 2020-05-21  1:35   ` Alexei Starovoitov
  2020-05-21  2:02     ` James Morris
  0 siblings, 1 reply; 9+ messages in thread
From: Alexei Starovoitov @ 2020-05-21  1:35 UTC (permalink / raw)
  To: Casey Schaufler
  Cc: KP Singh, LKML, bpf, LSM List, Alexei Starovoitov,
	Daniel Borkmann, James Morris, Anders Roxell

On Wed, May 20, 2020 at 8:15 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
>
> On 5/20/2020 5:56 AM, KP Singh wrote:
> > From: KP Singh <kpsingh@google.com>
> >
> > secid_to_secctx is not stackable, and since the BPF LSM registers this
> > hook by default, the call_int_hook logic is not suitable which
> > "bails-on-fail" and casues issues when other LSMs register this hook and
> > eventually breaks Audit.
> >
> > In order to fix this, directly iterate over the security hooks instead
> > of using call_int_hook as suggested in:
> >
> > https: //lore.kernel.org/bpf/9d0eb6c6-803a-ff3a-5603-9ad6d9edfc00@schaufler-ca.com/#t
> >
> > Fixes: 98e828a0650f ("security: Refactor declaration of LSM hooks")
> > Fixes: 625236ba3832 ("security: Fix the default value of secid_to_secctx hook"
> > Reported-by: Alexei Starovoitov <ast@kernel.org>
> > Signed-off-by: KP Singh <kpsingh@google.com>
>
> This looks fine.

Tested. audit works now.
I fixed missing ')' in the commit log
and applied to bpf tree.
It will be on the way to Linus tree soon.

Thanks!

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH bpf] security: Fix hook iteration for secid_to_secctx
  2020-05-21  1:35   ` Alexei Starovoitov
@ 2020-05-21  2:02     ` James Morris
  2020-05-21  3:12       ` Alexei Starovoitov
  0 siblings, 1 reply; 9+ messages in thread
From: James Morris @ 2020-05-21  2:02 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: Casey Schaufler, KP Singh, LKML, bpf, LSM List,
	Alexei Starovoitov, Daniel Borkmann, Anders Roxell

On Wed, 20 May 2020, Alexei Starovoitov wrote:

> On Wed, May 20, 2020 at 8:15 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >
> >
> > On 5/20/2020 5:56 AM, KP Singh wrote:
> > > From: KP Singh <kpsingh@google.com>
> > >
> > > secid_to_secctx is not stackable, and since the BPF LSM registers this
> > > hook by default, the call_int_hook logic is not suitable which
> > > "bails-on-fail" and casues issues when other LSMs register this hook and
> > > eventually breaks Audit.
> > >
> > > In order to fix this, directly iterate over the security hooks instead
> > > of using call_int_hook as suggested in:
> > >
> > > https: //lore.kernel.org/bpf/9d0eb6c6-803a-ff3a-5603-9ad6d9edfc00@schaufler-ca.com/#t
> > >
> > > Fixes: 98e828a0650f ("security: Refactor declaration of LSM hooks")
> > > Fixes: 625236ba3832 ("security: Fix the default value of secid_to_secctx hook"
> > > Reported-by: Alexei Starovoitov <ast@kernel.org>
> > > Signed-off-by: KP Singh <kpsingh@google.com>
> >
> > This looks fine.
> 
> Tested. audit works now.
> I fixed missing ')' in the commit log
> and applied to bpf tree.
> It will be on the way to Linus tree soon.

Please add:


Acked-by: James Morris <jamorris@linux.microsoft.com>


-- 
James Morris
<jmorris@namei.org>


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH bpf] security: Fix hook iteration for secid_to_secctx
  2020-05-21  2:02     ` James Morris
@ 2020-05-21  3:12       ` Alexei Starovoitov
  0 siblings, 0 replies; 9+ messages in thread
From: Alexei Starovoitov @ 2020-05-21  3:12 UTC (permalink / raw)
  To: James Morris
  Cc: Casey Schaufler, KP Singh, LKML, bpf, LSM List,
	Alexei Starovoitov, Daniel Borkmann, Anders Roxell

On Wed, May 20, 2020 at 7:02 PM James Morris <jmorris@namei.org> wrote:
>
> On Wed, 20 May 2020, Alexei Starovoitov wrote:
>
> > On Wed, May 20, 2020 at 8:15 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > >
> > >
> > > On 5/20/2020 5:56 AM, KP Singh wrote:
> > > > From: KP Singh <kpsingh@google.com>
> > > >
> > > > secid_to_secctx is not stackable, and since the BPF LSM registers this
> > > > hook by default, the call_int_hook logic is not suitable which
> > > > "bails-on-fail" and casues issues when other LSMs register this hook and
> > > > eventually breaks Audit.
> > > >
> > > > In order to fix this, directly iterate over the security hooks instead
> > > > of using call_int_hook as suggested in:
> > > >
> > > > https: //lore.kernel.org/bpf/9d0eb6c6-803a-ff3a-5603-9ad6d9edfc00@schaufler-ca.com/#t
> > > >
> > > > Fixes: 98e828a0650f ("security: Refactor declaration of LSM hooks")
> > > > Fixes: 625236ba3832 ("security: Fix the default value of secid_to_secctx hook"
> > > > Reported-by: Alexei Starovoitov <ast@kernel.org>
> > > > Signed-off-by: KP Singh <kpsingh@google.com>
> > >
> > > This looks fine.
> >
> > Tested. audit works now.
> > I fixed missing ')' in the commit log
> > and applied to bpf tree.
> > It will be on the way to Linus tree soon.
>
> Please add:
>
>
> Acked-by: James Morris <jamorris@linux.microsoft.com>

Thank you. Done.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH bpf] security: Fix hook iteration for secid_to_secctx
  2020-05-20 12:56 [PATCH bpf] security: Fix hook iteration for secid_to_secctx KP Singh
  2020-05-20 15:15 ` Casey Schaufler
@ 2020-06-19 12:49 ` Ondrej Mosnacek
  2020-06-19 13:13   ` KP Singh
  1 sibling, 1 reply; 9+ messages in thread
From: Ondrej Mosnacek @ 2020-06-19 12:49 UTC (permalink / raw)
  To: KP Singh
  Cc: Linux kernel mailing list, bpf, Linux Security Module list,
	Alexei Starovoitov, Daniel Borkmann, James Morris, Anders Roxell,
	Casey Schaufler

On Wed, May 20, 2020 at 2:56 PM KP Singh <kpsingh@chromium.org> wrote:
> From: KP Singh <kpsingh@google.com>
>
> secid_to_secctx is not stackable, and since the BPF LSM registers this
> hook by default, the call_int_hook logic is not suitable which
> "bails-on-fail" and casues issues when other LSMs register this hook and
> eventually breaks Audit.
>
> In order to fix this, directly iterate over the security hooks instead
> of using call_int_hook as suggested in:
>
> https: //lore.kernel.org/bpf/9d0eb6c6-803a-ff3a-5603-9ad6d9edfc00@schaufler-ca.com/#t
>
> Fixes: 98e828a0650f ("security: Refactor declaration of LSM hooks")
> Fixes: 625236ba3832 ("security: Fix the default value of secid_to_secctx hook"
> Reported-by: Alexei Starovoitov <ast@kernel.org>
> Signed-off-by: KP Singh <kpsingh@google.com>
[...]

Sorry for being late to the party, but doesn't this (and the
associated default return value patch) just paper over a bigger
problem? What if I have only the BPF LSM enabled and I attach a BPF
program to this hook that just returns 0? Doesn't that allow anything
privileged enough to do this to force the kernel to try and send
memory from uninitialized pointers to userspace and/or copy such
memory around and/or free uninitialized pointers?

Why on earth does the BPF LSM directly expose *all* of the hooks, even
those that are not being used for any security decisions (and are
"useful" in this context only for borking the kernel...)? Feel free to
prove me wrong, but this lazy approach of "let's just take all the
hooks as they are and stick BPF programs to them" doesn't seem like a
good choice... IMHO you should either limit the set of hooks that can
be attached to only those that aren't used to return back values via
pointers, or (if you really really need to do some state
updates/logging in those hooks) use wrapper functions that will call
the BPF progs via a simplified interface so that they cannot cause
unsafe behavior.

--
Ondrej Mosnacek
Software Engineer, Platform Security - SELinux kernel
Red Hat, Inc.


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH bpf] security: Fix hook iteration for secid_to_secctx
  2020-06-19 12:49 ` Ondrej Mosnacek
@ 2020-06-19 13:13   ` KP Singh
  2020-06-19 14:16     ` Ondrej Mosnacek
  0 siblings, 1 reply; 9+ messages in thread
From: KP Singh @ 2020-06-19 13:13 UTC (permalink / raw)
  To: Ondrej Mosnacek
  Cc: Linux kernel mailing list, bpf, Linux Security Module list,
	Alexei Starovoitov, Daniel Borkmann, James Morris, Anders Roxell,
	Casey Schaufler, Peter Zijlstra, jpoimboe

Hi,

On Fri, Jun 19, 2020 at 2:49 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> On Wed, May 20, 2020 at 2:56 PM KP Singh <kpsingh@chromium.org> wrote:
> > From: KP Singh <kpsingh@google.com>
> >
> > secid_to_secctx is not stackable, and since the BPF LSM registers this
> > hook by default, the call_int_hook logic is not suitable which
> > "bails-on-fail" and casues issues when other LSMs register this hook and
> > eventually breaks Audit.
> >
> > In order to fix this, directly iterate over the security hooks instead
> > of using call_int_hook as suggested in:
> >
> > https: //lore.kernel.org/bpf/9d0eb6c6-803a-ff3a-5603-9ad6d9edfc00@schaufler-ca.com/#t
> >
> > Fixes: 98e828a0650f ("security: Refactor declaration of LSM hooks")
> > Fixes: 625236ba3832 ("security: Fix the default value of secid_to_secctx hook"
> > Reported-by: Alexei Starovoitov <ast@kernel.org>
> > Signed-off-by: KP Singh <kpsingh@google.com>
> [...]
>
> Sorry for being late to the party, but doesn't this (and the
> associated default return value patch) just paper over a bigger
> problem? What if I have only the BPF LSM enabled and I attach a BPF
> program to this hook that just returns 0? Doesn't that allow anything
> privileged enough to do this to force the kernel to try and send
> memory from uninitialized pointers to userspace and/or copy such
> memory around and/or free uninitialized pointers?
>
> Why on earth does the BPF LSM directly expose *all* of the hooks, even
> those that are not being used for any security decisions (and are
> "useful" in this context only for borking the kernel...)? Feel free to
> prove me wrong, but this lazy approach of "let's just take all the
> hooks as they are and stick BPF programs to them" doesn't seem like a

The plan was definitely to not hook everywhere but only call the hooks
that do have a BPF program registered. This was one of the versions
we proposed in the initial patches where the call to the BPF LSM was
guarded by a static key with it being enabled only when there's a
BPF program attached to the hook.

https://lore.kernel.org/bpf/20200220175250.10795-5-kpsingh@chromium.org/

However, this special-cased BPF in the LSM framework, and, was met
with opposition. Our plan is to still achieve this, but we want to do this
with DEFINE_STATIC_CALL patches:

https://lore.kernel.org/lkml/cover.1547073843.git.jpoimboe@redhat.com

Using these, only can we enable the call into the hook based on whether
a program is attached, we can also eliminate the indirect call overhead which
currently affects the "slow" way which was decided in the discussion:

https://lore.kernel.org/bpf/202002241136.C4F9F7DFF@keescook/

> good choice... IMHO you should either limit the set of hooks that can
> be attached to only those that aren't used to return back values via

I am not sure if limiting the hooks is required here once we have
the ability to call into BPF only when a program is attached. If the
the user provides a BPF program, deliberately returns 0 (or any
other value) then it is working as intended. Even if we limit this in the
bpf LSM, deliberate privileged users can still achieve this with
other means.

- KP

> pointers, or (if you really really need to do some state
> updates/logging in those hooks) use wrapper functions that will call
> the BPF progs via a simplified interface so that they cannot cause
> unsafe behavior.
>
> --
> Ondrej Mosnacek
> Software Engineer, Platform Security - SELinux kernel
> Red Hat, Inc.
>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH bpf] security: Fix hook iteration for secid_to_secctx
  2020-06-19 13:13   ` KP Singh
@ 2020-06-19 14:16     ` Ondrej Mosnacek
  2020-06-21 21:54       ` KP Singh
  0 siblings, 1 reply; 9+ messages in thread
From: Ondrej Mosnacek @ 2020-06-19 14:16 UTC (permalink / raw)
  To: KP Singh
  Cc: Linux kernel mailing list, bpf, Linux Security Module list,
	Alexei Starovoitov, Daniel Borkmann, James Morris, Anders Roxell,
	Casey Schaufler, Peter Zijlstra, Josh Poimboeuf

On Fri, Jun 19, 2020 at 3:13 PM KP Singh <kpsingh@chromium.org> wrote:
> Hi,
>
> On Fri, Jun 19, 2020 at 2:49 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> >
> > On Wed, May 20, 2020 at 2:56 PM KP Singh <kpsingh@chromium.org> wrote:
> > > From: KP Singh <kpsingh@google.com>
> > >
> > > secid_to_secctx is not stackable, and since the BPF LSM registers this
> > > hook by default, the call_int_hook logic is not suitable which
> > > "bails-on-fail" and casues issues when other LSMs register this hook and
> > > eventually breaks Audit.
> > >
> > > In order to fix this, directly iterate over the security hooks instead
> > > of using call_int_hook as suggested in:
> > >
> > > https: //lore.kernel.org/bpf/9d0eb6c6-803a-ff3a-5603-9ad6d9edfc00@schaufler-ca.com/#t
> > >
> > > Fixes: 98e828a0650f ("security: Refactor declaration of LSM hooks")
> > > Fixes: 625236ba3832 ("security: Fix the default value of secid_to_secctx hook"
> > > Reported-by: Alexei Starovoitov <ast@kernel.org>
> > > Signed-off-by: KP Singh <kpsingh@google.com>
> > [...]
> >
> > Sorry for being late to the party, but doesn't this (and the
> > associated default return value patch) just paper over a bigger
> > problem? What if I have only the BPF LSM enabled and I attach a BPF
> > program to this hook that just returns 0? Doesn't that allow anything
> > privileged enough to do this to force the kernel to try and send
> > memory from uninitialized pointers to userspace and/or copy such
> > memory around and/or free uninitialized pointers?
> >
> > Why on earth does the BPF LSM directly expose *all* of the hooks, even
> > those that are not being used for any security decisions (and are
> > "useful" in this context only for borking the kernel...)? Feel free to
> > prove me wrong, but this lazy approach of "let's just take all the
> > hooks as they are and stick BPF programs to them" doesn't seem like a
>
> The plan was definitely to not hook everywhere but only call the hooks
> that do have a BPF program registered. This was one of the versions
> we proposed in the initial patches where the call to the BPF LSM was
> guarded by a static key with it being enabled only when there's a
> BPF program attached to the hook.
>
> https://lore.kernel.org/bpf/20200220175250.10795-5-kpsingh@chromium.org/
>
> However, this special-cased BPF in the LSM framework, and, was met
> with opposition. Our plan is to still achieve this, but we want to do this
> with DEFINE_STATIC_CALL patches:
>
> https://lore.kernel.org/lkml/cover.1547073843.git.jpoimboe@redhat.com
>
> Using these, only can we enable the call into the hook based on whether
> a program is attached, we can also eliminate the indirect call overhead which
> currently affects the "slow" way which was decided in the discussion:
>
> https://lore.kernel.org/bpf/202002241136.C4F9F7DFF@keescook/

Perhaps you are misunderstanding me... I don't have a problem with BPF
LSM registering callbacks for all the hooks. My point is about what
you can trigger once you attach programs to certain hooks. All the
above seem to be just optimizations/implementation details that do not
affect the problem I'm pointing to.

>
> > good choice... IMHO you should either limit the set of hooks that can
> > be attached to only those that aren't used to return back values via
>
> I am not sure if limiting the hooks is required here once we have
> the ability to call into BPF only when a program is attached. If the
> the user provides a BPF program, deliberately returns 0 (or any
> other value) then it is working as intended. Even if we limit this in the
> bpf LSM, deliberate privileged users can still achieve this with
> other means.

The point is that for this particular hook (secid_to_secctx) and a
couple others, the consequences of having control over the return
value are more serious than with other hooks. For most hooks, the
implementation usually just returns 0 (OK), -EACCESS (access denied)
or -E... (error) and the caller either continues as normal or handles
the error. But here if you return 0, you signal that you have
initialized the pointer and size to valid values. So suddenly the BPF
prog doesn't just control allow/deny decisions, but can now easily
trigger kernel panic. And when you look at the semantics of the hook,
you will realize that it doesn't really make sense to implement it via
BPF, since it can never populate the output values and the only
meaningful implementation would be to just return -EOPNOTSUPP.

Maybe I have it all wrong, but isn't the whole point of BPF programs
to provide a tight sandbox where you can only implement pure input ->
output functions + read/modify some internal state? Is it really
"working as intended" if you can crash the kernel by attaching a
simple BPF program to a certain hook? I mean yes, you can make the
system pretty much unusable already using the classic hooks by simply
returning -EACCESS for everything, but IMO that's quite different from
causing the kernel to do an invalid memory access.

-- 
Ondrej Mosnacek
Software Engineer, Platform Security - SELinux kernel
Red Hat, Inc.


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH bpf] security: Fix hook iteration for secid_to_secctx
  2020-06-19 14:16     ` Ondrej Mosnacek
@ 2020-06-21 21:54       ` KP Singh
  0 siblings, 0 replies; 9+ messages in thread
From: KP Singh @ 2020-06-21 21:54 UTC (permalink / raw)
  To: Ondrej Mosnacek
  Cc: Linux kernel mailing list, bpf, Linux Security Module list,
	Alexei Starovoitov, Daniel Borkmann, James Morris, Anders Roxell,
	Casey Schaufler, Peter Zijlstra, Josh Poimboeuf

On Fri, Jun 19, 2020 at 4:17 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> On Fri, Jun 19, 2020 at 3:13 PM KP Singh <kpsingh@chromium.org> wrote:
> > Hi,
> >
> > On Fri, Jun 19, 2020 at 2:49 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > >
> > > On Wed, May 20, 2020 at 2:56 PM KP Singh <kpsingh@chromium.org> wrote:
> > > > From: KP Singh <kpsingh@google.com>
> > > >
> > > > secid_to_secctx is not stackable, and since the BPF LSM registers this
> > > > hook by default, the call_int_hook logic is not suitable which
> > > > "bails-on-fail" and casues issues when other LSMs register this hook and
> > > > eventually breaks Audit.
> > > >
> > > > In order to fix this, directly iterate over the security hooks instead
> > > > of using call_int_hook as suggested in:
> > > >
> > > > https: //lore.kernel.org/bpf/9d0eb6c6-803a-ff3a-5603-9ad6d9edfc00@schaufler-ca.com/#t
> > > >
> > > > Fixes: 98e828a0650f ("security: Refactor declaration of LSM hooks")
> > > > Fixes: 625236ba3832 ("security: Fix the default value of secid_to_secctx hook"
> > > > Reported-by: Alexei Starovoitov <ast@kernel.org>
> > > > Signed-off-by: KP Singh <kpsingh@google.com>
> > > [...]
> > >
> > > Sorry for being late to the party, but doesn't this (and the
> > > associated default return value patch) just paper over a bigger
> > > problem? What if I have only the BPF LSM enabled and I attach a BPF
> > > program to this hook that just returns 0? Doesn't that allow anything
> > > privileged enough to do this to force the kernel to try and send
> > > memory from uninitialized pointers to userspace and/or copy such
> > > memory around and/or free uninitialized pointers?
> > >
> > > Why on earth does the BPF LSM directly expose *all* of the hooks, even
> > > those that are not being used for any security decisions (and are
> > > "useful" in this context only for borking the kernel...)? Feel free to
> > > prove me wrong, but this lazy approach of "let's just take all the
> > > hooks as they are and stick BPF programs to them" doesn't seem like a
> >
> > The plan was definitely to not hook everywhere but only call the hooks
> > that do have a BPF program registered. This was one of the versions
> > we proposed in the initial patches where the call to the BPF LSM was
> > guarded by a static key with it being enabled only when there's a
> > BPF program attached to the hook.
> >
> > https://lore.kernel.org/bpf/20200220175250.10795-5-kpsingh@chromium.org/
> >
> > However, this special-cased BPF in the LSM framework, and, was met
> > with opposition. Our plan is to still achieve this, but we want to do this
> > with DEFINE_STATIC_CALL patches:
> >
> > https://lore.kernel.org/lkml/cover.1547073843.git.jpoimboe@redhat.com
> >
> > Using these, only can we enable the call into the hook based on whether
> > a program is attached, we can also eliminate the indirect call overhead which
> > currently affects the "slow" way which was decided in the discussion:
> >
> > https://lore.kernel.org/bpf/202002241136.C4F9F7DFF@keescook/
>
> Perhaps you are misunderstanding me... I don't have a problem with BPF
> LSM registering callbacks for all the hooks. My point is about what
> you can trigger once you attach programs to certain hooks. All the
> above seem to be just optimizations/implementation details that do not
> affect the problem I'm pointing to.
>

The immediate concern was to fix the issue caused by the default
callback (bpf_lsm_secid_to_secctx) which affected even the users
who were not deliberately attaching a BPF program to the hook.

We can probably restrict attachment of BPF programs to be fexit trampolines
instead of fmod_ret by using some of the work that is being done for
BTF ID whitelists for the d_path helper. (fexit trampolines cannot
change the return
value of the default callback).

https://lore.kernel.org/bpf/20200616100512.2168860-9-jolsa@kernel.org/

With some of the optimization work which should remove this
default callback, we can also prevent the non-deliberate errors (the ones
that occur even when a BPF program is not attached to the LSM hook).
IMHO, these are more important to fix.

- KP

> >
> > > good choice... IMHO you should either limit the set of hooks that can
> > > be attached to only those that aren't used to return back values via
> >
> > I am not sure if limiting the hooks is required here once we have
> > the ability to call into BPF only when a program is attached. If the
> > the user provides a BPF program, deliberately returns 0 (or any
> > other value) then it is working as intended. Even if we limit this in the
> > bpf LSM, deliberate privileged users can still achieve this with
> > other means.
>
> The point is that for this particular hook (secid_to_secctx) and a
> couple others, the consequences of having control over the return
> value are more serious than with other hooks. For most hooks, the
> implementation usually just returns 0 (OK), -EACCESS (access denied)
> or -E... (error) and the caller either continues as normal or handles
> the error. But here if you return 0, you signal that you have
> initialized the pointer and size to valid values. So suddenly the BPF
> prog doesn't just control allow/deny decisions, but can now easily
> trigger kernel panic. And when you look at the semantics of the hook,
> you will realize that it doesn't really make sense to implement it via
> BPF, since it can never populate the output values and the only
> meaningful implementation would be to just return -EOPNOTSUPP.
>
> Maybe I have it all wrong, but isn't the whole point of BPF programs
> to provide a tight sandbox where you can only implement pure input ->
> output functions + read/modify some internal state? Is it really
> "working as intended" if you can crash the kernel by attaching a
> simple BPF program to a certain hook? I mean yes, you can make the
> system pretty much unusable already using the classic hooks by simply
> returning -EACCESS for everything, but IMO that's quite different from
> causing the kernel to do an invalid memory access.
>
> --
> Ondrej Mosnacek
> Software Engineer, Platform Security - SELinux kernel
> Red Hat, Inc.
>

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2020-06-21 21:54 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-20 12:56 [PATCH bpf] security: Fix hook iteration for secid_to_secctx KP Singh
2020-05-20 15:15 ` Casey Schaufler
2020-05-21  1:35   ` Alexei Starovoitov
2020-05-21  2:02     ` James Morris
2020-05-21  3:12       ` Alexei Starovoitov
2020-06-19 12:49 ` Ondrej Mosnacek
2020-06-19 13:13   ` KP Singh
2020-06-19 14:16     ` Ondrej Mosnacek
2020-06-21 21:54       ` KP Singh

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).