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

[Ondrej Mosnacek]

On Tue, Apr 13, 2021 at 10:13 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote:
> On Sat, Oct 12, 2019 at 2:59 AM Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
> > From: "Steven Rostedt (VMware)" <rostedt@xxxxxxxxxxx>
> >
> > 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=fWnjh2+eS4b15TnbsS4VPVtvBOwCy1jjEG_JHQ@xxxxxxxxxxxxxx
> >
> > Suggested-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Steven Rostedt (VMware) <rostedt@xxxxxxxxxxx>
> > ---
> >  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/20210507114048.138933-1-omosnace@xxxxxxxxxx/T/

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