[PATCH RFC v2 0/5] LSM: Add and use a hook for side-channel safety checks

[PATCH RFC v2 0/5] LSM: Add and use a hook for side-channel safety checks

[Casey Schaufler]

v2: SELinux access policy corrected.
    Use real_cred instead of cred.

This patchset provide a mechanism by which a security module
can advise the system about potential side-channel vulnerabilities.
If security_safe_sidechannel() returns 0 the security modules do
not know of any data that would be subject to a side-channel
attack. If the security module maintains data that it believes
may be susceptible to a side-channel attack it will return -EACCES.

Simple hooks are provided for SELinux and Smack. A new security
module is provided to make determinations regarding traditional
task attributes, including user IDs, capability sets and namespaces.

Signed-off-by: Casey Schaufler <casey.schaufler@intel.com>

---
 MAINTAINERS                        |   6 ++
 arch/x86/mm/tlb.c                  |  12 ++-
 include/linux/lsm_hooks.h          |  12 +++
 include/linux/security.h           |   1 +
 security/Kconfig                   |   1 +
 security/Makefile                  |   2 +
 security/security.c                |   6 ++
 security/selinux/hooks.c           |   9 +++
 security/sidechannel/Kconfig       |  60 ++++++++++++++
 security/sidechannel/Makefile      |   1 +
 security/sidechannel/sidechannel.c | 156 +++++++++++++++++++++++++++++++++++++
 security/smack/smack_lsm.c         |  18 +++++
 12 files changed, 280 insertions(+), 4 deletions(-)

[PATCH RFC v2 1/5] LSM: Introduce a hook for side-channel danger

[Casey Schaufler]

From: Casey Schaufler <cschaufler@localhost.localdomain>

There may be cases where the data maintained for
security controls is more sensitive than general
process information and that may be subjected to
side-channel attacks. An LSM hook is provided so
that this can be check for where the system would
take action should the current task have potential
access to the passed task.

Signed-off-by: Casey Schaufler <casey.schaufler@intel.com>
---
 include/linux/lsm_hooks.h | 7 +++++++
 include/linux/security.h  | 1 +
 security/security.c       | 5 +++++
 3 files changed, 13 insertions(+)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index a08bc2587b96..fd2a7e6beb01 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -698,6 +698,11 @@
  *	security attributes, e.g. for /proc/pid inodes.
  *	@p contains the task_struct for the task.
  *	@inode contains the inode structure for the inode.
+ * @task_safe_sidechannel:
+ *	Check if a side channel attack is harmless for the current task and @p.
+ *	The caller may have determined that no attack is possible, in which
+ *	case this hook won't get called.
+ *	@p contains the task_struct for the task.
  *
  * Security hooks for Netlink messaging.
  *
