[7/7,v2] tracing: Do not create tracefs files if tracefs lockdown is in effect

Commit Message

Steven Rostedt Oct. 12, 2019, 12:57 a.m. UTC

From: "Steven Rostedt (VMware)" <[e-mail]>

If on boot up, lockdown is activated for tracefs, don't even bother creating
the files. This can also prevent instances from being created if lockdown is
in effect.

Link: http://lkml.kernel.org/r/CAHk-=whC6Ji=[e-mail]

Suggested-by: Linus Torvalds <[e-mail]>
Signed-off-by: Steven Rostedt (VMware) <[e-mail]>
---
 fs/tracefs/inode.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Ondrej Mosnacek April 13, 2021, 8:13 a.m. UTC | #1

On Sat, Oct 12, 2019 at 2:59 AM Steven Rostedt <[e-mail]> wrote:
> From: "Steven Rostedt (VMware)" <[e-mail]>
>
> If on boot up, lockdown is activated for tracefs, don't even bother creating
> the files. This can also prevent instances from being created if lockdown is
> in effect.
>
> Link: http://lkml.kernel.org/r/CAHk-=whC6Ji=[e-mail]
>
> Suggested-by: Linus Torvalds <[e-mail]>
> Signed-off-by: Steven Rostedt (VMware) <[e-mail]>
> ---
>  fs/tracefs/inode.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/fs/tracefs/inode.c b/fs/tracefs/inode.c
> index eeeae0475da9..0caa151cae4e 100644
> --- a/fs/tracefs/inode.c
> +++ b/fs/tracefs/inode.c
> @@ -16,6 +16,7 @@
>  #include <linux/namei.h>
>  #include <linux/tracefs.h>
>  #include <linux/fsnotify.h>
> +#include <linux/security.h>
>  #include <linux/seq_file.h>
>  #include <linux/parser.h>
>  #include <linux/magic.h>
> @@ -390,6 +391,9 @@ struct dentry *tracefs_create_file(const char *name, umode_t mode,
>         struct dentry *dentry;
>         struct inode *inode;
>
> +       if (security_locked_down(LOCKDOWN_TRACEFS))
> +               return NULL;
> +
>         if (!(mode & S_IFMT))
>                 mode |= S_IFREG;
>         BUG_ON(!S_ISREG(mode));
> --
> 2.23.0

Hi all,

sorry for coming back to an old thread, but it turns out that this
patch doesn't play well with SELinux's implementation of the
security_locked_down() hook, which was added a few months later (so
not your fault :) in commit 59438b46471a ("security,lockdown,selinux:
implement SELinux lockdown").

What SELinux does is it checks if the current task's creds are allowed
the lockdown::integrity or lockdown::confidentiality permission in the
policy whenever security_locked_down() is called. The idea is to be
able to control at SELinux domain level which tasks can do these
sensitive operations (when the kernel is not actually locked down by
the Lockdown LSM).

With this patch + the SELinux lockdown mechanism in use, when a
userspace task loads a module that creates some tracefs nodes in its
initcall SELinux will check if the task has the
lockdown::confidentiality permission and if not, will report denials
in audit log and prevent the tracefs entries from being created. But
that is not a very logical behavior, since the task loading the module
is itself not (explicitly) doing anything that would breach
confidentiality. It just indirectly causes some tracefs nodes to be
created, but doesn't actually use them at that point.

Since it seems the other patches also added security_locked_down()
calls to the tracefs nodes' open functions, I guess reverting this
patch could be an acceptable way to fix this problem (please correct
me if there is something that this call catches, which the other ones
don't). However, even then I can understand that you (or someone else)
might want to keep this as an optimization, in which case we could
instead do this:
1. Add a new hook security_locked_down_permanently() (the name is open
for discussion), which would be intended for situations when we want
to avoid doing some pointless work when the kernel is in a "hard"
lockdown that can't be taken back (except perhaps in some rescue
scenario...).
2. This hook would be backed by the same implementation as
security_locked_down() in the Lockdown LSM and left unimplemented by
SELinux.
3. tracefs_create_file() would call this hook instead of security_locked_down().

This way it would work as before relative to the standard lockdown via
the Lockdown LSM and would be simply ignored by SELinux. I went over
all the security_locked_down() call in the kernel and I think this
alternative hook could also fit better in arch/powerpc/xmon/xmon.c,
where it seems to be called from interrupt context (so task creds are
irrelevant, anyway...) and mainly causes some values to be redacted.
(I also found a couple minor issues with how the hook is used in other
places, for which I plan to send patches later.)

Thoughts?

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

Ondrej Mosnacek May 7, 2021, noon UTC | #2

On Tue, Apr 13, 2021 at 10:13 AM Ondrej Mosnacek <[e-mail]> wrote:
> On Sat, Oct 12, 2019 at 2:59 AM Steven Rostedt <[e-mail]> wrote:
> > From: "Steven Rostedt (VMware)" <[e-mail]>
> >
> > If on boot up, lockdown is activated for tracefs, don't even bother creating
> > the files. This can also prevent instances from being created if lockdown is
> > in effect.
> >
> > Link: http://lkml.kernel.org/r/CAHk-=whC6Ji=[e-mail]
> >
> > Suggested-by: Linus Torvalds <[e-mail]>
> > Signed-off-by: Steven Rostedt (VMware) <[e-mail]>
> > ---
> >  fs/tracefs/inode.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/fs/tracefs/inode.c b/fs/tracefs/inode.c
> > index eeeae0475da9..0caa151cae4e 100644
> > --- a/fs/tracefs/inode.c
> > +++ b/fs/tracefs/inode.c
> > @@ -16,6 +16,7 @@
> >  #include <linux/namei.h>
> >  #include <linux/tracefs.h>
> >  #include <linux/fsnotify.h>
> > +#include <linux/security.h>
> >  #include <linux/seq_file.h>
> >  #include <linux/parser.h>
> >  #include <linux/magic.h>
> > @@ -390,6 +391,9 @@ struct dentry *tracefs_create_file(const char *name, umode_t mode,
> >         struct dentry *dentry;
> >         struct inode *inode;
> >
> > +       if (security_locked_down(LOCKDOWN_TRACEFS))
> > +               return NULL;
> > +
> >         if (!(mode & S_IFMT))
> >                 mode |= S_IFREG;
> >         BUG_ON(!S_ISREG(mode));
> > --
> > 2.23.0
>
> Hi all,
>
> sorry for coming back to an old thread, but it turns out that this
> patch doesn't play well with SELinux's implementation of the
> security_locked_down() hook, which was added a few months later (so
> not your fault :) in commit 59438b46471a ("security,lockdown,selinux:
> implement SELinux lockdown").
>
> What SELinux does is it checks if the current task's creds are allowed
> the lockdown::integrity or lockdown::confidentiality permission in the
> policy whenever security_locked_down() is called. The idea is to be
> able to control at SELinux domain level which tasks can do these
> sensitive operations (when the kernel is not actually locked down by
> the Lockdown LSM).
>
> With this patch + the SELinux lockdown mechanism in use, when a
> userspace task loads a module that creates some tracefs nodes in its
> initcall SELinux will check if the task has the
> lockdown::confidentiality permission and if not, will report denials
> in audit log and prevent the tracefs entries from being created. But
> that is not a very logical behavior, since the task loading the module
> is itself not (explicitly) doing anything that would breach
> confidentiality. It just indirectly causes some tracefs nodes to be
> created, but doesn't actually use them at that point.
>
> Since it seems the other patches also added security_locked_down()
> calls to the tracefs nodes' open functions, I guess reverting this
> patch could be an acceptable way to fix this problem (please correct
> me if there is something that this call catches, which the other ones
> don't). However, even then I can understand that you (or someone else)
> might want to keep this as an optimization, in which case we could
> instead do this:
> 1. Add a new hook security_locked_down_permanently() (the name is open
> for discussion), which would be intended for situations when we want
> to avoid doing some pointless work when the kernel is in a "hard"
> lockdown that can't be taken back (except perhaps in some rescue
> scenario...).
> 2. This hook would be backed by the same implementation as
> security_locked_down() in the Lockdown LSM and left unimplemented by
> SELinux.
> 3. tracefs_create_file() would call this hook instead of security_locked_down().
>
> This way it would work as before relative to the standard lockdown via
> the Lockdown LSM and would be simply ignored by SELinux. I went over
> all the security_locked_down() call in the kernel and I think this
> alternative hook could also fit better in arch/powerpc/xmon/xmon.c,
> where it seems to be called from interrupt context (so task creds are
> irrelevant, anyway...) and mainly causes some values to be redacted.
> (I also found a couple minor issues with how the hook is used in other
> places, for which I plan to send patches later.)
>
> Thoughts?

In the meantime I found some other places where the SELinux check
should be skipped, so I went ahead and sent this patch:
https://lore.kernel.org/lkml/[e-mail]/T/

Steven Rostedt May 7, 2021, 1:26 p.m. UTC | #3

On Tue, 13 Apr 2021 10:13:04 +0200
Ondrej Mosnacek <[e-mail]> wrote:

> Thoughts?

I never enable the lockdown feature for tracefs, so I'll leave it up to the
other people that do (and that care about this code) to answer.

-- Steve

Patch

diff --git a/fs/tracefs/inode.c b/fs/tracefs/inode.c
index eeeae0475da9..0caa151cae4e 100644
--- a/fs/tracefs/inode.c
+++ b/fs/tracefs/inode.c
@@ -16,6 +16,7 @@ 
 #include <linux/namei.h>
 #include <linux/tracefs.h>
 #include <linux/fsnotify.h>
+#include <linux/security.h>
 #include <linux/seq_file.h>
 #include <linux/parser.h>
 #include <linux/magic.h>
@@ -390,6 +391,9 @@  struct dentry *tracefs_create_file(const char *name, umode_t mode,
 	struct dentry *dentry;
 	struct inode *inode;
 
+	if (security_locked_down(LOCKDOWN_TRACEFS))
+		return NULL;
+
 	if (!(mode & S_IFMT))
 		mode |= S_IFREG;
 	BUG_ON(!S_ISREG(mode));