linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [GIT PULL] SELinux fixes for v5.15 (#1)
@ 2021-09-17  1:14 Paul Moore
  2021-09-22 17:57 ` Paul Moore
  0 siblings, 1 reply; 10+ messages in thread
From: Paul Moore @ 2021-09-17  1:14 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: selinux, linux-security-module, linux-kernel

Hi Linus,

A single patch to address some issues with the incorrect subject being
used in some of the SELinux lockdown access controls.  You saw, and
joined the discussion, earlier versions of this patch that included
the related BPF changes; the BPF changes have already been merged,
this patch has all the remainders.  Beyond that, the commit
description is pretty good so if you are interested in more detail I
would suggest reading that first.

Please merge for the next v5.15-rcX release, thank you.
-Paul

--
The following changes since commit 6880fa6c56601bb8ed59df6c30fd390cc5f6dd8f:

 Linux 5.15-rc1 (2021-09-12 16:28:37 -0700)

are available in the Git repository at:

 git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git
   tags/selinux-pr-20210916

for you to fetch changes up to fdc9cbff7a764513a5e72a03b796087fcadb2fa3:

 lockdown,selinux: fix wrong subject in some SELinux lockdown checks
   (2021-09-16 21:04:44 -0400)

----------------------------------------------------------------
selinux/stable-5.15 PR 20210916

----------------------------------------------------------------
Ondrej Mosnacek (1):
     lockdown,selinux: fix wrong subject in some SELinux lockdown checks

arch/powerpc/xmon/xmon.c             |  4 ++--
arch/x86/kernel/ioport.c             |  4 ++--
arch/x86/kernel/msr.c                |  4 ++--
arch/x86/mm/testmmiotrace.c          |  2 +-
drivers/acpi/acpi_configfs.c         |  2 +-
drivers/acpi/custom_method.c         |  2 +-
drivers/acpi/osl.c                   |  3 ++-
drivers/acpi/tables.c                |  2 +-
drivers/char/mem.c                   |  2 +-
drivers/cxl/pci.c                    |  2 +-
drivers/firmware/efi/efi.c           |  2 +-
drivers/firmware/efi/test/efi_test.c |  2 +-
drivers/pci/pci-sysfs.c              |  6 +++---
drivers/pci/proc.c                   |  6 +++---
drivers/pci/syscall.c                |  2 +-
drivers/pcmcia/cistpl.c              |  2 +-
drivers/tty/serial/serial_core.c     |  2 +-
fs/debugfs/file.c                    |  2 +-
fs/debugfs/inode.c                   |  2 +-
fs/proc/kcore.c                      |  2 +-
fs/tracefs/inode.c                   |  2 +-
include/linux/lsm_hook_defs.h        |  2 +-
include/linux/lsm_hooks.h            |  1 +
include/linux/security.h             |  5 +++--
kernel/bpf/helpers.c                 | 10 ++++++----
kernel/events/core.c                 |  2 +-
kernel/kexec.c                       |  2 +-
kernel/kexec_file.c                  |  2 +-
kernel/module.c                      |  2 +-
kernel/params.c                      |  2 +-
kernel/power/hibernate.c             |  2 +-
kernel/trace/bpf_trace.c             | 25 +++++++++++++++----------
kernel/trace/ftrace.c                |  4 ++--
kernel/trace/ring_buffer.c           |  2 +-
kernel/trace/trace.c                 | 10 +++++-----
kernel/trace/trace_events.c          |  2 +-
kernel/trace/trace_events_hist.c     |  4 ++--
kernel/trace/trace_events_synth.c    |  2 +-
kernel/trace/trace_events_trigger.c  |  2 +-
kernel/trace/trace_kprobe.c          |  6 +++---
kernel/trace/trace_printk.c          |  2 +-
kernel/trace/trace_stack.c           |  2 +-
kernel/trace/trace_stat.c            |  2 +-
kernel/trace/trace_uprobe.c          |  4 ++--
net/xfrm/xfrm_user.c                 | 11 +++++++++--
security/lockdown/lockdown.c         |  3 ++-
security/security.c                  |  4 ++--
security/selinux/hooks.c             |  7 +++++--
48 files changed, 100 insertions(+), 79 deletions(-)

-- 
paul moore
www.paul-moore.com

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

* Re: [GIT PULL] SELinux fixes for v5.15 (#1)
  2021-09-17  1:14 [GIT PULL] SELinux fixes for v5.15 (#1) Paul Moore
@ 2021-09-22 17:57 ` Paul Moore
  2021-09-22 20:55   ` Linus Torvalds
  0 siblings, 1 reply; 10+ messages in thread
From: Paul Moore @ 2021-09-22 17:57 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: selinux, linux-security-module, linux-kernel

On Thu, Sep 16, 2021 at 9:14 PM Paul Moore <paul@paul-moore.com> wrote:
>
> Hi Linus,
>
> A single patch to address some issues with the incorrect subject being
> used in some of the SELinux lockdown access controls.  You saw, and
> joined the discussion, earlier versions of this patch that included
> the related BPF changes; the BPF changes have already been merged,
> this patch has all the remainders.  Beyond that, the commit
> description is pretty good so if you are interested in more detail I
> would suggest reading that first.
>
> Please merge for the next v5.15-rcX release, thank you.
> -Paul

I wanted to check in on this PR to see if you were planning on merging
it for v5.15-rcX, kicking it back for -next instead, or simply glaring
at it with quiet disgust?

> --
> The following changes since commit 6880fa6c56601bb8ed59df6c30fd390cc5f6dd8f:
>
>  Linux 5.15-rc1 (2021-09-12 16:28:37 -0700)
>
> are available in the Git repository at:
>
>  git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git
>    tags/selinux-pr-20210916
>
> for you to fetch changes up to fdc9cbff7a764513a5e72a03b796087fcadb2fa3:
>
>  lockdown,selinux: fix wrong subject in some SELinux lockdown checks
>    (2021-09-16 21:04:44 -0400)
>
> ----------------------------------------------------------------
> selinux/stable-5.15 PR 20210916
>
> ----------------------------------------------------------------
> Ondrej Mosnacek (1):
>      lockdown,selinux: fix wrong subject in some SELinux lockdown checks
>
> arch/powerpc/xmon/xmon.c             |  4 ++--
> arch/x86/kernel/ioport.c             |  4 ++--
> arch/x86/kernel/msr.c                |  4 ++--
> arch/x86/mm/testmmiotrace.c          |  2 +-
> drivers/acpi/acpi_configfs.c         |  2 +-
> drivers/acpi/custom_method.c         |  2 +-
> drivers/acpi/osl.c                   |  3 ++-
> drivers/acpi/tables.c                |  2 +-
> drivers/char/mem.c                   |  2 +-
> drivers/cxl/pci.c                    |  2 +-
> drivers/firmware/efi/efi.c           |  2 +-
> drivers/firmware/efi/test/efi_test.c |  2 +-
> drivers/pci/pci-sysfs.c              |  6 +++---
> drivers/pci/proc.c                   |  6 +++---
> drivers/pci/syscall.c                |  2 +-
> drivers/pcmcia/cistpl.c              |  2 +-
> drivers/tty/serial/serial_core.c     |  2 +-
> fs/debugfs/file.c                    |  2 +-
> fs/debugfs/inode.c                   |  2 +-
> fs/proc/kcore.c                      |  2 +-
> fs/tracefs/inode.c                   |  2 +-
> include/linux/lsm_hook_defs.h        |  2 +-
> include/linux/lsm_hooks.h            |  1 +
> include/linux/security.h             |  5 +++--
> kernel/bpf/helpers.c                 | 10 ++++++----
> kernel/events/core.c                 |  2 +-
> kernel/kexec.c                       |  2 +-
> kernel/kexec_file.c                  |  2 +-
> kernel/module.c                      |  2 +-
> kernel/params.c                      |  2 +-
> kernel/power/hibernate.c             |  2 +-
> kernel/trace/bpf_trace.c             | 25 +++++++++++++++----------
> kernel/trace/ftrace.c                |  4 ++--
> kernel/trace/ring_buffer.c           |  2 +-
> kernel/trace/trace.c                 | 10 +++++-----
> kernel/trace/trace_events.c          |  2 +-
> kernel/trace/trace_events_hist.c     |  4 ++--
> kernel/trace/trace_events_synth.c    |  2 +-
> kernel/trace/trace_events_trigger.c  |  2 +-
> kernel/trace/trace_kprobe.c          |  6 +++---
> kernel/trace/trace_printk.c          |  2 +-
> kernel/trace/trace_stack.c           |  2 +-
> kernel/trace/trace_stat.c            |  2 +-
> kernel/trace/trace_uprobe.c          |  4 ++--
> net/xfrm/xfrm_user.c                 | 11 +++++++++--
> security/lockdown/lockdown.c         |  3 ++-
> security/security.c                  |  4 ++--
> security/selinux/hooks.c             |  7 +++++--
> 48 files changed, 100 insertions(+), 79 deletions(-)

-- 
paul moore
www.paul-moore.com

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

* Re: [GIT PULL] SELinux fixes for v5.15 (#1)
  2021-09-22 17:57 ` Paul Moore
@ 2021-09-22 20:55   ` Linus Torvalds
  2021-09-22 21:10     ` Linus Torvalds
  0 siblings, 1 reply; 10+ messages in thread
From: Linus Torvalds @ 2021-09-22 20:55 UTC (permalink / raw)
  To: Paul Moore; +Cc: SElinux list, LSM List, Linux Kernel Mailing List

On Wed, Sep 22, 2021 at 10:57 AM Paul Moore <paul@paul-moore.com> wrote:
>
> I wanted to check in on this PR to see if you were planning on merging
> it for v5.15-rcX, kicking it back for -next instead, or simply glaring
> at it with quiet disgust?

Heh. I glanced at it with quiet disgust when it came in, and then it
just got lost.

But now I'm looking at it again since you reminded me, and I don't
understand why it has to be done in such an ugly manner.

And I also don't think the thing makes a whit of sense in the first place.

Honestly, that whole "lockdown depends on current creds" makes NO
SENSE at all to me. The whole and only point about lockdown is that
it's locked down. Not that "oh, root can do X". That's against the
point.

The whole and only point of lockdown was as a global thing. Now
somebody seems to have noticed that they violated that basic rule, and
that it's about permissions after all, and it's just a complete mess.

But why the heck would a normal lockdown user have to care about this
fundamental design mistake? Why should a normal lockdown user pass in
a credential that doesn't make sense?

Since 99% of all users DON'T want special rules, why isn't the normal
security_locked_down() kept the way it is?

Make the few special cases do special things, in other words.

This is *literally* why we have all those wrapper functions in
security/security.c - so that people can do sane interfaces and not
call down to the raw hooks.

(Yeah, yeah, a lot of them do nothing but pass it down, but others do
other sanity stuff so that callers don't have to do pointless
boilerplate)

IOW, why is this changing all the normal users that *really* don't
want it, and that really have absolutely no business looking up the
current creds?

Make the regular security_locked_down() function do that, and add a

    if (WARN_ON_ONCE(!in_task()))
        return -EPERM;

so that any bad cases get flagged and refuse to continue.

And then the couple of special users (whether due to interrupt context
or whatever), could get their own wrapper function.

Note how that would
 (a) make the patch smaller
 (b) not pollute normal users pointlessly
 (c) actually be a kind of documentation too
 (d) not make the default lockdown testing function be senseless

because right now you have absolutely no explanation in the crazy
cases, so y9ou have code like this:

        lockdown = !!security_locked_down(NULL, LOCKDOWN_XMON_RW);

in xmon_is_locked_down(), and it makes no sense to anybody. Why the
NULL? What is going on?

Yes, yes, I can tell why the NULL, and what is going on, because I
read the commit message and it's in my context. But look at that line,
and tell me that it makes sense as code to anybody who has paged out
that context (or never had it in the first place).

If it said "security_globally_locked_down()" maybe it would at least
give a hint about what's going on.

But the other side of the argument is that the *common* lockdown
functions are insane too after the patch. In kernel/module.c, this
line:

        return security_locked_down(current_cred(), LOCKDOWN_MODULE_SIGNATURE);

really screams "lockdown is fundamentally broken and mis-designed".

Seriously. If lockdown needs "current creds" then lockdown is wrong.

I was unhappy about lockdown from before, so maybe I'm more likely to
just reject this kind of garbage, but this really looks completely
broken to me.

Hmm? This lockdown stuff is some ugly random code to begin with, I
think the interfaces need to make SENSE. Passing in credentials
fundamentally does not make sense to me.

              Linus

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

* Re: [GIT PULL] SELinux fixes for v5.15 (#1)
  2021-09-22 20:55   ` Linus Torvalds
@ 2021-09-22 21:10     ` Linus Torvalds
  2021-09-22 21:40       ` Paul Moore
  0 siblings, 1 reply; 10+ messages in thread
From: Linus Torvalds @ 2021-09-22 21:10 UTC (permalink / raw)
  To: Paul Moore; +Cc: SElinux list, LSM List, Linux Kernel Mailing List

On Wed, Sep 22, 2021 at 1:55 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Make the regular security_locked_down() function do that, and add a
>
>     if (WARN_ON_ONCE(!in_task()))
>         return -EPERM;
>
> so that any bad cases get flagged and refuse to continue.

Actually, no, I take that back.

It's not the "!in_task()" case that is the problem. That's just the symptom.

The real problem is that we clearly have some lock-down rule that
seems to care about credentials and who it is that does the lockdown
query. That seems to be the real issue here. Doing lockdown checks
from interrupts should be fine.

The security layer really seems to be doing odd and random things.
Maybe we should have some debug switch with the above WARN_ON() inside
current_cred() (and a number of other 'current' users too). Just to
see if there are other cases where people look up basically random
credentials.

When the security layer is confused, that's not a good sign.

                 Linus

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

* Re: [GIT PULL] SELinux fixes for v5.15 (#1)
  2021-09-22 21:10     ` Linus Torvalds
@ 2021-09-22 21:40       ` Paul Moore
  2021-09-22 23:43         ` Linus Torvalds
  0 siblings, 1 reply; 10+ messages in thread
From: Paul Moore @ 2021-09-22 21:40 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: SElinux list, LSM List, Linux Kernel Mailing List

On Wed, Sep 22, 2021 at 5:10 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Wed, Sep 22, 2021 at 1:55 PM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > Make the regular security_locked_down() function do that, and add a
> >
> >     if (WARN_ON_ONCE(!in_task()))
> >         return -EPERM;
> >
> > so that any bad cases get flagged and refuse to continue.
>
> Actually, no, I take that back.
>
> It's not the "!in_task()" case that is the problem. That's just the symptom.
>
> The real problem is that we clearly have some lock-down rule that
> seems to care about credentials and who it is that does the lockdown
> query. That seems to be the real issue here. Doing lockdown checks
> from interrupts should be fine.

The basic idea, or problem from a LSM point of view, is that in some
cases you have a user task which is doing the lockdown access check
and in others you have the kernel itself; the creds parameter to
security_locked_down() hook was intended to be used to indicate if it
was a user task (param == current_cred()) or the kernel (param ==
NULL).  There was a discussion about using two different hooks/funcs,
e.g. security_locked_down() and security_locked_down_kern(), instead
of the creds parameter, but there were more votes for the param
variant.

As I type this I'm trying to muster something other than indifference
towards this patch, but the reality is I just want to be done with it.
If you'll merge a revision of this patch that does away with the cred
parameter and goes with the two hooks I'm not going to argue against
it.

During the review of the latest draft of this patch I half-jokingly
said it was cursed, perhaps it's time to honestly consider it cursed.

-- 
paul moore
www.paul-moore.com

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

* Re: [GIT PULL] SELinux fixes for v5.15 (#1)
  2021-09-22 21:40       ` Paul Moore
@ 2021-09-22 23:43         ` Linus Torvalds
  2021-09-23 15:43           ` Paul Moore
  0 siblings, 1 reply; 10+ messages in thread
From: Linus Torvalds @ 2021-09-22 23:43 UTC (permalink / raw)
  To: Paul Moore; +Cc: SElinux list, LSM List, Linux Kernel Mailing List

On Wed, Sep 22, 2021 at 2:40 PM Paul Moore <paul@paul-moore.com> wrote:
>
> The basic idea, or problem from a LSM point of view, is that in some
> cases you have a user task which is doing the lockdown access check
> and in others you have the kernel itself

I don't understand. In that case, it would be a boolean for "kernel vs user".

But that's not what it is. It literally seems to care about _which_
user, and looks at cred_sid().

This is what makes no sense to me. If it's about lockdown,. then the
user is immaterial. Either it's locked down, or it's not.

Yeah, yeah, clearly that isn't how it works. Something is very rotten
in the state of lockdown. But that rottenness shouldn't then be
exposed as a horrible interface.

Why has selinux allowed the SID to be an issue for SECCLASS_LOCKDOWN at all?

And why is selinux foceing it's very odd and arguably completely
misguided "lockdown" logic onto the security layer?

Yes, using "current_sid()" in selinux_lockdown() is clearly wrong,
since it's not sensible in an interrupt, but lockdown questions are.

But why isn't that just considered a selinux bug, and that

        u32 sid = current_sid();

just replaced with something silly like

        // lockdown is lockdown, user labeling isn't relevant
        u32 sid = SECINITSID_UNLABELED;

and solve that issue that way? Just say that lockdown cannot depend on
who is asking for it.

         Linus

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

* Re: [GIT PULL] SELinux fixes for v5.15 (#1)
  2021-09-22 23:43         ` Linus Torvalds
@ 2021-09-23 15:43           ` Paul Moore
  2021-09-23 15:53             ` Linus Torvalds
  0 siblings, 1 reply; 10+ messages in thread
From: Paul Moore @ 2021-09-23 15:43 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: SElinux list, LSM List, Linux Kernel Mailing List

On Wed, Sep 22, 2021 at 7:43 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Wed, Sep 22, 2021 at 2:40 PM Paul Moore <paul@paul-moore.com> wrote:
> >
> > The basic idea, or problem from a LSM point of view, is that in some
> > cases you have a user task which is doing the lockdown access check
> > and in others you have the kernel itself
>
> I don't understand. In that case, it would be a boolean for "kernel vs user".
>
> But that's not what it is. It literally seems to care about _which_
> user, and looks at cred_sid().

Well, yes, it does look at the credential if it is passed; I guess I
wrongly assumed that was understood.  If it was just a simple
user/kernel decision then yes, it would be a boolean (or similar).

> This is what makes no sense to me. If it's about lockdown,. then the
> user is immaterial. Either it's locked down, or it's not.

If all you have is the lockdown LSM, then yes, lockdown doesn't take
into account the context of the request, it is simply a test of the
lockdown threshold: only disclosures on the proper side of the
lockdown value are allowed.

However, we have the LSM framework because there is never one way to
solve a problem, and the LSM hooks have always changed to support
these different approaches to access control.  While the lockdown LSM
takes a context-free approach to enforcing the lockdown setting, the
SELinux LSM takes a different enforcement approach which not only
better integrates with the SELinux policy, but it offers new
functionality beyond the lockdown LSM:

* Access based on the integrity and confidentiality reasons can be
specified independently with SELinux.

* Provide the ability to define the lockdown level within the context
of individual security domains.

It's also worth noting that with LSM stacking and the combination of
the lockdown and SELinux LSMs, the SELinux lockdown controls would not
grant any additional disclosures beyond what the lockdown LSM would
allow, the SELinux controls would only further restrict the disclosure
of specific security domains as specified in the SELinux policy.

--
paul moore
www.paul-moore.com

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

* Re: [GIT PULL] SELinux fixes for v5.15 (#1)
  2021-09-23 15:43           ` Paul Moore
@ 2021-09-23 15:53             ` Linus Torvalds
  2021-09-23 17:13               ` Paul Moore
  0 siblings, 1 reply; 10+ messages in thread
From: Linus Torvalds @ 2021-09-23 15:53 UTC (permalink / raw)
  To: Paul Moore; +Cc: SElinux list, LSM List, Linux Kernel Mailing List

On Thu, Sep 23, 2021 at 8:43 AM Paul Moore <paul@paul-moore.com> 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.

SELinux did something wrong. Stop doing it. Stop sending patches to
then screw up the generic security layer, and violate the rules under
which these patches were accepted.

We have now this week have two discussions about the selinux doing
completely invalid and incorrect things, and both were related to just
thinking that it's ok to just randomly access thread data.

At some point, you just have to look at the SELinux code and say
:"this does something wrong".

Instead of this kind of "no, everybody else is wrong, I will modify
them to do what I mistakenly did".

IOW, just make the patch be

   diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
   index 6517f221d52c..4e93bf5dc8ef 100644
   --- a/security/selinux/hooks.c
   +++ b/security/selinux/hooks.c
   @@ -7016,7 +7016,8 @@ static void selinux_bpf_prog_free(struct
bpf_prog_aux *aux)
    static int selinux_lockdown(enum lockdown_reason what)
    {
        struct common_audit_data ad;
   -    u32 sid = current_sid();
   +    /* Lockdown requests come in non-thread context, can't use
'current_sid()' */
   +    u32 sid = SECINITSID_UNLABELED;
        int invalid_reason = (what <= LOCKDOWN_NONE) ||
                             (what == LOCKDOWN_INTEGRITY_MAX) ||
                             (what >= LOCKDOWN_CONFIDENTIALITY_MAX);

and stop accessing random security ID's from random contexts.

And stop thinking it's ok for SELinux to just do bad things.

               Linus

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

* Re: [GIT PULL] SELinux fixes for v5.15 (#1)
  2021-09-23 15:53             ` Linus Torvalds
@ 2021-09-23 17:13               ` Paul Moore
  2021-09-23 17:29                 ` Linus Torvalds
  0 siblings, 1 reply; 10+ messages in thread
From: Paul Moore @ 2021-09-23 17:13 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: SElinux list, LSM List, Linux Kernel Mailing List

On Thu, Sep 23, 2021 at 11:53 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Thu, Sep 23, 2021 at 8:43 AM Paul Moore <paul@paul-moore.com> 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

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

* Re: [GIT PULL] SELinux fixes for v5.15 (#1)
  2021-09-23 17:13               ` Paul Moore
@ 2021-09-23 17:29                 ` Linus Torvalds
  0 siblings, 0 replies; 10+ messages in thread
From: Linus Torvalds @ 2021-09-23 17:29 UTC (permalink / raw)
  To: Paul Moore; +Cc: SElinux list, LSM List, Linux Kernel Mailing List

On Thu, Sep 23, 2021 at 10:13 AM Paul Moore <paul@paul-moore.com> wrote:
>
> It's become clear you *really* don't like passing the cred pointer
> here, presumably based on a very specific security model for lockdown.

Not just that. I'm sensitive about the cred pointer because of the
timing, but I'm also sensitive about just looking at code and saying
"that makes no sense".

Having callers do pointless things that make no sense to the callers
is a bad thing. Having a patch where 59 out of 63 callers just
mindlessly pass in "current_cred()", and then remaining three callers
pass in NULL for NO DISCERNIBLE REASON is a sign of just a bad
interface.

And I realize that to you, these interfaces are what you do.

To everybody else, it's completely mindless noise that makes no sense
at all, because there's no _logic_ to it. It's random LSM calls in
random contexts that have no clear reason for them, because the
"reason" is hidden in some odd SELinux rule that no kernel developer
even knows about.

You can't even grep for the use of it, because there is none. It's
literally randomness. Which means that it _has_ to make conceptual
sense to be at all maintainable.

So when I see a diffstat where basically 90% of the patch has
ABSOLUTELY NOTHING to do with SElinux or anything you maintain, and
that 90% makes absolutely no sense to begin with, guess what? I think
that patch is bad.

When I then look closer, and see that the _reason_ for the patch is
that SElinux - yet again - incorrectly accessed process credentials in
random contexts, I just do "this is not just a bad patch, this is
actually a fundamental problem with SELinux".

See where I'm coming from? The "specific security model for lockdown"
is just the high-level view of what made sense. You violated that view
for bad reasons, with a bad patch, and with bad timing.

This isn't _just_ a SElinux problem. I think it's a LSM problem in
general. Lots of those random callbacks are completely
incomprehensible, have odd rules, and the LSM people aren't even
trying to have them make sense.

As long as the oddity is contained, that's one thing. But when it is
made this obvious and affects random code in strange places in the
kernel, and it's for a feature that had a lot of discussion even when
it got merged, I'm putting my foot down.

             Linus

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

end of thread, other threads:[~2021-09-23 17:29 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-17  1:14 [GIT PULL] SELinux fixes for v5.15 (#1) Paul Moore
2021-09-22 17:57 ` Paul Moore
2021-09-22 20:55   ` Linus Torvalds
2021-09-22 21:10     ` Linus Torvalds
2021-09-22 21:40       ` Paul Moore
2021-09-22 23:43         ` Linus Torvalds
2021-09-23 15:43           ` Paul Moore
2021-09-23 15:53             ` Linus Torvalds
2021-09-23 17:13               ` Paul Moore
2021-09-23 17:29                 ` Linus Torvalds

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).