Re: [GIT PULL] SELinux fixes for v5.15 (#1)

[Paul Moore]

On Thu, Sep 23, 2021 at 11:53 AM Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Thu, Sep 23, 2021 at 8:43 AM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> >
> > However, we have the LSM framework because there is never one way to
> > solve a problem,
>
> The thing is, the lockdown patches were merged because they were allegedly sane.
>
> As far as I can tell, this is purely a SELinux internal bug.

I'm not sure why that matters, but sure.  There is a problem, we are
working to fix it.

> SELinux did something wrong. Stop doing it.

We *are* trying to fix the problem.  Please stop pretending we are
not.  You can certainly disagree with our approach, but I'm getting
tired of the chastising where you imply we are actively trying to
screw things up or do bad things.  We care about the kernel.  We care
a lot.  I care more than I probably should, and probably more than is
healthy at times.  You can disagree with the design and/or
implementation, but making claims that we are "thinking it's ok for
SELinux to just do bad things." is just plain wrong not to mention
insulting.

> Stop sending patches to
> then screw up the generic security layer, and violate the rules under
> which these patches were accepted.

*Sigh*

There was plenty of discussion about this patch and the previous
drafts, no one was overly upset about adding the caller context/cred
info.  The other LSM folks were okay with it.  We've got plenty of
historical examples of the LSM hook evolving over time to adapt to
LSMs adding new functionality, new LSMs, and new kernel modules.  So
let's look at why you are shouting about "screwing up the generic
security layer", but let's try to keep the focus at the LSM interface
level.

Prior to this patch there was one relevant LSM hook for lockdown:

  int security_locked_down(enum lockdown_reason what);

... the patch in this PR changed it to this:

  int security_locked_down(const struct cred *cred,
                           enum lockdown_reason what);

It's become clear you *really* don't like passing the cred pointer
here, presumably based on a very specific security model for lockdown.
At this point there are two thoughts that spring to mind: 1) how else
can we enable the SELinux model that we want to implement and 2) why
is the LSM forcing a single security model on LSMs for the lockdown
hook?

Let's deal with the first point first.  If you aren't going to merge a
change to the LSM framework that allows for the context credentials,
would you be willing to merge a new LSM hook that is used in place of
the existing lockdown hook for callers that are not associated with a
user task?  Both hooks would take a single lockdown_reason as the only
argument and would look something like this:

  int security_locked_down(enum lockdown_reason what);
  int security_locked_down_kern(enum lockdown_reason what);

There is already precedence in the kernel as a whole for LSM hooks
that exist solely for kernel (non-user tasks) operations so this
wouldn't be a big stretch.  LSMs that don't care to make a distinction
between the two, e.g. the existing lockdown LSM, could set the LSM
hook to point to the same function (in the case of lockdown this would
be lockdown_is_locked_down()).

However, if the above doesn't fly, let's move on to the second thought
I mentioned above: why is the LSM forcing a single security model on
LSMs for the lockdown hook?  If the lockdown functionality is really
going to be restricted to just a single security model, why is it
implemented as a LSM and not as core kernel functionality?  The
original motivation for the LSM was that the kernel needed an
abstraction layer to support multiple security models and we've seen
it do just that over the years; SELinux may have been the first, but
the number has certainly grown over the years and the LSM framework
has evolved right along with it.  Putting restrictions on the LSM
framework so that only specific security models could be implemented
is not something we have really done, and at this point I think it
would be a major mistake.

--
paul moore
www.paul-moore.com