@@ -1611,6 +1616,7 @@ union security_list_options {
 	int (*task_prctl)(int option, unsigned long arg2, unsigned long arg3,
 				unsigned long arg4, unsigned long arg5);
 	void (*task_to_inode)(struct task_struct *p, struct inode *inode);
+	int (*task_safe_sidechannel)(struct task_struct *p);
 
 	int (*ipc_permission)(struct kern_ipc_perm *ipcp, short flag);
 	void (*ipc_getsecid)(struct kern_ipc_perm *ipcp, u32 *secid);
@@ -1897,6 +1903,7 @@ struct security_hook_heads {
 	struct hlist_head task_kill;
 	struct hlist_head task_prctl;
 	struct hlist_head task_to_inode;
+	struct hlist_head task_safe_sidechannel;
 	struct hlist_head ipc_permission;
 	struct hlist_head ipc_getsecid;
 	struct hlist_head msg_msg_alloc_security;
diff --git a/include/linux/security.h b/include/linux/security.h
index 3410acfe139c..69a5526f789f 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -366,6 +366,7 @@ int security_task_kill(struct task_struct *p, struct siginfo *info,
 int security_task_prctl(int option, unsigned long arg2, unsigned long arg3,
 			unsigned long arg4, unsigned long arg5);
 void security_task_to_inode(struct task_struct *p, struct inode *inode);
+int security_task_safe_sidechannel(struct task_struct *p);
 int security_ipc_permission(struct kern_ipc_perm *ipcp, short flag);
 void security_ipc_getsecid(struct kern_ipc_perm *ipcp, u32 *secid);
 int security_msg_msg_alloc(struct msg_msg *msg);
diff --git a/security/security.c b/security/security.c
index 4927e7cc7d96..353b711e635a 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1165,6 +1165,11 @@ void security_task_to_inode(struct task_struct *p, struct inode *inode)
 	call_void_hook(task_to_inode, p, inode);
 }
 
+int security_task_safe_sidechannel(struct task_struct *p)
+{
+	return call_int_hook(task_safe_sidechannel, 0, p);
+}
+
 int security_ipc_permission(struct kern_ipc_perm *ipcp, short flag)
 {
 	return call_int_hook(ipc_permission, 0, ipcp, flag);
-- 
2.17.1

[PATCH RFC v2 2/5] X86: Support LSM determination of side-channel vulnerability

[Casey Schaufler]

From: Casey Schaufler <cschaufler@localhost.localdomain>

When switching between tasks it may be necessary
to set an indirect branch prediction barrier if the
tasks are potentially vulnerable to side-channel
attacks. This adds a call to security_task_safe_sidechannel
so that security modules can weigh in on the decision.

Signed-off-by: Casey Schaufler <casey.schaufler@intel.com>
---
 arch/x86/mm/tlb.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/arch/x86/mm/tlb.c b/arch/x86/mm/tlb.c
index 6eb1f34c3c85..8714d4af06aa 100644
--- a/arch/x86/mm/tlb.c
+++ b/arch/x86/mm/tlb.c
@@ -7,6 +7,7 @@
 #include <linux/export.h>
 #include <linux/cpu.h>
 #include <linux/debugfs.h>
+#include <linux/security.h>
 
 #include <asm/tlbflush.h>
 #include <asm/mmu_context.h>
@@ -270,11 +271,14 @@ void switch_mm_irqs_off(struct mm_struct *prev, struct mm_struct *next,
 		 * threads. It will also not flush if we switch to idle
 		 * thread and back to the same process. It will flush if we
 		 * switch to a different non-dumpable process.
+		 * If a security module thinks that the transition
+		 * is unsafe do the flush.
 		 */
-		if (tsk && tsk->mm &&
-		    tsk->mm->context.ctx_id != last_ctx_id &&
-		    get_dumpable(tsk->mm) != SUID_DUMP_USER)
-			indirect_branch_prediction_barrier();
+		if (tsk && tsk->mm && tsk->mm->context.ctx_id != last_ctx_id) {
+			if (get_dumpable(tsk->mm) != SUID_DUMP_USER ||
+			    security_task_safe_sidechannel(tsk) != 0)
+				indirect_branch_prediction_barrier();
+		}
 
 		if (IS_ENABLED(CONFIG_VMAP_STACK)) {
 			/*
-- 
2.17.1

Re: [PATCH RFC v2 2/5] X86: Support LSM determination of side-channel vulnerability

[Jann Horn]

On Sat, Aug 18, 2018 at 12:17 AM Casey Schaufler
<casey.schaufler@intel.com> wrote:
>
> From: Casey Schaufler <cschaufler@localhost.localdomain>
>
> When switching between tasks it may be necessary
> to set an indirect branch prediction barrier if the
> tasks are potentially vulnerable to side-channel
> attacks. This adds a call to security_task_safe_sidechannel
> so that security modules can weigh in on the decision.
>
> Signed-off-by: Casey Schaufler <casey.schaufler@intel.com>
> ---
>  arch/x86/mm/tlb.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/mm/tlb.c b/arch/x86/mm/tlb.c
> index 6eb1f34c3c85..8714d4af06aa 100644
> --- a/arch/x86/mm/tlb.c
> +++ b/arch/x86/mm/tlb.c
> @@ -7,6 +7,7 @@
>  #include <linux/export.h>
>  #include <linux/cpu.h>
>  #include <linux/debugfs.h>
> +#include <linux/security.h>
>
>  #include <asm/tlbflush.h>
>  #include <asm/mmu_context.h>
> @@ -270,11 +271,14 @@ void switch_mm_irqs_off(struct mm_struct *prev, struct mm_struct *next,
>                  * threads. It will also not flush if we switch to idle
>                  * thread and back to the same process. It will flush if we
>                  * switch to a different non-dumpable process.
> +                * If a security module thinks that the transition
> +                * is unsafe do the flush.
>                  */
> -               if (tsk && tsk->mm &&
> -                   tsk->mm->context.ctx_id != last_ctx_id &&
> -                   get_dumpable(tsk->mm) != SUID_DUMP_USER)
> -                       indirect_branch_prediction_barrier();
> +               if (tsk && tsk->mm && tsk->mm->context.ctx_id != last_ctx_id) {
> +                       if (get_dumpable(tsk->mm) != SUID_DUMP_USER ||
> +                           security_task_safe_sidechannel(tsk) != 0)
> +                               indirect_branch_prediction_barrier();
> +               }

When you posted v1 of this series, I asked:

| Does this enforce transitivity? What happens if we first switch from
| an attacker task to a task without ->mm, and immediately afterwards
| from the task without ->mm to a victim task? In that case, whether a
| flush happens between the attacker task and the victim task depends on
| whether the LSM thinks that the mm-less task should have access to the
| victim task, right?

Have you addressed that? I don't see it...

RE: [PATCH RFC v2 2/5] X86: Support LSM determination of side-channel vulnerability

[Schaufler, Casey]

> -----Original Message-----
> From: Jann Horn [mailto:jannh@google.com]
> Sent: Friday, August 17, 2018 4:55 PM
> To: Schaufler, Casey <casey.schaufler@intel.com>
> Cc: Kernel Hardening <kernel-hardening@lists.openwall.com>; kernel list
> <linux-kernel@vger.kernel.org>; linux-security-module <linux-security-
> module@vger.kernel.org>; selinux@tycho.nsa.gov; Hansen, Dave
> <dave.hansen@intel.com>; Dock, Deneen T <deneen.t.dock@intel.com>;
> kristen@linux.intel.com; Arjan van de Ven <arjan@linux.intel.com>
> Subject: Re: [PATCH RFC v2 2/5] X86: Support LSM determination of side-
> channel vulnerability
> 
> On Sat, Aug 18, 2018 at 12:17 AM Casey Schaufler
> <casey.schaufler@intel.com> wrote:
> >
> > From: Casey Schaufler <cschaufler@localhost.localdomain>
> >
> > When switching between tasks it may be necessary
> > to set an indirect branch prediction barrier if the
> > tasks are potentially vulnerable to side-channel
> > attacks. This adds a call to security_task_safe_sidechannel
> > so that security modules can weigh in on the decision.
> >
> > Signed-off-by: Casey Schaufler <casey.schaufler@intel.com>
> > ---
> >  arch/x86/mm/tlb.c | 12 ++++++++----
> >  1 file changed, 8 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/x86/mm/tlb.c b/arch/x86/mm/tlb.c
> > index 6eb1f34c3c85..8714d4af06aa 100644
> > --- a/arch/x86/mm/tlb.c
> > +++ b/arch/x86/mm/tlb.c
> > @@ -7,6 +7,7 @@
> >  #include <linux/export.h>
> >  #include <linux/cpu.h>
> >  #include <linux/debugfs.h>
> > +#include <linux/security.h>
> >
> >  #include <asm/tlbflush.h>
> >  #include <asm/mmu_context.h>
> > @@ -270,11 +271,14 @@ void switch_mm_irqs_off(struct mm_struct *prev,
> struct mm_struct *next,
> >                  * threads. It will also not flush if we switch to idle
> >                  * thread and back to the same process. It will flush if we
> >                  * switch to a different non-dumpable process.
> > +                * If a security module thinks that the transition
> > +                * is unsafe do the flush.
> >                  */
> > -               if (tsk && tsk->mm &&
> > -                   tsk->mm->context.ctx_id != last_ctx_id &&
> > -                   get_dumpable(tsk->mm) != SUID_DUMP_USER)
> > -                       indirect_branch_prediction_barrier();
> > +               if (tsk && tsk->mm && tsk->mm->context.ctx_id != last_ctx_id) {
> > +                       if (get_dumpable(tsk->mm) != SUID_DUMP_USER ||
> > +                           security_task_safe_sidechannel(tsk) != 0)
> > +                               indirect_branch_prediction_barrier();
> > +               }
> 
> When you posted v1 of this series, I asked:
> 
> | Does this enforce transitivity? What happens if we first switch from
> | an attacker task to a task without ->mm, and immediately afterwards
> | from the task without ->mm to a victim task? In that case, whether a
> | flush happens between the attacker task and the victim task depends on
> | whether the LSM thinks that the mm-less task should have access to the
> | victim task, right?
> 
> Have you addressed that? I don't see it...

Nope. That's going to require maintaining state about all the
tasks in the chain that might still have cache involvement.

	A -> B -> C -> D

If B and C don't do anything cacheworthy D could conceivably attack A.
The amount of state required to detect this case would be prohibitive.
I think that if you're sufficiently concerned about this case you should just
go ahead and set the barrier. I'm willing to learn something that says I'm
wrong.

Re: [PATCH RFC v2 2/5] X86: Support LSM determination of side-channel vulnerability

[Jann Horn]

On Mon, Aug 20, 2018 at 4:45 PM Schaufler, Casey
<casey.schaufler@intel.com> wrote:
>
> > -----Original Message-----
> > From: Jann Horn [mailto:jannh@google.com]
> > Sent: Friday, August 17, 2018 4:55 PM
> > To: Schaufler, Casey <casey.schaufler@intel.com>
> > Cc: Kernel Hardening <kernel-hardening@lists.openwall.com>; kernel list
> > <linux-kernel@vger.kernel.org>; linux-security-module <linux-security-
> > module@vger.kernel.org>; selinux@tycho.nsa.gov; Hansen, Dave
> > <dave.hansen@intel.com>; Dock, Deneen T <deneen.t.dock@intel.com>;
> > kristen@linux.intel.com; Arjan van de Ven <arjan@linux.intel.com>
> > Subject: Re: [PATCH RFC v2 2/5] X86: Support LSM determination of side-
> > channel vulnerability
> >
> > On Sat, Aug 18, 2018 at 12:17 AM Casey Schaufler
> > <casey.schaufler@intel.com> wrote:
> > >
> > > From: Casey Schaufler <cschaufler@localhost.localdomain>
> > >
> > > When switching between tasks it may be necessary
> > > to set an indirect branch prediction barrier if the
> > > tasks are potentially vulnerable to side-channel
> > > attacks. This adds a call to security_task_safe_sidechannel
> > > so that security modules can weigh in on the decision.
> > >
> > > Signed-off-by: Casey Schaufler <casey.schaufler@intel.com>
> > > ---
> > >  arch/x86/mm/tlb.c | 12 ++++++++----
> > >  1 file changed, 8 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/arch/x86/mm/tlb.c b/arch/x86/mm/tlb.c
> > > index 6eb1f34c3c85..8714d4af06aa 100644
> > > --- a/arch/x86/mm/tlb.c
> > > +++ b/arch/x86/mm/tlb.c
> > > @@ -7,6 +7,7 @@
> > >  #include <linux/export.h>
> > >  #include <linux/cpu.h>
> > >  #include <linux/debugfs.h>
> > > +#include <linux/security.h>
> > >
> > >  #include <asm/tlbflush.h>
> > >  #include <asm/mmu_context.h>
> > > @@ -270,11 +271,14 @@ void switch_mm_irqs_off(struct mm_struct *prev,
> > struct mm_struct *next,
> > >                  * threads. It will also not flush if we switch to idle
> > >                  * thread and back to the same process. It will flush if we
> > >                  * switch to a different non-dumpable process.
> > > +                * If a security module thinks that the transition
> > > +                * is unsafe do the flush.
> > >                  */
> > > -               if (tsk && tsk->mm &&
> > > -                   tsk->mm->context.ctx_id != last_ctx_id &&
> > > -                   get_dumpable(tsk->mm) != SUID_DUMP_USER)
> > > -                       indirect_branch_prediction_barrier();
> > > +               if (tsk && tsk->mm && tsk->mm->context.ctx_id != last_ctx_id) {
> > > +                       if (get_dumpable(tsk->mm) != SUID_DUMP_USER ||
> > > +                           security_task_safe_sidechannel(tsk) != 0)
> > > +                               indirect_branch_prediction_barrier();
> > > +               }
> >
> > When you posted v1 of this series, I asked:
> >
> > | Does this enforce transitivity? What happens if we first switch from
> > | an attacker task to a task without ->mm, and immediately afterwards
> > | from the task without ->mm to a victim task? In that case, whether a
> > | flush happens between the attacker task and the victim task depends on
> > | whether the LSM thinks that the mm-less task should have access to the
> > | victim task, right?
> >
> > Have you addressed that? I don't see it...
>
> Nope. That's going to require maintaining state about all the
> tasks in the chain that might still have cache involvement.
>
>         A -> B -> C -> D

Really?

From what I can tell, it'd be enough to:

 - ensure that the LSM-based access checks behave approximately transitively
   (which I think they already do, mostly)
 - keep a copy of the metadata of the last non-kernel task on the CPU

> If B and C don't do anything cacheworthy D could conceivably attack A.
> The amount of state required to detect this case would be prohibitive.
> I think that if you're sufficiently concerned about this case you should just
> go ahead and set the barrier. I'm willing to learn something that says I'm
> wrong.

That means that an attacker who can e.g. get a CPU to first switch
from an attacker task to a softirqd (e.g. for network packet
processing or whatever), then switch from the softirqd to a root-owned
victim task would be able to bypass the check, right? That doesn't
sound like a very complicated attack...

I very much dislike the idea of adding a mitigation with a known
bypass technique to the kernel.

RE: [PATCH RFC v2 2/5] X86: Support LSM determination of side-channel vulnerability

[Schaufler, Casey]

> -----Original Message-----
> From: Jann Horn [mailto:jannh@google.com]
> Sent: Tuesday, August 21, 2018 3:20 AM
> To: Schaufler, Casey <casey.schaufler@intel.com>
> Cc: Kernel Hardening <kernel-hardening@lists.openwall.com>; kernel list
> <linux-kernel@vger.kernel.org>; linux-security-module <linux-security-
> module@vger.kernel.org>; selinux@tycho.nsa.gov; Hansen, Dave
> <dave.hansen@intel.com>; Dock, Deneen T <deneen.t.dock@intel.com>;
> kristen@linux.intel.com; Arjan van de Ven <arjan@linux.intel.com>
> Subject: Re: [PATCH RFC v2 2/5] X86: Support LSM determination of side-
> channel vulnerability
> 
> On Mon, Aug 20, 2018 at 4:45 PM Schaufler, Casey
> <casey.schaufler@intel.com> wrote:
> >
> > > -----Original Message-----
> > > From: Jann Horn [mailto:jannh@google.com]
> > > Sent: Friday, August 17, 2018 4:55 PM
> > > To: Schaufler, Casey <casey.schaufler@intel.com>
> > > Cc: Kernel Hardening <kernel-hardening@lists.openwall.com>; kernel list
> > > <linux-kernel@vger.kernel.org>; linux-security-module <linux-security-
> > > module@vger.kernel.org>; selinux@tycho.nsa.gov; Hansen, Dave
> > > <dave.hansen@intel.com>; Dock, Deneen T <deneen.t.dock@intel.com>;
> > > kristen@linux.intel.com; Arjan van de Ven <arjan@linux.intel.com>
> > > Subject: Re: [PATCH RFC v2 2/5] X86: Support LSM determination of side-
> > > channel vulnerability
> > >
> > > On Sat, Aug 18, 2018 at 12:17 AM Casey Schaufler
> > > <casey.schaufler@intel.com> wrote:
> > > >
> > > > From: Casey Schaufler <cschaufler@localhost.localdomain>
> > > >
> > > > When switching between tasks it may be necessary
> > > > to set an indirect branch prediction barrier if the
> > > > tasks are potentially vulnerable to side-channel
> > > > attacks. This adds a call to security_task_safe_sidechannel
> > > > so that security modules can weigh in on the decision.
> > > >
> > > > Signed-off-by: Casey Schaufler <casey.schaufler@intel.com>
> > > > ---
> > > >  arch/x86/mm/tlb.c | 12 ++++++++----
> > > >  1 file changed, 8 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/arch/x86/mm/tlb.c b/arch/x86/mm/tlb.c
> > > > index 6eb1f34c3c85..8714d4af06aa 100644
> > > > --- a/arch/x86/mm/tlb.c
> > > > +++ b/arch/x86/mm/tlb.c
> > > > @@ -7,6 +7,7 @@
> > > >  #include <linux/export.h>
> > > >  #include <linux/cpu.h>
> > > >  #include <linux/debugfs.h>
> > > > +#include <linux/security.h>
> > > >
> > > >  #include <asm/tlbflush.h>
> > > >  #include <asm/mmu_context.h>
> > > > @@ -270,11 +271,14 @@ void switch_mm_irqs_off(struct mm_struct
> *prev,
> > > struct mm_struct *next,
> > > >                  * threads. It will also not flush if we switch to idle
> > > >                  * thread and back to the same process. It will flush if we
> > > >                  * switch to a different non-dumpable process.
> > > > +                * If a security module thinks that the transition
> > > > +                * is unsafe do the flush.
> > > >                  */
> > > > -               if (tsk && tsk->mm &&
> > > > -                   tsk->mm->context.ctx_id != last_ctx_id &&
> > > > -                   get_dumpable(tsk->mm) != SUID_DUMP_USER)
> > > > -                       indirect_branch_prediction_barrier();
> > > > +               if (tsk && tsk->mm && tsk->mm->context.ctx_id != last_ctx_id)
> {
> > > > +                       if (get_dumpable(tsk->mm) != SUID_DUMP_USER ||
> > > > +                           security_task_safe_sidechannel(tsk) != 0)
> > > > +                               indirect_branch_prediction_barrier();
> > > > +               }
> > >
> > > When you posted v1 of this series, I asked:
> > >
> > > | Does this enforce transitivity? What happens if we first switch from
> > > | an attacker task to a task without ->mm, and immediately afterwards
> > > | from the task without ->mm to a victim task? In that case, whether a
> > > | flush happens between the attacker task and the victim task depends on
> > > | whether the LSM thinks that the mm-less task should have access to the
> > > | victim task, right?
> > >
> > > Have you addressed that? I don't see it...
> >
> > Nope. That's going to require maintaining state about all the
> > tasks in the chain that might still have cache involvement.
> >
> >         A -> B -> C -> D
> 
> Really?

I am willing to be educated otherwise. My understanding
of Modern Processor Technology will never be so deep that
I won't listen to reason.

> 
> From what I can tell, it'd be enough to:
> 
>  - ensure that the LSM-based access checks behave approximately transitively
>    (which I think they already do, mostly)

Smack rules are explicitly and intentionally not transitive.

A reads B, B reads C does *not* imply A reads C.

>  - keep a copy of the metadata of the last non-kernel task on the CPU

Do you have a suggestion of how one might do that?
I'm willing to believe the information could be available,
but I have yet to come up with a mechanism for getting it.

> 
> > If B and C don't do anything cacheworthy D could conceivably attack A.
> > The amount of state required to detect this case would be prohibitive.
> > I think that if you're sufficiently concerned about this case you should just
> > go ahead and set the barrier. I'm willing to learn something that says I'm
> > wrong.
> 
> That means that an attacker who can e.g. get a CPU to first switch
> from an attacker task to a softirqd (e.g. for network packet
> processing or whatever), then switch from the softirqd to a root-owned
> victim task would be able to bypass the check, right? That doesn't
> sound like a very complicated attack...

Maybe my brain is still stuck in the 1980's, but that sounds pretty
complicated to me! Of course, the fact that it's beyond where I would
go doesn't mean it's implausible.

> 
> I very much dislike the idea of adding a mitigation with a known
> bypass technique to the kernel.

That's fair. I'll look more closely at getting previous_cred_this_cpu().

Thank!

Re: [PATCH RFC v2 2/5] X86: Support LSM determination of side-channel vulnerability

[Jann Horn]

On Tue, Aug 21, 2018 at 6:37 PM Schaufler, Casey
<casey.schaufler@intel.com> wrote:
>
> > -----Original Message-----
> > From: Jann Horn [mailto:jannh@google.com]
> > Sent: Tuesday, August 21, 2018 3:20 AM
> > To: Schaufler, Casey <casey.schaufler@intel.com>
> > Cc: Kernel Hardening <kernel-hardening@lists.openwall.com>; kernel list
> > <linux-kernel@vger.kernel.org>; linux-security-module <linux-security-
> > module@vger.kernel.org>; selinux@tycho.nsa.gov; Hansen, Dave
> > <dave.hansen@intel.com>; Dock, Deneen T <deneen.t.dock@intel.com>;
> > kristen@linux.intel.com; Arjan van de Ven <arjan@linux.intel.com>
> > Subject: Re: [PATCH RFC v2 2/5] X86: Support LSM determination of side-
> > channel vulnerability
> >
> > On Mon, Aug 20, 2018 at 4:45 PM Schaufler, Casey
> > <casey.schaufler@intel.com> wrote:
> > >
> > > > -----Original Message-----
> > > > From: Jann Horn [mailto:jannh@google.com]
> > > > Sent: Friday, August 17, 2018 4:55 PM
> > > > To: Schaufler, Casey <casey.schaufler@intel.com>
> > > > Cc: Kernel Hardening <kernel-hardening@lists.openwall.com>; kernel list
> > > > <linux-kernel@vger.kernel.org>; linux-security-module <linux-security-
> > > > module@vger.kernel.org>; selinux@tycho.nsa.gov; Hansen, Dave
> > > > <dave.hansen@intel.com>; Dock, Deneen T <deneen.t.dock@intel.com>;
> > > > kristen@linux.intel.com; Arjan van de Ven <arjan@linux.intel.com>
> > > > Subject: Re: [PATCH RFC v2 2/5] X86: Support LSM determination of side-
> > > > channel vulnerability
> > > >
> > > > On Sat, Aug 18, 2018 at 12:17 AM Casey Schaufler
> > > > <casey.schaufler@intel.com> wrote:
> > > > >
> > > > > From: Casey Schaufler <cschaufler@localhost.localdomain>
> > > > >
> > > > > When switching between tasks it may be necessary
> > > > > to set an indirect branch prediction barrier if the
> > > > > tasks are potentially vulnerable to side-channel
> > > > > attacks. This adds a call to security_task_safe_sidechannel
> > > > > so that security modules can weigh in on the decision.
> > > > >
> > > > > Signed-off-by: Casey Schaufler <casey.schaufler@intel.com>
> > > > > ---
> > > > >  arch/x86/mm/tlb.c | 12 ++++++++----
> > > > >  1 file changed, 8 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/arch/x86/mm/tlb.c b/arch/x86/mm/tlb.c
> > > > > index 6eb1f34c3c85..8714d4af06aa 100644
> > > > > --- a/arch/x86/mm/tlb.c
> > > > > +++ b/arch/x86/mm/tlb.c
> > > > > @@ -7,6 +7,7 @@
> > > > >  #include <linux/export.h>
> > > > >  #include <linux/cpu.h>
> > > > >  #include <linux/debugfs.h>
> > > > > +#include <linux/security.h>
> > > > >
> > > > >  #include <asm/tlbflush.h>
> > > > >  #include <asm/mmu_context.h>
> > > > > @@ -270,11 +271,14 @@ void switch_mm_irqs_off(struct mm_struct
> > *prev,
> > > > struct mm_struct *next,
> > > > >                  * threads. It will also not flush if we switch to idle
> > > > >                  * thread and back to the same process. It will flush if we
> > > > >                  * switch to a different non-dumpable process.
> > > > > +                * If a security module thinks that the transition
> > > > > +                * is unsafe do the flush.
> > > > >                  */
> > > > > -               if (tsk && tsk->mm &&
> > > > > -                   tsk->mm->context.ctx_id != last_ctx_id &&
> > > > > -                   get_dumpable(tsk->mm) != SUID_DUMP_USER)
> > > > > -                       indirect_branch_prediction_barrier();
> > > > > +               if (tsk && tsk->mm && tsk->mm->context.ctx_id != last_ctx_id)
> > {
> > > > > +                       if (get_dumpable(tsk->mm) != SUID_DUMP_USER ||
> > > > > +                           security_task_safe_sidechannel(tsk) != 0)
> > > > > +                               indirect_branch_prediction_barrier();
> > > > > +               }
> > > >
> > > > When you posted v1 of this series, I asked:
> > > >
> > > > | Does this enforce transitivity? What happens if we first switch from
> > > > | an attacker task to a task without ->mm, and immediately afterwards
> > > > | from the task without ->mm to a victim task? In that case, whether a
> > > > | flush happens between the attacker task and the victim task depends on
> > > > | whether the LSM thinks that the mm-less task should have access to the
> > > > | victim task, right?
> > > >
> > > > Have you addressed that? I don't see it...
> > >
> > > Nope. That's going to require maintaining state about all the
> > > tasks in the chain that might still have cache involvement.
> > >
> > >         A -> B -> C -> D
> >
> > Really?
>
> I am willing to be educated otherwise. My understanding
> of Modern Processor Technology will never be so deep that
> I won't listen to reason.
>
> >
> > From what I can tell, it'd be enough to:
> >
> >  - ensure that the LSM-based access checks behave approximately transitively
> >    (which I think they already do, mostly)
>
> Smack rules are explicitly and intentionally not transitive.
>
> A reads B, B reads C does *not* imply A reads C.

Ah. :(

Well, at least for UID-based checks, capability comparisons and
namespace comparisons, the relationship should be transitive, right?

> >  - keep a copy of the metadata of the last non-kernel task on the CPU
>
> Do you have a suggestion of how one might do that?
> I'm willing to believe the information could be available,
> but I have yet to come up with a mechanism for getting it.

The obvious solution would be to take a refcounted reference on the
old task's objective creds, but you probably want to avoid the
resulting cache line bouncing...

For safe_by_uid(), I think you could get away with just stashing the
last UID in a percpu variable, instead of keeping the full creds
struct around. That should be fairly cheap?

Namespace comparisons, and whatever SELinux/Smack/AppArmor do
internally, are probably more complicated, since you'd potentially
have to deal with changes of internal IDs and such if the policy gets
reloaded in the wrong moment.
For namespaces, perhaps you could give each namespace a unique 128-bit
ID and then save and compare those, just like UIDs.
For LSMs whose internal IDs might change after a policy reload, things
would probably be more messy. Perhaps you could save, e.g. for
SELinux, something like an (sid,policy_generation_counter) pair? I
don't know all that much about the internals of classic LSMs.

> > > If B and C don't do anything cacheworthy D could conceivably attack A.
> > > The amount of state required to detect this case would be prohibitive.
> > > I think that if you're sufficiently concerned about this case you should just
> > > go ahead and set the barrier. I'm willing to learn something that says I'm
> > > wrong.
> >
> > That means that an attacker who can e.g. get a CPU to first switch
> > from an attacker task to a softirqd (e.g. for network packet
> > processing or whatever), then switch from the softirqd to a root-owned
> > victim task would be able to bypass the check, right? That doesn't
> > sound like a very complicated attack...
>
> Maybe my brain is still stuck in the 1980's, but that sounds pretty
> complicated to me! Of course, the fact that it's beyond where I would
> go doesn't mean it's implausible.

It seems to me like this could happen relatively easily if you have
one attacker task that keeps calling sched_yield() together with a
victim task on a logical core that's also running a softirqd? Attacker
voluntarily preempts, softirqd runs for packet processing, softirqd
ends processing, kernel schedules victim? I'm not sure how high the
injection success rate would be with that though.

> > I very much dislike the idea of adding a mitigation with a known
> > bypass technique to the kernel.
>
> That's fair. I'll look more closely at getting previous_cred_this_cpu().
>
> Thank!
>

[PATCH RFC v2 3/5] LSM: Security module checking for side-channel dangers

[Casey Schaufler]

From: Casey Schaufler <cschaufler@localhost.localdomain>

The sidechannel LSM checks for cases where a side-channel
attack may be dangerous based on security attributes of tasks.
This includes:
	Effective UID of the tasks is different
	Capablity sets are different
	Tasks are in different namespaces
An option is also provided to assert that task are never
to be considered safe. This is high paranoia, and expensive
as well.

Signed-off-by: Casey Schaufler <casey.schaufler@intel.com>
---
 MAINTAINERS                        |   6 ++
 include/linux/lsm_hooks.h          |   5 +
 security/Kconfig                   |   1 +
 security/Makefile                  |   2 +
 security/security.c                |   1 +
 security/sidechannel/Kconfig       |  60 +++++++++++
 security/sidechannel/Makefile      |   1 +
 security/sidechannel/sidechannel.c | 156 +++++++++++++++++++++++++++++
 8 files changed, 232 insertions(+)
 create mode 100644 security/sidechannel/Kconfig
 create mode 100644 security/sidechannel/Makefile
 create mode 100644 security/sidechannel/sidechannel.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 3119bba7971c..d078d6a5b471 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13066,6 +13066,12 @@ F:	drivers/slimbus/
 F:	Documentation/devicetree/bindings/slimbus/
 F:	include/linux/slimbus.h
 
+SIDECHANNEL SECURITY MODULE
+M:	Casey Schaufler <casey.schaufler@intel.com>
+L:	linux-security-module@vger.kernel.org
+S:	Maintained
+F:	security/sidechannel/
+
 SMACK SECURITY MODULE
 M:	Casey Schaufler <casey@schaufler-ca.com>
 L:	linux-security-module@vger.kernel.org
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index fd2a7e6beb01..d48e4a085fe2 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -2088,5 +2088,10 @@ void __init loadpin_add_hooks(void);
 #else
 static inline void loadpin_add_hooks(void) { };
 #endif
+#ifdef CONFIG_SECURITY_SIDECHANNEL
+void __init sidechannel_add_hooks(void);
+#else
+static inline void sidechannel_add_hooks(void) { };
+#endif
 
 #endif /* ! __LINUX_LSM_HOOKS_H */
diff --git a/security/Kconfig b/security/Kconfig
index c4302067a3ad..28cb7b2939ee 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -237,6 +237,7 @@ source security/tomoyo/Kconfig
 source security/apparmor/Kconfig
 source security/loadpin/Kconfig
 source security/yama/Kconfig
+source security/sidechannel/Kconfig
 
 source security/integrity/Kconfig
 
diff --git a/security/Makefile b/security/Makefile
index 4d2d3782ddef..d0c9e1b227f9 100644
--- a/security/Makefile
+++ b/security/Makefile
@@ -10,6 +10,7 @@ subdir-$(CONFIG_SECURITY_TOMOYO)        += tomoyo
 subdir-$(CONFIG_SECURITY_APPARMOR)	+= apparmor
 subdir-$(CONFIG_SECURITY_YAMA)		+= yama
 subdir-$(CONFIG_SECURITY_LOADPIN)	+= loadpin
+subdir-$(CONFIG_SECURITY_SIDECHANNEL)	+= sidechannel
 
 # always enable default capabilities
 obj-y					+= commoncap.o
@@ -25,6 +26,7 @@ obj-$(CONFIG_SECURITY_TOMOYO)		+= tomoyo/
 obj-$(CONFIG_SECURITY_APPARMOR)		+= apparmor/
 obj-$(CONFIG_SECURITY_YAMA)		+= yama/
 obj-$(CONFIG_SECURITY_LOADPIN)		+= loadpin/
+obj-$(CONFIG_SECURITY_SIDECHANNEL)	+= sidechannel/
 obj-$(CONFIG_CGROUP_DEVICE)		+= device_cgroup.o
 
 # Object integrity file lists
diff --git a/security/security.c b/security/security.c
index 353b711e635a..777919349751 100644
--- a/security/security.c
+++ b/security/security.c
@@ -80,6 +80,7 @@ int __init security_init(void)
 	capability_add_hooks();
 	yama_add_hooks();
 	loadpin_add_hooks();
+	sidechannel_add_hooks();
 
 	/*
 	 * Load all the remaining security modules.
diff --git a/security/sidechannel/Kconfig b/security/sidechannel/Kconfig
new file mode 100644
index 000000000000..af9396534128
--- /dev/null
+++ b/security/sidechannel/Kconfig
@@ -0,0 +1,60 @@
+config SECURITY_SIDECHANNEL
+	bool "Sidechannel attack safety extra checks"
+	depends on SECURITY
+	default n
+	help
+	  Look for a variety of cases where a side-channel attack
+	  could potentially be exploited. Instruct the switching
+	  code to use the indirect_branch_prediction_barrier in
+	  cases where the passed task and the current task may be
+	  at risk.
+
+          If you are unsure how to answer this question, answer N.
+
+config SECURITY_SIDECHANNEL_UIDS
+	bool "Sidechannel check on UID"
+	depends on SECURITY_SIDECHANNEL
+	default n
+	help
+	  Assume that tasks with different effective UIDs may be
+	  subject to side-channel attacks. As most task switching
+	  occurs between tasks with different effective UIDs this
+	  can have a significant performance impact.
+
+          If you are unsure how to answer this question, answer N.
+
+
+config SECURITY_SIDECHANNEL_CAPABILITIES
+	bool "Sidechannel check on capability sets"
+	depends on SECURITY_SIDECHANNEL
+	default n
+	help
+	  Assume that tasks with different sets of privilege may be
+	  subject to side-channel attacks. Potential interactions
+	  where the attacker lacks capabilities the attacked has
+	  are blocked.
+
+          If you are unsure how to answer this question, answer N.
+
+config SECURITY_SIDECHANNEL_NAMESPACES
+	bool "Sidechannel check on namespaces"
+	depends on SECURITY_SIDECHANNEL
+	depends on NAMESPACES
+	default n
+	help
+	  Assume that tasks in different namespaces may be
+	  subject to side-channel attacks. User, PID and cgroup
+	  namespaces are checked.
+
+          If you are unsure how to answer this question, answer N.
+
+config SECURITY_SIDECHANNEL_ALWAYS
+	bool "Sidechannel assumed to always be possible"
+	depends on SECURITY_SIDECHANNEL
+	default n
+	help
+	  Assume that all tasks may be subject to side-channel attacks.
+	  Always instruct the system to use countermeasures regardless
+	  of the potential impact.
+
+          If you are unsure how to answer this question, answer N.
diff --git a/security/sidechannel/Makefile b/security/sidechannel/Makefile
new file mode 100644
index 000000000000..f61d83f28035
--- /dev/null
+++ b/security/sidechannel/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_SECURITY_SIDECHANNEL) += sidechannel.o
diff --git a/security/sidechannel/sidechannel.c b/security/sidechannel/sidechannel.c
new file mode 100644
index 000000000000..9dc875611bd8
--- /dev/null
+++ b/security/sidechannel/sidechannel.c
@@ -0,0 +1,156 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Side Channel Safety Security Module
+ *
+ * Copyright (C) 2018 Intel Corporation.
+ *
+ */
+
+#define pr_fmt(fmt) "SideChannel: " fmt
+
+#include <linux/types.h>
+#include <linux/lsm_hooks.h>
+#include <linux/capability.h>
+#include <linux/cred.h>
+#include <linux/sched.h>
+#include <linux/string_helpers.h>
+#include <linux/nsproxy.h>
+#include <linux/pid_namespace.h>
+
+#ifdef CONFIG_SECURITY_SIDECHANNEL_ALWAYS
+static int sidechannel_task_safe_sidechannel(struct task_struct *p)
+{
+	return -EACCES;
+}
+#else
+/*
+ * safe_by_uid - Are task and current sidechannel safe?
+ * @p: task to check on
+ *
+ * Returns 0 if the tasks are sidechannel safe, -EACCES otherwise.
+ */
+#ifdef CONFIG_SECURITY_SIDECHANNEL_UIDS
+static int safe_by_uid(struct task_struct *p)
+{
+	const struct cred *ccred = current_real_cred();
+	const struct cred *pcred = get_task_cred(p);
+
+	/*
+	 * Credential checks. Considered safe if:
+	 *	UIDs are the same
+	 */
+	if (ccred != pcred && ccred->euid.val != pcred->euid.val)
+		return -EACCES;
+	return 0;
+}
+#else
+static inline int safe_by_uid(struct task_struct *p)
+{
+	return 0;
+}
+#endif
+
+/*
+ * safe_by_capability - Are task and current sidechannel safe?
+ * @p: task to check on
+ *
+ * Returns 0 if the tasks are sidechannel safe, -EACCES otherwise.
+ */
+#ifdef CONFIG_SECURITY_SIDECHANNEL_CAPABILITIES
+static int safe_by_capability(struct task_struct *p)
+{
+	const struct cred *ccred = current_real_cred();
+	const struct cred *pcred = get_task_cred(p);
+
+	/*
+	 * Capabilities checks. Considered safe if:
+	 *	current has all the capabilities p does
+	 */
+	if (ccred != pcred &&
+	    !cap_issubset(pcred->cap_effective, ccred->cap_effective))
+		return -EACCES;
+	return 0;
+}
+#else
+static inline int safe_by_capability(struct task_struct *p)
+{
+	return 0;
+}
+#endif
+
+#ifdef CONFIG_SECURITY_SIDECHANNEL_NAMESPACES
+/**
+ * safe_by_namespace - Are task and current sidechannel safe?
+ * @p: task to check on
+ *
+ * Returns 0 if the tasks are sidechannel safe, -EACCES otherwise.
+ */
+static int safe_by_namespace(struct task_struct *p)
+{
+	struct cgroup_namespace *ccgn = NULL;
+	struct cgroup_namespace *pcgn = NULL;
+
+	/*
+	 * Namespace checks. Considered safe if:
+	 *	cgroup namespace is the same
+	 *	User namespace is the same
+	 *	PID namespace is the same
+	 */
+	if (current->nsproxy)
+		ccgn = current->nsproxy->cgroup_ns;
+	if (p->nsproxy)
+		pcgn = p->nsproxy->cgroup_ns;
+	if (ccgn != pcgn)
+		return -EACCES;
+	if (current_real_cred()->user_ns != get_task_cred(p)->user_ns)
+		return -EACCES;
+	if (task_active_pid_ns(current) != task_active_pid_ns(p))
+		return -EACCES;
+	return 0;
+}
+#else
+static inline int safe_by_namespace(struct task_struct *p)
+{
+	return 0;
+}
+#endif
+
+/**
+ * sidechannel_task_safe_sidechannel - Are task and current sidechannel safe?
+ * @p: task to check on
+ *
+ * Returns 0 if the tasks are sidechannel safe, -EACCES otherwise.
+ */
+static int sidechannel_task_safe_sidechannel(struct task_struct *p)
+{
+	int rc;
+
+	/*
+	 * Easy optimizations
+	 */
+	if (p == current || p->pid == current->pid)
+		return 0;
+
+	rc = safe_by_uid(p);
+	if (rc)
+		return rc;
+	rc = safe_by_capability(p);
+	if (rc)
+		return rc;
+	rc = safe_by_namespace(p);
+	if (rc)
+		return rc;
+	return 0;
+}
+#endif /* CONFIG_SECURITY_SIDECHANNEL_ALWAYS */
+
+static struct security_hook_list sidechannel_hooks[] __lsm_ro_after_init = {
+	LSM_HOOK_INIT(task_safe_sidechannel, sidechannel_task_safe_sidechannel),
+};
+
+void __init sidechannel_add_hooks(void)
+{
+	pr_info("Extra sidechannel checks enabled\n");
+	security_add_hooks(sidechannel_hooks, ARRAY_SIZE(sidechannel_hooks),
+			   "sidechannel");
+}
-- 
2.17.1

Re: [PATCH RFC v2 3/5] LSM: Security module checking for side-channel dangers

[Jann Horn]

On Sat, Aug 18, 2018 at 12:17 AM Casey Schaufler
<casey.schaufler@intel.com> wrote:
>
> From: Casey Schaufler <cschaufler@localhost.localdomain>
>
> The sidechannel LSM checks for cases where a side-channel
> attack may be dangerous based on security attributes of tasks.
> This includes:
>         Effective UID of the tasks is different
>         Capablity sets are different
>         Tasks are in different namespaces
> An option is also provided to assert that task are never
> to be considered safe. This is high paranoia, and expensive
> as well.
>
> Signed-off-by: Casey Schaufler <casey.schaufler@intel.com>
[...]
> +#ifdef CONFIG_SECURITY_SIDECHANNEL_UIDS
> +static int safe_by_uid(struct task_struct *p)
> +{
> +       const struct cred *ccred = current_real_cred();
> +       const struct cred *pcred = get_task_cred(p);
> +
> +       /*
> +        * Credential checks. Considered safe if:
> +        *      UIDs are the same
> +        */
> +       if (ccred != pcred && ccred->euid.val != pcred->euid.val)
> +               return -EACCES;
> +       return 0;
> +}

This function looks bogus. get_task_cred() bumps the refcount on the
returned cred struct pointer, but you don't drop it. You probably want
to use something that doesn't fiddle with the refcount at all here to
avoid cacheline bouncing - possibly a raw rcu_dereference_protected()
if there are no better helpers.

Same thing for the other get_task_cred() calls further down in the patch.

RE: [PATCH RFC v2 3/5] LSM: Security module checking for side-channel dangers

[Schaufler, Casey]

> -----Original Message-----
> From: Jann Horn [mailto:jannh@google.com]
> Sent: Friday, August 17, 2018 4:53 PM
> To: Schaufler, Casey <casey.schaufler@intel.com>
> Cc: Kernel Hardening <kernel-hardening@lists.openwall.com>; kernel list
> <linux-kernel@vger.kernel.org>; linux-security-module <linux-security-
> module@vger.kernel.org>; selinux@tycho.nsa.gov; Hansen, Dave
> <dave.hansen@intel.com>; Dock, Deneen T <deneen.t.dock@intel.com>;
> kristen@linux.intel.com; Arjan van de Ven <arjan@linux.intel.com>
> Subject: Re: [PATCH RFC v2 3/5] LSM: Security module checking for side-
> channel dangers
> 
> On Sat, Aug 18, 2018 at 12:17 AM Casey Schaufler
> <casey.schaufler@intel.com> wrote:
> >
> > From: Casey Schaufler <cschaufler@localhost.localdomain>
> >
> > The sidechannel LSM checks for cases where a side-channel
> > attack may be dangerous based on security attributes of tasks.
> > This includes:
> >         Effective UID of the tasks is different
> >         Capablity sets are different
> >         Tasks are in different namespaces
> > An option is also provided to assert that task are never
> > to be considered safe. This is high paranoia, and expensive
> > as well.
> >
> > Signed-off-by: Casey Schaufler <casey.schaufler@intel.com>
> [...]
> > +#ifdef CONFIG_SECURITY_SIDECHANNEL_UIDS
> > +static int safe_by_uid(struct task_struct *p)
> > +{
> > +       const struct cred *ccred = current_real_cred();
> > +       const struct cred *pcred = get_task_cred(p);
> > +
> > +       /*
> > +        * Credential checks. Considered safe if:
> > +        *      UIDs are the same
> > +        */
> > +       if (ccred != pcred && ccred->euid.val != pcred->euid.val)
> > +               return -EACCES;
> > +       return 0;
> > +}
> 
> This function looks bogus. get_task_cred() bumps the refcount on the
> returned cred struct pointer, but you don't drop it. You probably want
> to use something that doesn't fiddle with the refcount at all here to
> avoid cacheline bouncing - possibly a raw rcu_dereference_protected()
> if there are no better helpers.
> 
> Same thing for the other get_task_cred() calls further down in the patch.

Thanks. Looks like I whacked out v2 a bit hastily.

[PATCH RFC v2 4/5] Smack: Support determination of side-channel vulnerability

[Casey Schaufler]

Smack considers its private task data safe if the current task
has read access to the passed task.

Signed-off-by: Casey Schaufler <casey.schaufler@intel.com>
---
 security/smack/smack_lsm.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 91750205a5de..85dc053e610c 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -2299,6 +2299,23 @@ static void smack_task_to_inode(struct task_struct *p, struct inode *inode)
 	isp->smk_inode = skp;
 }
 
+/**
+ * smack_task_safe_sidechannel - Are the task and current sidechannel safe?
+ * @p: task to check on
+ *
+ * A crude value for sidechannel safety is that the current task is
+ * already allowed to read from the other.
+ *
+ * Returns 0 if the tasks are sidechannel safe, -EACCES otherwise.
+ */
+static int smack_task_safe_sidechannel(struct task_struct *p)
+{
+	struct smack_known *skp = smk_of_task_struct(p);
+	struct smack_known *ckp = smk_of_task_struct(current);
+
+	return smk_access(ckp, skp, MAY_READ, NULL);
+}
+
 /*
  * Socket hooks.
  */
@@ -4718,6 +4735,7 @@ static struct security_hook_list smack_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(task_movememory, smack_task_movememory),
 	LSM_HOOK_INIT(task_kill, smack_task_kill),
 	LSM_HOOK_INIT(task_to_inode, smack_task_to_inode),
+	LSM_HOOK_INIT(task_safe_sidechannel, smack_task_safe_sidechannel),
 
 	LSM_HOOK_INIT(ipc_permission, smack_ipc_permission),
 	LSM_HOOK_INIT(ipc_getsecid, smack_ipc_getsecid),
-- 
2.17.1

[PATCH RFC v2 5/5] SELinux: Support SELinux determination of side-channel vulnerability

[Casey Schaufler]

SELinux considers tasks to be side-channel safe if they
have PROCESS_SHARE access.

Signed-off-by: Casey Schaufler <casey.schaufler@intel.com>
---
 security/selinux/hooks.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index a8bf324130f5..7fbd7d7ac1cb 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -4219,6 +4219,14 @@ static void selinux_task_to_inode(struct task_struct *p,
 	spin_unlock(&isec->lock);
 }
 
+static int selinux_task_safe_sidechannel(struct task_struct *p)
+{
+	struct av_decision avd;
+
+	return avc_has_perm_noaudit(&selinux_state, current_sid(), task_sid(p),
+				    SECCLASS_FILE, FILE__READ, 0, &avd);
+}
+
 /* Returns error only if unable to parse addresses */
 static int selinux_parse_skb_ipv4(struct sk_buff *skb,
 			struct common_audit_data *ad, u8 *proto)
@@ -7002,6 +7010,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(task_movememory, selinux_task_movememory),
 	LSM_HOOK_INIT(task_kill, selinux_task_kill),
 	LSM_HOOK_INIT(task_to_inode, selinux_task_to_inode),
+	LSM_HOOK_INIT(task_safe_sidechannel, selinux_task_safe_sidechannel),
 
 	LSM_HOOK_INIT(ipc_permission, selinux_ipc_permission),
 	LSM_HOOK_INIT(ipc_getsecid, selinux_ipc_getsecid),
-- 
2.17.1

Re: [PATCH RFC v2 5/5] SELinux: Support SELinux determination of side-channel vulnerability

[Stephen Smalley]

On 08/17/2018 06:16 PM, Casey Schaufler wrote:
> SELinux considers tasks to be side-channel safe if they
> have PROCESS_SHARE access.

Now the description and the code no longer match.

> 
> Signed-off-by: Casey Schaufler <casey.schaufler@intel.com>
> ---
>   security/selinux/hooks.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index a8bf324130f5..7fbd7d7ac1cb 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -4219,6 +4219,14 @@ static void selinux_task_to_inode(struct task_struct *p,
>   	spin_unlock(&isec->lock);
>   }
>   
> +static int selinux_task_safe_sidechannel(struct task_struct *p)
> +{
> +	struct av_decision avd;
> +
> +	return avc_has_perm_noaudit(&selinux_state, current_sid(), task_sid(p),
> +				    SECCLASS_FILE, FILE__READ, 0, &avd);
> +}

And my question from before still stands:  why do we need a new hook and 
new security module instead of just using ptrace_may_access()?

> +
>   /* Returns error only if unable to parse addresses */
>   static int selinux_parse_skb_ipv4(struct sk_buff *skb,
>   			struct common_audit_data *ad, u8 *proto)
> @@ -7002,6 +7010,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
>   	LSM_HOOK_INIT(task_movememory, selinux_task_movememory),
>   	LSM_HOOK_INIT(task_kill, selinux_task_kill),
>   	LSM_HOOK_INIT(task_to_inode, selinux_task_to_inode),
> +	LSM_HOOK_INIT(task_safe_sidechannel, selinux_task_safe_sidechannel),
>   
>   	LSM_HOOK_INIT(ipc_permission, selinux_ipc_permission),
>   	LSM_HOOK_INIT(ipc_getsecid, selinux_ipc_getsecid),
> 

RE: [PATCH RFC v2 5/5] SELinux: Support SELinux determination of side-channel vulnerability

[Schaufler, Casey]

> -----Original Message-----
> From: Stephen Smalley [mailto:sds@tycho.nsa.gov]
> Sent: Monday, August 20, 2018 9:03 AM
> To: Schaufler, Casey <casey.schaufler@intel.com>; kernel-
> hardening@lists.openwall.com; linux-kernel@vger.kernel.org; linux-security-
> module@vger.kernel.org; selinux@tycho.nsa.gov; Hansen, Dave
> <dave.hansen@intel.com>; Dock, Deneen T <deneen.t.dock@intel.com>;
> kristen@linux.intel.com; arjan@linux.intel.com
> Subject: Re: [PATCH RFC v2 5/5] SELinux: Support SELinux determination of
> side-channel vulnerability
> 
> On 08/17/2018 06:16 PM, Casey Schaufler wrote:
> > SELinux considers tasks to be side-channel safe if they
> > have PROCESS_SHARE access.
> 
> Now the description and the code no longer match.

You're right.

> >
> > Signed-off-by: Casey Schaufler <casey.schaufler@intel.com>
> > ---
> >   security/selinux/hooks.c | 9 +++++++++
> >   1 file changed, 9 insertions(+)
> >
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index a8bf324130f5..7fbd7d7ac1cb 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -4219,6 +4219,14 @@ static void selinux_task_to_inode(struct
> task_struct *p,
> >   	spin_unlock(&isec->lock);
> >   }
> >
> > +static int selinux_task_safe_sidechannel(struct task_struct *p)
> > +{
> > +	struct av_decision avd;
> > +
> > +	return avc_has_perm_noaudit(&selinux_state, current_sid(),
> task_sid(p),
> > +				    SECCLASS_FILE, FILE__READ, 0, &avd);
> > +}
> 
> And my question from before still stands:  why do we need a new hook and
> new security module instead of just using ptrace_may_access()?

Locking. The SELinux check, for example, will lock up solid while trying
to generate an audit record. There is no good reason aside from coding
convenience to assume that the same restrictions will apply for side-channel
as apply to ptrace. I'm actually a touch surprised you're not suggesting a
separate SECCLASS or access mode for the SELinux hook.

> 
> > +
> >   /* Returns error only if unable to parse addresses */
> >   static int selinux_parse_skb_ipv4(struct sk_buff *skb,
> >   			struct common_audit_data *ad, u8 *proto)
> > @@ -7002,6 +7010,7 @@ static struct security_hook_list selinux_hooks[]
> __lsm_ro_after_init = {
> >   	LSM_HOOK_INIT(task_movememory, selinux_task_movememory),
> >   	LSM_HOOK_INIT(task_kill, selinux_task_kill),
> >   	LSM_HOOK_INIT(task_to_inode, selinux_task_to_inode),
> > +	LSM_HOOK_INIT(task_safe_sidechannel,
> selinux_task_safe_sidechannel),
> >
> >   	LSM_HOOK_INIT(ipc_permission, selinux_ipc_permission),
> >   	LSM_HOOK_INIT(ipc_getsecid, selinux_ipc_getsecid),
> >

Re: [PATCH RFC v2 5/5] SELinux: Support SELinux determination of side-channel vulnerability

[Stephen Smalley]

On 08/20/2018 12:59 PM, Schaufler, Casey wrote:
>> -----Original Message-----
>> From: Stephen Smalley [mailto:sds@tycho.nsa.gov]
>> Sent: Monday, August 20, 2018 9:03 AM
>> To: Schaufler, Casey <casey.schaufler@intel.com>; kernel-
>> hardening@lists.openwall.com; linux-kernel@vger.kernel.org; linux-security-
>> module@vger.kernel.org; selinux@tycho.nsa.gov; Hansen, Dave
>> <dave.hansen@intel.com>; Dock, Deneen T <deneen.t.dock@intel.com>;
>> kristen@linux.intel.com; arjan@linux.intel.com
>> Subject: Re: [PATCH RFC v2 5/5] SELinux: Support SELinux determination of
>> side-channel vulnerability
>>
>> On 08/17/2018 06:16 PM, Casey Schaufler wrote:
>>> SELinux considers tasks to be side-channel safe if they
>>> have PROCESS_SHARE access.
>>
>> Now the description and the code no longer match.
> 
> You're right.
> 
>>>
>>> Signed-off-by: Casey Schaufler <casey.schaufler@intel.com>
>>> ---
>>>    security/selinux/hooks.c | 9 +++++++++
>>>    1 file changed, 9 insertions(+)
>>>
>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>>> index a8bf324130f5..7fbd7d7ac1cb 100644
>>> --- a/security/selinux/hooks.c
>>> +++ b/security/selinux/hooks.c
>>> @@ -4219,6 +4219,14 @@ static void selinux_task_to_inode(struct
>> task_struct *p,
>>>    	spin_unlock(&isec->lock);
>>>    }
>>>
>>> +static int selinux_task_safe_sidechannel(struct task_struct *p)
>>> +{
>>> +	struct av_decision avd;
>>> +
>>> +	return avc_has_perm_noaudit(&selinux_state, current_sid(),
>> task_sid(p),
>>> +				    SECCLASS_FILE, FILE__READ, 0, &avd);
>>> +}
>>
>> And my question from before still stands:  why do we need a new hook and
>> new security module instead of just using ptrace_may_access()?
> 
> Locking. The SELinux check, for example, will lock up solid while trying
> to generate an audit record. There is no good reason aside from coding
> convenience to assume that the same restrictions will apply for side-channel
> as apply to ptrace. I'm actually a touch surprised you're not suggesting a
> separate SECCLASS or access mode for the SELinux hook.

The PTRACE_MODE_NOAUDIT flag to ptrace_may_access() would address the 
locking concern. Duplicating the ptrace access checking logic seems 
prone to errors and inconsistencies. I can't imagine policy writers 
understanding what "safe sidechannel" means, much less deciding when to 
allow it.

> 
>>
>>> +
>>>    /* Returns error only if unable to parse addresses */
>>>    static int selinux_parse_skb_ipv4(struct sk_buff *skb,
>>>    			struct common_audit_data *ad, u8 *proto)
>>> @@ -7002,6 +7010,7 @@ static struct security_hook_list selinux_hooks[]
>> __lsm_ro_after_init = {
>>>    	LSM_HOOK_INIT(task_movememory, selinux_task_movememory),
>>>    	LSM_HOOK_INIT(task_kill, selinux_task_kill),
>>>    	LSM_HOOK_INIT(task_to_inode, selinux_task_to_inode),
>>> +	LSM_HOOK_INIT(task_safe_sidechannel,
>> selinux_task_safe_sidechannel),
>>>
>>>    	LSM_HOOK_INIT(ipc_permission, selinux_ipc_permission),
>>>    	LSM_HOOK_INIT(ipc_getsecid, selinux_ipc_getsecid),
>>>
> 

RE: [PATCH RFC v2 5/5] SELinux: Support SELinux determination of side-channel vulnerability

[Schaufler, Casey]

> -----Original Message-----
> From: Stephen Smalley [mailto:sds@tycho.nsa.gov]
> Sent: Monday, August 20, 2018 10:44 AM
> To: Schaufler, Casey <casey.schaufler@intel.com>; kernel-
> hardening@lists.openwall.com; linux-kernel@vger.kernel.org; linux-security-
> module@vger.kernel.org; selinux@tycho.nsa.gov; Hansen, Dave
> <dave.hansen@intel.com>; Dock, Deneen T <deneen.t.dock@intel.com>;
> kristen@linux.intel.com; arjan@linux.intel.com
> Subject: Re: [PATCH RFC v2 5/5] SELinux: Support SELinux determination of
> side-channel vulnerability
> 
> On 08/20/2018 12:59 PM, Schaufler, Casey wrote:
> >> -----Original Message-----
> >> From: Stephen Smalley [mailto:sds@tycho.nsa.gov]
> >> Sent: Monday, August 20, 2018 9:03 AM
> >> To: Schaufler, Casey <casey.schaufler@intel.com>; kernel-
> >> hardening@lists.openwall.com; linux-kernel@vger.kernel.org; linux-security-
> >> module@vger.kernel.org; selinux@tycho.nsa.gov; Hansen, Dave
> >> <dave.hansen@intel.com>; Dock, Deneen T <deneen.t.dock@intel.com>;
> >> kristen@linux.intel.com; arjan@linux.intel.com
> >> Subject: Re: [PATCH RFC v2 5/5] SELinux: Support SELinux determination of
> >> side-channel vulnerability
> >>
> >> On 08/17/2018 06:16 PM, Casey Schaufler wrote:
> >>> SELinux considers tasks to be side-channel safe if they
> >>> have PROCESS_SHARE access.
> >>
> >> Now the description and the code no longer match.
> >
> > You're right.
> >
> >>>
> >>> Signed-off-by: Casey Schaufler <casey.schaufler@intel.com>
> >>> ---
> >>>    security/selinux/hooks.c | 9 +++++++++
> >>>    1 file changed, 9 insertions(+)
> >>>
> >>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> >>> index a8bf324130f5..7fbd7d7ac1cb 100644
> >>> --- a/security/selinux/hooks.c
> >>> +++ b/security/selinux/hooks.c
> >>> @@ -4219,6 +4219,14 @@ static void selinux_task_to_inode(struct
> >> task_struct *p,
> >>>    	spin_unlock(&isec->lock);
> >>>    }
> >>>
> >>> +static int selinux_task_safe_sidechannel(struct task_struct *p)
> >>> +{
> >>> +	struct av_decision avd;
> >>> +
> >>> +	return avc_has_perm_noaudit(&selinux_state, current_sid(),
> >> task_sid(p),
> >>> +				    SECCLASS_FILE, FILE__READ, 0, &avd);
> >>> +}
> >>
> >> And my question from before still stands:  why do we need a new hook and
> >> new security module instead of just using ptrace_may_access()?
> >
> > Locking. The SELinux check, for example, will lock up solid while trying
> > to generate an audit record. There is no good reason aside from coding
> > convenience to assume that the same restrictions will apply for side-channel
> > as apply to ptrace. I'm actually a touch surprised you're not suggesting a
> > separate SECCLASS or access mode for the SELinux hook.
> 
> The PTRACE_MODE_NOAUDIT flag to ptrace_may_access() would address the
> locking concern.

OK ...

> Duplicating the ptrace access checking logic seems
> prone to errors and inconsistencies.

That's true only if the ptrace logic and the safe-sidechannel logic
are identical. I don't believe that is a safe assumption. It would sure
be convenient. But I would hate to see a change made for either
ptrace or safe_sidechannel that interfered with the correct behavior
of the other.

> I can't imagine policy writers
> understanding what "safe sidechannel" means, much less deciding when to
> allow it.

I can't argue with that. But then, I have always had trouble with the
SELinux policy scheme.