* Re: [PATCH] LSM: allow an LSM to disable all hooks at once
From: Paul Moore @ 2019-12-13 23:23 UTC (permalink / raw)
To: Ondrej Mosnacek
Cc: Stephen Smalley, Casey Schaufler, Kees Cook,
Linux Security Module list, James Morris, Serge E. Hallyn,
SElinux list, John Johansen, Micah Morton, Tetsuo Handa
In-Reply-To: <CAFqZXNv7PUgrU9Qe28e3cHnRAwjKZLVmNrOZggvkE5AB7T9o1Q@mail.gmail.com>
On Fri, Dec 13, 2019 at 9:07 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> On Thu, Dec 12, 2019 at 8:01 PM Paul Moore <paul@paul-moore.com> wrote:
> > On Thu, Dec 12, 2019 at 1:48 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> > > On 12/12/19 1:18 PM, Paul Moore wrote:
> > > > On Thu, Dec 12, 2019 at 1:10 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> > > >>
> > > >> On 12/12/19 1:09 PM, Paul Moore wrote:
> > > >>> On Thu, Dec 12, 2019 at 12:57 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> > > >>>> On 12/12/19 12:54 PM, Paul Moore wrote:
> > > >>>>> On Thu, Dec 12, 2019 at 8:14 AM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> > > >>>>>> On 12/12/19 6:49 AM, Ondrej Mosnacek wrote:
> > > >>>>>>> On Wed, Dec 11, 2019 at 8:12 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> > > >>>>>>>> On 12/11/19 1:35 PM, Casey Schaufler wrote:
> > > >>>>>>>>> On 12/11/2019 8:42 AM, Kees Cook wrote:
> > > >>>>>>>>>> On Wed, Dec 11, 2019 at 09:29:10AM -0500, Stephen Smalley wrote:
> > > >>>>>>>>>>> On 12/11/19 9:08 AM, Ondrej Mosnacek wrote:
> > > >>>>>
> > > >>>>> ...
> > > >>>>>
> > > >>>>>>>> selinux_state.initialized reflects whether a policy has
> > > >>>>>>>> been loaded. With a few exceptions in certain hook functions, it is
> > > >>>>>>>> only checked by the security server service functions
> > > >>>>>>>> (security/selinux/ss/services.c) prior to accessing the policydb. So
> > > >>>>>>>> there is a lot of SELinux processing that would still occur in that
> > > >>>>>>>> situation unless we added if (!selinux_state.initialized) return 0;
> > > >>>>>>>> checks to all the hook functions, which would create the same exposure
> > > >>>>>>>> and would further break the SELinux-enabled case (we need to perform
> > > >>>>>>>> some SELinux processing pre-policy-load to allocate blobs and track what
> > > >>>>>>>> tasks and objects require delayed security initialization when policy
> > > >>>>>>>> load finally occurs).
> > > >>>>>>>
> > > >>>>>>> I think what Casey was suggesting is to add another flag that would
> > > >>>>>>> switch from "no policy loaded, but we expect it to be loaded
> > > >>>>>>> eventually" to "no policy loaded and we don't expect/allow it to be
> > > >>>>>>> loaded any more", which is essentially equivalent to checking
> > > >>>>>>> selinux_enabled in each hook, which you had already brought up.
> > > >>>>>>
> > > >>>>>> Yep. if (!selinux_enabled) return 0; or if (selinux_state.disabled)
> > > >>>>>> return 0; under #ifdef CONFIG_SECURITY_SELINUX_DISABLE in every hook
> > > >>>>>> might be the best option until it can be removed altogether; avoids
> > > >>>>>> impacting the LSM framework or any other security module, preserves the
> > > >>>>>> existing functionality, fairly low overhead on the SELinux-disabled case.
> > > >>>>>
> > > >>>>> Just so I'm understanding this thread correctly, the above change
> > > >>>>> (adding enabled checks to each SELinux hook implementation) is only
> > > >>>>> until Fedora can figure out a way to deprecate and remove the runtime
> > > >>>>> disable?
> > > >>>>
> > > >>>> That's my understanding. In the interim, Android kernels should already
> > > >>>> be disabling CONFIG_SECURITY_SELINUX_DISABLE and other distros may
> > > >>>> choose to disable it as long as they don't care about supporting SELinux
> > > >>>> runtime disable.
> > > >>>
> > > >>> Okay, I just wanted to make sure I wasn't missing something.
> > > >>> Honestly, I'd rather Fedora just go ahead and do whatever it is they
> > > >>> need to do to turn off CONFIG_SECURITY_SELINUX_DISABLE (it sounds like
> > > >>> they have a plan and are working on it), I'm not overly excited about
> > > >>> temporarily cluttering up the code with additional "enabled" checks
> > > >>> when the status quo works, even if it is less than ideal.
> > > >>
> > > >> The status quo is producing kernel crashes, courtesy of LSM stacking
> > > >> changes...
> > > >
> > > > How prevalent are these crashes?
> > > >
> > > > This also only happens when disabling SELinux at runtime, yes?
> > > > Something we've advised against for some time now and are working to
> > > > eliminate? Let's just get rid of the runtime disable *soon*, and if
> > > > we need a stop-gap fix let's just go with the hook reordering since
> > > > that seems to minimize the impact, if not resolve it.
> > >
> > > Not optimistic given that the original bug was opened 2.5+ years ago,
> > > commenters identified fairly significant challenges to removing the
> > > support, and no visible progress was ever made. I suppose the hook
> > > reordering will do but seems fragile and hacky. Whatever.
> >
> > Based on Ondrej's comments in this thread it sounded as if there was
> > some renewed interest in actually eliminating it from Fedora this
> > time. If that's not the case, perhaps it's time to start that effort
> > back up, and if we can't ever get away from the runtime disable then
> > we can take the step of admitting that everything is terrible and we
> > can consider some of the options presented in this thread.
>
> Yes, we are quite determined to do what it takes to remove it. It may
> go more smoothly than expected, or it may get unexpectedly
> complicated. It's hard to tell at this point.
That's good to hear, I know it is going to be difficult, but I think
we can all agree it's the right thing to do. Any idea when you might
have a better idea of the time involved?
Next week I think I'm going to put together a RFC patch that marks
CONFIG_SECURITY_SELINUX_DISABLE as deprecated, and displays a warning
when it is used at runtime. Later on when we have a better idea of
the removal date, we can start adding delays when it is used to help
get people to migrate to the cmdline approach.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH v6 00/27] nfs: Mount API conversion
From: Scott Mayhew @ 2019-12-13 21:39 UTC (permalink / raw)
To: Schumaker, Anna
Cc: trond.myklebust@hammerspace.com,
linux-security-module@vger.kernel.org, linux-nfs@vger.kernel.org,
linux-fsdevel@vger.kernel.org, viro@zeniv.linux.org.uk,
linux-kernel@vger.kernel.org, dhowells@redhat.com
In-Reply-To: <498258bf630d4c2667920f21341a2a6e82a3788d.camel@netapp.com>
On Tue, 10 Dec 2019, Schumaker, Anna wrote:
> Hi Scott,
>
> On Tue, 2019-12-10 at 07:30 -0500, Scott Mayhew wrote:
> > Hi Anna, Trond,
> >
> > Here's a set of patches that converts NFS to use the mount API. Note that
> > there are a lot of preliminary patches, some from David and some from Al.
> > The final patch (the one that does the actual conversion) from the David's
> > initial posting has been split into 5 separate patches, and the entire set
> > has been rebased on top of v5.5-rc1.
>
> Thanks for the updated patches! Everything looks okay to me, but I've only
> tested with the legacy mount command. I'm curious if you've tested it using the
> new system?
I've hacked up mount.nfs for testing the new syscalls (for mounting... I
haven't quite figured out remounting yet) here:
https://github.com/scottmayhew/nfs-utils/tree/fscontext
It seems to be working okay, with one exception. If I mount the same
NFS export with the same mount options multiple times, then I get
multiple mounts:
[root@fedora30 ~]# mount.nfs nfs:/export /mnt/t
[root@fedora30 ~]# mount.nfs nfs:/export /mnt/t
[root@fedora30 ~]# grep /mnt/t /proc/mounts
nfs:/export /mnt/t nfs rw,seclabel,relatime,vers=4.2,rsize=1048576,wsize=1048576,namlen=255,hard,proto=tcp,timeo=600,retrans=2,sec=krb5,clientaddr=192.168.122.239,local_lock=none,addr=192.168.122.3 0 0
nfs:/export /mnt/t nfs rw,seclabel,relatime,vers=4.2,rsize=1048576,wsize=1048576,namlen=255,hard,proto=tcp,timeo=600,retrans=2,sec=krb5,clientaddr=192.168.122.239,local_lock=none,addr=192.168.122.3 0 0
That doesn't happen with the mount() syscall:
[root@fedora30 ~]# mount.nfs.old nfs:/export /mnt/t
[root@fedora30 ~]# mount.nfs.old nfs:/export /mnt/t
[root@fedora30 ~]# grep /mnt/t /proc/mounts
nfs:/export /mnt/t nfs rw,seclabel,relatime,vers=4.2,rsize=1048576,wsize=1048576,namlen=255,hard,proto=tcp,timeo=600,retrans=2,sec=krb5,clientaddr=192.168.122.239,local_lock=none,addr=192.168.122.3 0 0
-Scott
>
> Thanks,
> Anna
>
> >
> > Changes since v5:
> > - fixed possible derefence of error pointer in nfs4_validate_fspath()
> > reported by Dan Carpenter
> > - rebased on top of v5.5-rc1
> > Changes since v4:
> > - further split the original "NFS: Add fs_context support" patch (new
> > patch is about 25% smaller than the v4 patch)
> > - fixed NFSv4 referral mounts (broken in the original patch)
> > - fixed leak of nfs_fattr when fs_context is freed
> > Changes since v3:
> > - changed license and copyright text in fs/nfs/fs_context.c
> > Changes since v2:
> > - fixed the conversion of the nconnect= option
> > - added '#if IS_ENABLED(CONFIG_NFS_V4)' around nfs4_parse_monolithic()
> > to avoid unused-function warning when compiling with v4 disabled
> > Chagnes since v1:
> > - split up patch 23 into 4 separate patches
> >
> > -Scott
> >
> > Al Viro (15):
> > saner calling conventions for nfs_fs_mount_common()
> > nfs: stash server into struct nfs_mount_info
> > nfs: lift setting mount_info from nfs4_remote{,_referral}_mount
> > nfs: fold nfs4_remote_fs_type and nfs4_remote_referral_fs_type
> > nfs: don't bother setting/restoring export_path around
> > do_nfs_root_mount()
> > nfs4: fold nfs_do_root_mount/nfs_follow_remote_path
> > nfs: lift setting mount_info from nfs_xdev_mount()
> > nfs: stash nfs_subversion reference into nfs_mount_info
> > nfs: don't bother passing nfs_subversion to ->try_mount() and
> > nfs_fs_mount_common()
> > nfs: merge xdev and remote file_system_type
> > nfs: unexport nfs_fs_mount_common()
> > nfs: don't pass nfs_subversion to ->create_server()
> > nfs: get rid of mount_info ->fill_super()
> > nfs_clone_sb_security(): simplify the check for server bogosity
> > nfs: get rid of ->set_security()
> >
> > David Howells (8):
> > NFS: Move mount parameterisation bits into their own file
> > NFS: Constify mount argument match tables
> > NFS: Rename struct nfs_parsed_mount_data to struct nfs_fs_context
> > NFS: Split nfs_parse_mount_options()
> > NFS: Deindent nfs_fs_context_parse_option()
> > NFS: Add a small buffer in nfs_fs_context to avoid string dup
> > NFS: Do some tidying of the parsing code
> > NFS: Add fs_context support.
> >
> > Scott Mayhew (4):
> > NFS: rename nfs_fs_context pointer arg in a few functions
> > NFS: Convert mount option parsing to use functionality from
> > fs_parser.h
> > NFS: Additional refactoring for fs_context conversion
> > NFS: Attach supplementary error information to fs_context.
> >
> > fs/nfs/Makefile | 2 +-
> > fs/nfs/client.c | 80 +-
> > fs/nfs/fs_context.c | 1424 +++++++++++++++++++++++++
> > fs/nfs/fscache.c | 2 +-
> > fs/nfs/getroot.c | 73 +-
> > fs/nfs/internal.h | 132 +--
> > fs/nfs/namespace.c | 146 ++-
> > fs/nfs/nfs3_fs.h | 2 +-
> > fs/nfs/nfs3client.c | 6 +-
> > fs/nfs/nfs3proc.c | 2 +-
> > fs/nfs/nfs4_fs.h | 9 +-
> > fs/nfs/nfs4client.c | 99 +-
> > fs/nfs/nfs4file.c | 1 +
> > fs/nfs/nfs4namespace.c | 292 +++---
> > fs/nfs/nfs4proc.c | 2 +-
> > fs/nfs/nfs4super.c | 257 ++---
> > fs/nfs/proc.c | 2 +-
> > fs/nfs/super.c | 2217 +++++----------------------------------
> > include/linux/nfs_xdr.h | 9 +-
> > 19 files changed, 2287 insertions(+), 2470 deletions(-)
> > create mode 100644 fs/nfs/fs_context.c
> >
^ permalink raw reply
* Re: [PATCH] LSM: allow an LSM to disable all hooks at once
From: James Morris @ 2019-12-13 18:48 UTC (permalink / raw)
To: Ondrej Mosnacek
Cc: Tetsuo Handa, Linux Security Module list, Serge E. Hallyn,
Casey Schaufler, SElinux list, Paul Moore, Stephen Smalley,
John Johansen, Kees Cook, Micah Morton
In-Reply-To: <CAFqZXNv=AnPxHuQCusoJQGEJ8Q+yF7_TPBCRyAcE5OPzoYFc9w@mail.gmail.com>
On Thu, 12 Dec 2019, Ondrej Mosnacek wrote:
> I'd say the burden of implementing this would lie on the arms of
> whoever prepares the patches for dynamic load/unload.
Correct, and I don't see any such patches being accepted.
Go and look at some exploits, where LSM is used as a rootkit API...
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: [PATCH] LSM: allow an LSM to disable all hooks at once
From: Ondrej Mosnacek @ 2019-12-13 14:06 UTC (permalink / raw)
To: Paul Moore
Cc: Stephen Smalley, Casey Schaufler, Kees Cook,
Linux Security Module list, James Morris, Serge E. Hallyn,
SElinux list, John Johansen, Micah Morton, Tetsuo Handa
In-Reply-To: <CAHC9VhSqNGxqGQU6QfB3SxHZZdtPh79-4vOrpDXLr9zxT_X4bg@mail.gmail.com>
On Thu, Dec 12, 2019 at 8:01 PM Paul Moore <paul@paul-moore.com> wrote:
> On Thu, Dec 12, 2019 at 1:48 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> > On 12/12/19 1:18 PM, Paul Moore wrote:
> > > On Thu, Dec 12, 2019 at 1:10 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> > >>
> > >> On 12/12/19 1:09 PM, Paul Moore wrote:
> > >>> On Thu, Dec 12, 2019 at 12:57 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> > >>>> On 12/12/19 12:54 PM, Paul Moore wrote:
> > >>>>> On Thu, Dec 12, 2019 at 8:14 AM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> > >>>>>> On 12/12/19 6:49 AM, Ondrej Mosnacek wrote:
> > >>>>>>> On Wed, Dec 11, 2019 at 8:12 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> > >>>>>>>> On 12/11/19 1:35 PM, Casey Schaufler wrote:
> > >>>>>>>>> On 12/11/2019 8:42 AM, Kees Cook wrote:
> > >>>>>>>>>> On Wed, Dec 11, 2019 at 09:29:10AM -0500, Stephen Smalley wrote:
> > >>>>>>>>>>> On 12/11/19 9:08 AM, Ondrej Mosnacek wrote:
> > >>>>>
> > >>>>> ...
> > >>>>>
> > >>>>>>>> selinux_state.initialized reflects whether a policy has
> > >>>>>>>> been loaded. With a few exceptions in certain hook functions, it is
> > >>>>>>>> only checked by the security server service functions
> > >>>>>>>> (security/selinux/ss/services.c) prior to accessing the policydb. So
> > >>>>>>>> there is a lot of SELinux processing that would still occur in that
> > >>>>>>>> situation unless we added if (!selinux_state.initialized) return 0;
> > >>>>>>>> checks to all the hook functions, which would create the same exposure
> > >>>>>>>> and would further break the SELinux-enabled case (we need to perform
> > >>>>>>>> some SELinux processing pre-policy-load to allocate blobs and track what
> > >>>>>>>> tasks and objects require delayed security initialization when policy
> > >>>>>>>> load finally occurs).
> > >>>>>>>
> > >>>>>>> I think what Casey was suggesting is to add another flag that would
> > >>>>>>> switch from "no policy loaded, but we expect it to be loaded
> > >>>>>>> eventually" to "no policy loaded and we don't expect/allow it to be
> > >>>>>>> loaded any more", which is essentially equivalent to checking
> > >>>>>>> selinux_enabled in each hook, which you had already brought up.
> > >>>>>>
> > >>>>>> Yep. if (!selinux_enabled) return 0; or if (selinux_state.disabled)
> > >>>>>> return 0; under #ifdef CONFIG_SECURITY_SELINUX_DISABLE in every hook
> > >>>>>> might be the best option until it can be removed altogether; avoids
> > >>>>>> impacting the LSM framework or any other security module, preserves the
> > >>>>>> existing functionality, fairly low overhead on the SELinux-disabled case.
> > >>>>>
> > >>>>> Just so I'm understanding this thread correctly, the above change
> > >>>>> (adding enabled checks to each SELinux hook implementation) is only
> > >>>>> until Fedora can figure out a way to deprecate and remove the runtime
> > >>>>> disable?
> > >>>>
> > >>>> That's my understanding. In the interim, Android kernels should already
> > >>>> be disabling CONFIG_SECURITY_SELINUX_DISABLE and other distros may
> > >>>> choose to disable it as long as they don't care about supporting SELinux
> > >>>> runtime disable.
> > >>>
> > >>> Okay, I just wanted to make sure I wasn't missing something.
> > >>> Honestly, I'd rather Fedora just go ahead and do whatever it is they
> > >>> need to do to turn off CONFIG_SECURITY_SELINUX_DISABLE (it sounds like
> > >>> they have a plan and are working on it), I'm not overly excited about
> > >>> temporarily cluttering up the code with additional "enabled" checks
> > >>> when the status quo works, even if it is less than ideal.
> > >>
> > >> The status quo is producing kernel crashes, courtesy of LSM stacking
> > >> changes...
> > >
> > > How prevalent are these crashes?
> > >
> > > This also only happens when disabling SELinux at runtime, yes?
> > > Something we've advised against for some time now and are working to
> > > eliminate? Let's just get rid of the runtime disable *soon*, and if
> > > we need a stop-gap fix let's just go with the hook reordering since
> > > that seems to minimize the impact, if not resolve it.
> >
> > Not optimistic given that the original bug was opened 2.5+ years ago,
> > commenters identified fairly significant challenges to removing the
> > support, and no visible progress was ever made. I suppose the hook
> > reordering will do but seems fragile and hacky. Whatever.
>
> Based on Ondrej's comments in this thread it sounded as if there was
> some renewed interest in actually eliminating it from Fedora this
> time. If that's not the case, perhaps it's time to start that effort
> back up, and if we can't ever get away from the runtime disable then
> we can take the step of admitting that everything is terrible and we
> can consider some of the options presented in this thread.
Yes, we are quite determined to do what it takes to remove it. It may
go more smoothly than expected, or it may get unexpectedly
complicated. It's hard to tell at this point.
--
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.
^ permalink raw reply
* Re: [PATCH] LSM: allow an LSM to disable all hooks at once
From: Ondrej Mosnacek @ 2019-12-13 13:54 UTC (permalink / raw)
To: Paul Moore
Cc: Stephen Smalley, Casey Schaufler, Kees Cook,
Linux Security Module list, James Morris, Serge E. Hallyn,
SElinux list, John Johansen, Micah Morton, Tetsuo Handa
In-Reply-To: <CAHC9VhQG9zZEL53XRdLHdmFJDpg8qAd9p61Xkm5AdSgM=-5eAg@mail.gmail.com>
On Thu, Dec 12, 2019 at 7:18 PM Paul Moore <paul@paul-moore.com> wrote:
> On Thu, Dec 12, 2019 at 1:10 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> > On 12/12/19 1:09 PM, Paul Moore wrote:
> > > On Thu, Dec 12, 2019 at 12:57 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> > >> On 12/12/19 12:54 PM, Paul Moore wrote:
> > >>> ...
> > >>> Just so I'm understanding this thread correctly, the above change
> > >>> (adding enabled checks to each SELinux hook implementation) is only
> > >>> until Fedora can figure out a way to deprecate and remove the runtime
> > >>> disable?
> > >>
> > >> That's my understanding. In the interim, Android kernels should already
> > >> be disabling CONFIG_SECURITY_SELINUX_DISABLE and other distros may
> > >> choose to disable it as long as they don't care about supporting SELinux
> > >> runtime disable.
> > >
> > > Okay, I just wanted to make sure I wasn't missing something.
> > > Honestly, I'd rather Fedora just go ahead and do whatever it is they
> > > need to do to turn off CONFIG_SECURITY_SELINUX_DISABLE (it sounds like
> > > they have a plan and are working on it), I'm not overly excited about
> > > temporarily cluttering up the code with additional "enabled" checks
> > > when the status quo works, even if it is less than ideal.
> >
> > The status quo is producing kernel crashes, courtesy of LSM stacking
> > changes...
>
> How prevalent are these crashes?
I don't think they are prevalent, we only received one report for RHEL
and it came in ~ 6 months after 8.0 was released, which was the first
release that had the stacking patch. I wasn't able to reproduce it
without adding delays between the hook removals. However, the report
may have some specific configuration where it happens more often due
to just the "right" timing of some events...
>
> This also only happens when disabling SELinux at runtime, yes?
> Something we've advised against for some time now and are working to
> eliminate? Let's just get rid of the runtime disable *soon*, and if
> we need a stop-gap fix let's just go with the hook reordering since
> that seems to minimize the impact, if not resolve it.
>
> I'm not going to comment on the stacking changes.
--
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.
^ permalink raw reply
* Re: [PATCH 2/2] efi: show error messages only when loading certificates is failed
From: Joey Lee @ 2019-12-13 10:35 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Chun-Yi Lee, Josh Boyer, Serge E . Hallyn, Nayna Jain, Mimi Zohar,
James Morris, David Howells, linux-efi, Linux Kernel Mailing List,
linux-security-module
In-Reply-To: <CAKv+Gu8-Ay2R9wU-wwz2w+Q9jZOduXYigmFJL8Rmppnm1CSpHg@mail.gmail.com>
Hi Ard,
On Fri, Dec 13, 2019 at 10:04:14AM +0000, Ard Biesheuvel wrote:
> On Fri, 13 Dec 2019 at 10:21, Joey Lee <JLee@suse.com> wrote:
> >
> > Hi Ard,
> >
> > On Fri, Dec 13, 2019 at 09:10:12AM +0000, Ard Biesheuvel wrote:
> > > On Fri, 13 Dec 2019 at 10:07, Lee, Chun-Yi <joeyli.kernel@gmail.com> wrote:
> > > >
> > > > When loading certificates list from EFI variables, the error
> > > > message and efi status code always be emitted to dmesg. It looks
> > > > ugly:
> > > >
> > > > [ 2.335031] Couldn't get size: 0x800000000000000e
> > > > [ 2.335032] Couldn't get UEFI MokListRT
> > > > [ 2.339985] Couldn't get size: 0x800000000000000e
> > > > [ 2.339987] Couldn't get UEFI dbx list
> > > >
> > > > This cosmetic patch moved the messages to the error handling code
> > > > path. And, it also shows the corresponding status string of status
> > > > code.
> > > >
> > >
> > > So what output do we get after applying this patch when those
> > > variables don't exist?
> > >
> >
> > A "UEFI:xxxx list was not found" message will be exposed in dmesg
> > when kernel loglevel be set to debug. Otherwise there have no messages.
> >
>
> OK, that works for me.
>
> I take it this will go via the linux-security tree along with 1/2?
>
Yes, this patch must go with 1/2 patch.
> Acked-by: Ard Biesheuvel <ardb@kernel.org>
>
Thanks for your review!
Joey Lee
>
>
> > > > Signed-off-by: "Lee, Chun-Yi" <jlee@suse.com>
> > > > ---
> > > > security/integrity/platform_certs/load_uefi.c | 40 ++++++++++++++-------------
> > > > 1 file changed, 21 insertions(+), 19 deletions(-)
> > > >
> > > > diff --git a/security/integrity/platform_certs/load_uefi.c b/security/integrity/platform_certs/load_uefi.c
> > > > index 81b19c52832b..b6c60fb3fb6c 100644
> > > > --- a/security/integrity/platform_certs/load_uefi.c
> > > > +++ b/security/integrity/platform_certs/load_uefi.c
> > > > @@ -1,4 +1,5 @@
> > > > // SPDX-License-Identifier: GPL-2.0
> > > > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > > >
> > > > #include <linux/kernel.h>
> > > > #include <linux/sched.h>
> > > > @@ -39,7 +40,7 @@ static __init bool uefi_check_ignore_db(void)
> > > > * Get a certificate list blob from the named EFI variable.
> > > > */
> > > > static __init void *get_cert_list(efi_char16_t *name, efi_guid_t *guid,
> > > > - unsigned long *size)
> > > > + unsigned long *size, const char *source)
> > > > {
> > > > efi_status_t status;
> > > > unsigned long lsize = 4;
> > > > @@ -48,23 +49,30 @@ static __init void *get_cert_list(efi_char16_t *name, efi_guid_t *guid,
> > > >
> > > > status = efi.get_variable(name, guid, NULL, &lsize, &tmpdb);
> > > > if (status != EFI_BUFFER_TOO_SMALL) {
> > > > - pr_err("Couldn't get size: 0x%lx\n", status);
> > > > - return NULL;
> > > > + if (status == EFI_NOT_FOUND) {
> > > > + pr_debug("%s list was not found\n", source);
> > > > + return NULL;
> > > > + }
> > > > + goto err;
> > > > }
> > > >
> > > > db = kmalloc(lsize, GFP_KERNEL);
> > > > - if (!db)
> > > > - return NULL;
> > > > + if (!db) {
> > > > + status = EFI_OUT_OF_RESOURCES;
> > > > + goto err;
> > > > + }
> > > >
> > > > status = efi.get_variable(name, guid, NULL, &lsize, db);
> > > > if (status != EFI_SUCCESS) {
> > > > kfree(db);
> > > > - pr_err("Error reading db var: 0x%lx\n", status);
> > > > - return NULL;
> > > > + goto err;
> > > > }
> > > >
> > > > *size = lsize;
> > > > return db;
> > > > +err:
> > > > + pr_err("Couldn't get %s list: %s\n", source, efi_status_to_str(status));
> > > > + return NULL;
> > > > }
> > > >
> > > > /*
> > > > @@ -153,10 +161,8 @@ static int __init load_uefi_certs(void)
> > > > * an error if we can't get them.
> > > > */
> > > > if (!uefi_check_ignore_db()) {
> > > > - db = get_cert_list(L"db", &secure_var, &dbsize);
> > > > - if (!db) {
> > > > - pr_err("MODSIGN: Couldn't get UEFI db list\n");
> > > > - } else {
> > > > + db = get_cert_list(L"db", &secure_var, &dbsize, "UEFI:db");
> > > > + if (db) {
> > > > rc = parse_efi_signature_list("UEFI:db",
> > > > db, dbsize, get_handler_for_db);
> > > > if (rc)
> > > > @@ -166,10 +172,8 @@ static int __init load_uefi_certs(void)
> > > > }
> > > > }
> > > >
> > > > - mok = get_cert_list(L"MokListRT", &mok_var, &moksize);
> > > > - if (!mok) {
> > > > - pr_info("Couldn't get UEFI MokListRT\n");
> > > > - } else {
> > > > + mok = get_cert_list(L"MokListRT", &mok_var, &moksize, "UEFI:MokListRT");
> > > > + if (mok) {
> > > > rc = parse_efi_signature_list("UEFI:MokListRT",
> > > > mok, moksize, get_handler_for_db);
> > > > if (rc)
> > > > @@ -177,10 +181,8 @@ static int __init load_uefi_certs(void)
> > > > kfree(mok);
> > > > }
> > > >
> > > > - dbx = get_cert_list(L"dbx", &secure_var, &dbxsize);
> > > > - if (!dbx) {
> > > > - pr_info("Couldn't get UEFI dbx list\n");
> > > > - } else {
> > > > + dbx = get_cert_list(L"dbx", &secure_var, &dbxsize, "UEFI:dbx");
> > > > + if (dbx) {
> > > > rc = parse_efi_signature_list("UEFI:dbx",
> > > > dbx, dbxsize,
> > > > get_handler_for_dbx);
> > > > --
> > > > 2.16.4
> > > >
^ permalink raw reply
* Re: [PATCH 1/2 v2] efi: add a function to convert the status code to a string
From: Joey Lee @ 2019-12-13 10:33 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Lee, Chun-Yi, James Morris, Serge E . Hallyn, David Howells,
Josh Boyer, Nayna Jain, Mimi Zohar, linux-efi,
linux-security-module, Linux Kernel Mailing List
In-Reply-To: <CAKv+Gu8sGku8e1q9ku_wXfcXTGQ5W8Lt_q5KEK9WycgHw15TgA@mail.gmail.com>
On Fri, Dec 13, 2019 at 10:03:27AM +0000, Ard Biesheuvel wrote:
> On Fri, 13 Dec 2019 at 10:07, Lee, Chun-Yi <joeyli.kernel@gmail.com> wrote:
> >
> > This function can be used to convert EFI status code to a string
> > to improve the readability of log.
> >
> > v2:
> > Moved the convert function to efi.c
> >
>
> Please put the patch series revision log below the ---
OK! I will move to below the --- next time.
>
> > Signed-off-by: "Lee, Chun-Yi" <jlee@suse.com>
>
> Acked-by: Ard Biesheuvel <ardb@kernel.org>
>
Thanks for your review!
> > ---
v2:
Moved the convert function to efi.c
> > drivers/firmware/efi/efi.c | 32 ++++++++++++++++++++++++++++++++
> > include/linux/efi.h | 1 +
> > 2 files changed, 33 insertions(+)
Do you mean move the revision log to here like the above?
Thanks a lot!
Joey Lee
> >
> > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> > index e98bbf8e56d9..8bdc1c17eb5d 100644
> > --- a/drivers/firmware/efi/efi.c
> > +++ b/drivers/firmware/efi/efi.c
> > @@ -954,6 +954,38 @@ int efi_status_to_err(efi_status_t status)
> > return err;
> > }
> >
> > +#define EFI_STATUS_STR(_status) \
> > + EFI_##_status : return "EFI_" __stringify(_status)
> > +
> > +const char *efi_status_to_str(efi_status_t status)
> > +{
> > + switch (status) {
> > + case EFI_STATUS_STR(SUCCESS);
> > + case EFI_STATUS_STR(LOAD_ERROR);
> > + case EFI_STATUS_STR(INVALID_PARAMETER);
> > + case EFI_STATUS_STR(UNSUPPORTED);
> > + case EFI_STATUS_STR(BAD_BUFFER_SIZE);
> > + case EFI_STATUS_STR(BUFFER_TOO_SMALL);
> > + case EFI_STATUS_STR(NOT_READY);
> > + case EFI_STATUS_STR(DEVICE_ERROR);
> > + case EFI_STATUS_STR(WRITE_PROTECTED);
> > + case EFI_STATUS_STR(OUT_OF_RESOURCES);
> > + case EFI_STATUS_STR(NOT_FOUND);
> > + case EFI_STATUS_STR(ABORTED);
> > + case EFI_STATUS_STR(SECURITY_VIOLATION);
> > + }
> > + /*
> > + * There are two possibilities for this message to be exposed:
> > + * - Caller feeds a unknown status code from firmware.
> > + * - A new status code be defined in efi.h but we forgot to update
> > + * this function.
> > + */
> > + pr_warn("Unknown efi status: 0x%lx\n", status);
> > +
> > + return "Unknown efi status";
> > +}
> > +EXPORT_SYMBOL(efi_status_to_str);
> > +
> > static DEFINE_SPINLOCK(efi_mem_reserve_persistent_lock);
> > static struct linux_efi_memreserve *efi_memreserve_root __ro_after_init;
> >
> > diff --git a/include/linux/efi.h b/include/linux/efi.h
> > index d87acf62958e..2c6848d2b112 100644
> > --- a/include/linux/efi.h
> > +++ b/include/linux/efi.h
> > @@ -1228,6 +1228,7 @@ efi_capsule_pending(int *reset_type)
> > #endif
> >
> > extern int efi_status_to_err(efi_status_t status);
> > +extern const char *efi_status_to_str(efi_status_t status);
> >
> > /*
> > * Variable Attributes
> > --
> > 2.16.4
> >
^ permalink raw reply
* Re: [PATCH 2/2] efi: show error messages only when loading certificates is failed
From: Ard Biesheuvel @ 2019-12-13 10:04 UTC (permalink / raw)
To: Joey Lee
Cc: Chun-Yi Lee, Josh Boyer, Serge E . Hallyn, Nayna Jain, Mimi Zohar,
James Morris, David Howells, linux-efi, Linux Kernel Mailing List,
linux-security-module
In-Reply-To: <20191213092049.GW22409@linux-l9pv.suse>
On Fri, 13 Dec 2019 at 10:21, Joey Lee <JLee@suse.com> wrote:
>
> Hi Ard,
>
> On Fri, Dec 13, 2019 at 09:10:12AM +0000, Ard Biesheuvel wrote:
> > On Fri, 13 Dec 2019 at 10:07, Lee, Chun-Yi <joeyli.kernel@gmail.com> wrote:
> > >
> > > When loading certificates list from EFI variables, the error
> > > message and efi status code always be emitted to dmesg. It looks
> > > ugly:
> > >
> > > [ 2.335031] Couldn't get size: 0x800000000000000e
> > > [ 2.335032] Couldn't get UEFI MokListRT
> > > [ 2.339985] Couldn't get size: 0x800000000000000e
> > > [ 2.339987] Couldn't get UEFI dbx list
> > >
> > > This cosmetic patch moved the messages to the error handling code
> > > path. And, it also shows the corresponding status string of status
> > > code.
> > >
> >
> > So what output do we get after applying this patch when those
> > variables don't exist?
> >
>
> A "UEFI:xxxx list was not found" message will be exposed in dmesg
> when kernel loglevel be set to debug. Otherwise there have no messages.
>
OK, that works for me.
I take it this will go via the linux-security tree along with 1/2?
Acked-by: Ard Biesheuvel <ardb@kernel.org>
> > > Signed-off-by: "Lee, Chun-Yi" <jlee@suse.com>
> > > ---
> > > security/integrity/platform_certs/load_uefi.c | 40 ++++++++++++++-------------
> > > 1 file changed, 21 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/security/integrity/platform_certs/load_uefi.c b/security/integrity/platform_certs/load_uefi.c
> > > index 81b19c52832b..b6c60fb3fb6c 100644
> > > --- a/security/integrity/platform_certs/load_uefi.c
> > > +++ b/security/integrity/platform_certs/load_uefi.c
> > > @@ -1,4 +1,5 @@
> > > // SPDX-License-Identifier: GPL-2.0
> > > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > >
> > > #include <linux/kernel.h>
> > > #include <linux/sched.h>
> > > @@ -39,7 +40,7 @@ static __init bool uefi_check_ignore_db(void)
> > > * Get a certificate list blob from the named EFI variable.
> > > */
> > > static __init void *get_cert_list(efi_char16_t *name, efi_guid_t *guid,
> > > - unsigned long *size)
> > > + unsigned long *size, const char *source)
> > > {
> > > efi_status_t status;
> > > unsigned long lsize = 4;
> > > @@ -48,23 +49,30 @@ static __init void *get_cert_list(efi_char16_t *name, efi_guid_t *guid,
> > >
> > > status = efi.get_variable(name, guid, NULL, &lsize, &tmpdb);
> > > if (status != EFI_BUFFER_TOO_SMALL) {
> > > - pr_err("Couldn't get size: 0x%lx\n", status);
> > > - return NULL;
> > > + if (status == EFI_NOT_FOUND) {
> > > + pr_debug("%s list was not found\n", source);
> > > + return NULL;
> > > + }
> > > + goto err;
> > > }
> > >
> > > db = kmalloc(lsize, GFP_KERNEL);
> > > - if (!db)
> > > - return NULL;
> > > + if (!db) {
> > > + status = EFI_OUT_OF_RESOURCES;
> > > + goto err;
> > > + }
> > >
> > > status = efi.get_variable(name, guid, NULL, &lsize, db);
> > > if (status != EFI_SUCCESS) {
> > > kfree(db);
> > > - pr_err("Error reading db var: 0x%lx\n", status);
> > > - return NULL;
> > > + goto err;
> > > }
> > >
> > > *size = lsize;
> > > return db;
> > > +err:
> > > + pr_err("Couldn't get %s list: %s\n", source, efi_status_to_str(status));
> > > + return NULL;
> > > }
> > >
> > > /*
> > > @@ -153,10 +161,8 @@ static int __init load_uefi_certs(void)
> > > * an error if we can't get them.
> > > */
> > > if (!uefi_check_ignore_db()) {
> > > - db = get_cert_list(L"db", &secure_var, &dbsize);
> > > - if (!db) {
> > > - pr_err("MODSIGN: Couldn't get UEFI db list\n");
> > > - } else {
> > > + db = get_cert_list(L"db", &secure_var, &dbsize, "UEFI:db");
> > > + if (db) {
> > > rc = parse_efi_signature_list("UEFI:db",
> > > db, dbsize, get_handler_for_db);
> > > if (rc)
> > > @@ -166,10 +172,8 @@ static int __init load_uefi_certs(void)
> > > }
> > > }
> > >
> > > - mok = get_cert_list(L"MokListRT", &mok_var, &moksize);
> > > - if (!mok) {
> > > - pr_info("Couldn't get UEFI MokListRT\n");
> > > - } else {
> > > + mok = get_cert_list(L"MokListRT", &mok_var, &moksize, "UEFI:MokListRT");
> > > + if (mok) {
> > > rc = parse_efi_signature_list("UEFI:MokListRT",
> > > mok, moksize, get_handler_for_db);
> > > if (rc)
> > > @@ -177,10 +181,8 @@ static int __init load_uefi_certs(void)
> > > kfree(mok);
> > > }
> > >
> > > - dbx = get_cert_list(L"dbx", &secure_var, &dbxsize);
> > > - if (!dbx) {
> > > - pr_info("Couldn't get UEFI dbx list\n");
> > > - } else {
> > > + dbx = get_cert_list(L"dbx", &secure_var, &dbxsize, "UEFI:dbx");
> > > + if (dbx) {
> > > rc = parse_efi_signature_list("UEFI:dbx",
> > > dbx, dbxsize,
> > > get_handler_for_dbx);
> > > --
> > > 2.16.4
> > >
^ permalink raw reply
* Re: [PATCH 1/2 v2] efi: add a function to convert the status code to a string
From: Ard Biesheuvel @ 2019-12-13 10:03 UTC (permalink / raw)
To: Lee, Chun-Yi
Cc: James Morris, Serge E . Hallyn, David Howells, Josh Boyer,
Nayna Jain, Mimi Zohar, linux-efi, linux-security-module,
Linux Kernel Mailing List, Lee, Chun-Yi
In-Reply-To: <20191213090646.12329-2-jlee@suse.com>
On Fri, 13 Dec 2019 at 10:07, Lee, Chun-Yi <joeyli.kernel@gmail.com> wrote:
>
> This function can be used to convert EFI status code to a string
> to improve the readability of log.
>
> v2:
> Moved the convert function to efi.c
>
Please put the patch series revision log below the ---
> Signed-off-by: "Lee, Chun-Yi" <jlee@suse.com>
Acked-by: Ard Biesheuvel <ardb@kernel.org>
> ---
> drivers/firmware/efi/efi.c | 32 ++++++++++++++++++++++++++++++++
> include/linux/efi.h | 1 +
> 2 files changed, 33 insertions(+)
>
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index e98bbf8e56d9..8bdc1c17eb5d 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -954,6 +954,38 @@ int efi_status_to_err(efi_status_t status)
> return err;
> }
>
> +#define EFI_STATUS_STR(_status) \
> + EFI_##_status : return "EFI_" __stringify(_status)
> +
> +const char *efi_status_to_str(efi_status_t status)
> +{
> + switch (status) {
> + case EFI_STATUS_STR(SUCCESS);
> + case EFI_STATUS_STR(LOAD_ERROR);
> + case EFI_STATUS_STR(INVALID_PARAMETER);
> + case EFI_STATUS_STR(UNSUPPORTED);
> + case EFI_STATUS_STR(BAD_BUFFER_SIZE);
> + case EFI_STATUS_STR(BUFFER_TOO_SMALL);
> + case EFI_STATUS_STR(NOT_READY);
> + case EFI_STATUS_STR(DEVICE_ERROR);
> + case EFI_STATUS_STR(WRITE_PROTECTED);
> + case EFI_STATUS_STR(OUT_OF_RESOURCES);
> + case EFI_STATUS_STR(NOT_FOUND);
> + case EFI_STATUS_STR(ABORTED);
> + case EFI_STATUS_STR(SECURITY_VIOLATION);
> + }
> + /*
> + * There are two possibilities for this message to be exposed:
> + * - Caller feeds a unknown status code from firmware.
> + * - A new status code be defined in efi.h but we forgot to update
> + * this function.
> + */
> + pr_warn("Unknown efi status: 0x%lx\n", status);
> +
> + return "Unknown efi status";
> +}
> +EXPORT_SYMBOL(efi_status_to_str);
> +
> static DEFINE_SPINLOCK(efi_mem_reserve_persistent_lock);
> static struct linux_efi_memreserve *efi_memreserve_root __ro_after_init;
>
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index d87acf62958e..2c6848d2b112 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -1228,6 +1228,7 @@ efi_capsule_pending(int *reset_type)
> #endif
>
> extern int efi_status_to_err(efi_status_t status);
> +extern const char *efi_status_to_str(efi_status_t status);
>
> /*
> * Variable Attributes
> --
> 2.16.4
>
^ permalink raw reply
* Re: [PATCH 2/2] efi: show error messages only when loading certificates is failed
From: Joey Lee @ 2019-12-13 9:20 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Chun-Yi Lee, Josh Boyer, Serge E . Hallyn, Nayna Jain, Mimi Zohar,
James Morris, David Howells, linux-efi, Linux Kernel Mailing List,
linux-security-module
In-Reply-To: <CAKv+Gu_2GTqKJNVpMEg4ic_3ACb5GJKAkgfFWoEdWqMN7pmwiA@mail.gmail.com>
Hi Ard,
On Fri, Dec 13, 2019 at 09:10:12AM +0000, Ard Biesheuvel wrote:
> On Fri, 13 Dec 2019 at 10:07, Lee, Chun-Yi <joeyli.kernel@gmail.com> wrote:
> >
> > When loading certificates list from EFI variables, the error
> > message and efi status code always be emitted to dmesg. It looks
> > ugly:
> >
> > [ 2.335031] Couldn't get size: 0x800000000000000e
> > [ 2.335032] Couldn't get UEFI MokListRT
> > [ 2.339985] Couldn't get size: 0x800000000000000e
> > [ 2.339987] Couldn't get UEFI dbx list
> >
> > This cosmetic patch moved the messages to the error handling code
> > path. And, it also shows the corresponding status string of status
> > code.
> >
>
> So what output do we get after applying this patch when those
> variables don't exist?
>
A "UEFI:xxxx list was not found" message will be exposed in dmesg
when kernel loglevel be set to debug. Otherwise there have no messages.
Thanks
Joey Lee
> > Signed-off-by: "Lee, Chun-Yi" <jlee@suse.com>
> > ---
> > security/integrity/platform_certs/load_uefi.c | 40 ++++++++++++++-------------
> > 1 file changed, 21 insertions(+), 19 deletions(-)
> >
> > diff --git a/security/integrity/platform_certs/load_uefi.c b/security/integrity/platform_certs/load_uefi.c
> > index 81b19c52832b..b6c60fb3fb6c 100644
> > --- a/security/integrity/platform_certs/load_uefi.c
> > +++ b/security/integrity/platform_certs/load_uefi.c
> > @@ -1,4 +1,5 @@
> > // SPDX-License-Identifier: GPL-2.0
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> >
> > #include <linux/kernel.h>
> > #include <linux/sched.h>
> > @@ -39,7 +40,7 @@ static __init bool uefi_check_ignore_db(void)
> > * Get a certificate list blob from the named EFI variable.
> > */
> > static __init void *get_cert_list(efi_char16_t *name, efi_guid_t *guid,
> > - unsigned long *size)
> > + unsigned long *size, const char *source)
> > {
> > efi_status_t status;
> > unsigned long lsize = 4;
> > @@ -48,23 +49,30 @@ static __init void *get_cert_list(efi_char16_t *name, efi_guid_t *guid,
> >
> > status = efi.get_variable(name, guid, NULL, &lsize, &tmpdb);
> > if (status != EFI_BUFFER_TOO_SMALL) {
> > - pr_err("Couldn't get size: 0x%lx\n", status);
> > - return NULL;
> > + if (status == EFI_NOT_FOUND) {
> > + pr_debug("%s list was not found\n", source);
> > + return NULL;
> > + }
> > + goto err;
> > }
> >
> > db = kmalloc(lsize, GFP_KERNEL);
> > - if (!db)
> > - return NULL;
> > + if (!db) {
> > + status = EFI_OUT_OF_RESOURCES;
> > + goto err;
> > + }
> >
> > status = efi.get_variable(name, guid, NULL, &lsize, db);
> > if (status != EFI_SUCCESS) {
> > kfree(db);
> > - pr_err("Error reading db var: 0x%lx\n", status);
> > - return NULL;
> > + goto err;
> > }
> >
> > *size = lsize;
> > return db;
> > +err:
> > + pr_err("Couldn't get %s list: %s\n", source, efi_status_to_str(status));
> > + return NULL;
> > }
> >
> > /*
> > @@ -153,10 +161,8 @@ static int __init load_uefi_certs(void)
> > * an error if we can't get them.
> > */
> > if (!uefi_check_ignore_db()) {
> > - db = get_cert_list(L"db", &secure_var, &dbsize);
> > - if (!db) {
> > - pr_err("MODSIGN: Couldn't get UEFI db list\n");
> > - } else {
> > + db = get_cert_list(L"db", &secure_var, &dbsize, "UEFI:db");
> > + if (db) {
> > rc = parse_efi_signature_list("UEFI:db",
> > db, dbsize, get_handler_for_db);
> > if (rc)
> > @@ -166,10 +172,8 @@ static int __init load_uefi_certs(void)
> > }
> > }
> >
> > - mok = get_cert_list(L"MokListRT", &mok_var, &moksize);
> > - if (!mok) {
> > - pr_info("Couldn't get UEFI MokListRT\n");
> > - } else {
> > + mok = get_cert_list(L"MokListRT", &mok_var, &moksize, "UEFI:MokListRT");
> > + if (mok) {
> > rc = parse_efi_signature_list("UEFI:MokListRT",
> > mok, moksize, get_handler_for_db);
> > if (rc)
> > @@ -177,10 +181,8 @@ static int __init load_uefi_certs(void)
> > kfree(mok);
> > }
> >
> > - dbx = get_cert_list(L"dbx", &secure_var, &dbxsize);
> > - if (!dbx) {
> > - pr_info("Couldn't get UEFI dbx list\n");
> > - } else {
> > + dbx = get_cert_list(L"dbx", &secure_var, &dbxsize, "UEFI:dbx");
> > + if (dbx) {
> > rc = parse_efi_signature_list("UEFI:dbx",
> > dbx, dbxsize,
> > get_handler_for_dbx);
> > --
> > 2.16.4
> >
^ permalink raw reply
* Re: [PATCH 2/2] efi: show error messages only when loading certificates is failed
From: Ard Biesheuvel @ 2019-12-13 9:10 UTC (permalink / raw)
To: Lee, Chun-Yi
Cc: James Morris, Serge E . Hallyn, David Howells, Josh Boyer,
Nayna Jain, Mimi Zohar, linux-efi, linux-security-module,
Linux Kernel Mailing List, Lee, Chun-Yi
In-Reply-To: <20191213090646.12329-3-jlee@suse.com>
On Fri, 13 Dec 2019 at 10:07, Lee, Chun-Yi <joeyli.kernel@gmail.com> wrote:
>
> When loading certificates list from EFI variables, the error
> message and efi status code always be emitted to dmesg. It looks
> ugly:
>
> [ 2.335031] Couldn't get size: 0x800000000000000e
> [ 2.335032] Couldn't get UEFI MokListRT
> [ 2.339985] Couldn't get size: 0x800000000000000e
> [ 2.339987] Couldn't get UEFI dbx list
>
> This cosmetic patch moved the messages to the error handling code
> path. And, it also shows the corresponding status string of status
> code.
>
So what output do we get after applying this patch when those
variables don't exist?
> Signed-off-by: "Lee, Chun-Yi" <jlee@suse.com>
> ---
> security/integrity/platform_certs/load_uefi.c | 40 ++++++++++++++-------------
> 1 file changed, 21 insertions(+), 19 deletions(-)
>
> diff --git a/security/integrity/platform_certs/load_uefi.c b/security/integrity/platform_certs/load_uefi.c
> index 81b19c52832b..b6c60fb3fb6c 100644
> --- a/security/integrity/platform_certs/load_uefi.c
> +++ b/security/integrity/platform_certs/load_uefi.c
> @@ -1,4 +1,5 @@
> // SPDX-License-Identifier: GPL-2.0
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>
> #include <linux/kernel.h>
> #include <linux/sched.h>
> @@ -39,7 +40,7 @@ static __init bool uefi_check_ignore_db(void)
> * Get a certificate list blob from the named EFI variable.
> */
> static __init void *get_cert_list(efi_char16_t *name, efi_guid_t *guid,
> - unsigned long *size)
> + unsigned long *size, const char *source)
> {
> efi_status_t status;
> unsigned long lsize = 4;
> @@ -48,23 +49,30 @@ static __init void *get_cert_list(efi_char16_t *name, efi_guid_t *guid,
>
> status = efi.get_variable(name, guid, NULL, &lsize, &tmpdb);
> if (status != EFI_BUFFER_TOO_SMALL) {
> - pr_err("Couldn't get size: 0x%lx\n", status);
> - return NULL;
> + if (status == EFI_NOT_FOUND) {
> + pr_debug("%s list was not found\n", source);
> + return NULL;
> + }
> + goto err;
> }
>
> db = kmalloc(lsize, GFP_KERNEL);
> - if (!db)
> - return NULL;
> + if (!db) {
> + status = EFI_OUT_OF_RESOURCES;
> + goto err;
> + }
>
> status = efi.get_variable(name, guid, NULL, &lsize, db);
> if (status != EFI_SUCCESS) {
> kfree(db);
> - pr_err("Error reading db var: 0x%lx\n", status);
> - return NULL;
> + goto err;
> }
>
> *size = lsize;
> return db;
> +err:
> + pr_err("Couldn't get %s list: %s\n", source, efi_status_to_str(status));
> + return NULL;
> }
>
> /*
> @@ -153,10 +161,8 @@ static int __init load_uefi_certs(void)
> * an error if we can't get them.
> */
> if (!uefi_check_ignore_db()) {
> - db = get_cert_list(L"db", &secure_var, &dbsize);
> - if (!db) {
> - pr_err("MODSIGN: Couldn't get UEFI db list\n");
> - } else {
> + db = get_cert_list(L"db", &secure_var, &dbsize, "UEFI:db");
> + if (db) {
> rc = parse_efi_signature_list("UEFI:db",
> db, dbsize, get_handler_for_db);
> if (rc)
> @@ -166,10 +172,8 @@ static int __init load_uefi_certs(void)
> }
> }
>
> - mok = get_cert_list(L"MokListRT", &mok_var, &moksize);
> - if (!mok) {
> - pr_info("Couldn't get UEFI MokListRT\n");
> - } else {
> + mok = get_cert_list(L"MokListRT", &mok_var, &moksize, "UEFI:MokListRT");
> + if (mok) {
> rc = parse_efi_signature_list("UEFI:MokListRT",
> mok, moksize, get_handler_for_db);
> if (rc)
> @@ -177,10 +181,8 @@ static int __init load_uefi_certs(void)
> kfree(mok);
> }
>
> - dbx = get_cert_list(L"dbx", &secure_var, &dbxsize);
> - if (!dbx) {
> - pr_info("Couldn't get UEFI dbx list\n");
> - } else {
> + dbx = get_cert_list(L"dbx", &secure_var, &dbxsize, "UEFI:dbx");
> + if (dbx) {
> rc = parse_efi_signature_list("UEFI:dbx",
> dbx, dbxsize,
> get_handler_for_dbx);
> --
> 2.16.4
>
^ permalink raw reply
* [PATCH 2/2] efi: show error messages only when loading certificates is failed
From: Lee, Chun-Yi @ 2019-12-13 9:06 UTC (permalink / raw)
To: Ard Biesheuvel, James Morris, Serge E . Hallyn, David Howells,
Josh Boyer, Nayna Jain, Mimi Zohar
Cc: linux-efi, linux-security-module, linux-kernel, Lee, Chun-Yi
In-Reply-To: <20191213090646.12329-1-jlee@suse.com>
When loading certificates list from EFI variables, the error
message and efi status code always be emitted to dmesg. It looks
ugly:
[ 2.335031] Couldn't get size: 0x800000000000000e
[ 2.335032] Couldn't get UEFI MokListRT
[ 2.339985] Couldn't get size: 0x800000000000000e
[ 2.339987] Couldn't get UEFI dbx list
This cosmetic patch moved the messages to the error handling code
path. And, it also shows the corresponding status string of status
code.
Signed-off-by: "Lee, Chun-Yi" <jlee@suse.com>
---
security/integrity/platform_certs/load_uefi.c | 40 ++++++++++++++-------------
1 file changed, 21 insertions(+), 19 deletions(-)
diff --git a/security/integrity/platform_certs/load_uefi.c b/security/integrity/platform_certs/load_uefi.c
index 81b19c52832b..b6c60fb3fb6c 100644
--- a/security/integrity/platform_certs/load_uefi.c
+++ b/security/integrity/platform_certs/load_uefi.c
@@ -1,4 +1,5 @@
// SPDX-License-Identifier: GPL-2.0
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
#include <linux/kernel.h>
#include <linux/sched.h>
@@ -39,7 +40,7 @@ static __init bool uefi_check_ignore_db(void)
* Get a certificate list blob from the named EFI variable.
*/
static __init void *get_cert_list(efi_char16_t *name, efi_guid_t *guid,
- unsigned long *size)
+ unsigned long *size, const char *source)
{
efi_status_t status;
unsigned long lsize = 4;
@@ -48,23 +49,30 @@ static __init void *get_cert_list(efi_char16_t *name, efi_guid_t *guid,
status = efi.get_variable(name, guid, NULL, &lsize, &tmpdb);
if (status != EFI_BUFFER_TOO_SMALL) {
- pr_err("Couldn't get size: 0x%lx\n", status);
- return NULL;
+ if (status == EFI_NOT_FOUND) {
+ pr_debug("%s list was not found\n", source);
+ return NULL;
+ }
+ goto err;
}
db = kmalloc(lsize, GFP_KERNEL);
- if (!db)
- return NULL;
+ if (!db) {
+ status = EFI_OUT_OF_RESOURCES;
+ goto err;
+ }
status = efi.get_variable(name, guid, NULL, &lsize, db);
if (status != EFI_SUCCESS) {
kfree(db);
- pr_err("Error reading db var: 0x%lx\n", status);
- return NULL;
+ goto err;
}
*size = lsize;
return db;
+err:
+ pr_err("Couldn't get %s list: %s\n", source, efi_status_to_str(status));
+ return NULL;
}
/*
@@ -153,10 +161,8 @@ static int __init load_uefi_certs(void)
* an error if we can't get them.
*/
if (!uefi_check_ignore_db()) {
- db = get_cert_list(L"db", &secure_var, &dbsize);
- if (!db) {
- pr_err("MODSIGN: Couldn't get UEFI db list\n");
- } else {
+ db = get_cert_list(L"db", &secure_var, &dbsize, "UEFI:db");
+ if (db) {
rc = parse_efi_signature_list("UEFI:db",
db, dbsize, get_handler_for_db);
if (rc)
@@ -166,10 +172,8 @@ static int __init load_uefi_certs(void)
}
}
- mok = get_cert_list(L"MokListRT", &mok_var, &moksize);
- if (!mok) {
- pr_info("Couldn't get UEFI MokListRT\n");
- } else {
+ mok = get_cert_list(L"MokListRT", &mok_var, &moksize, "UEFI:MokListRT");
+ if (mok) {
rc = parse_efi_signature_list("UEFI:MokListRT",
mok, moksize, get_handler_for_db);
if (rc)
@@ -177,10 +181,8 @@ static int __init load_uefi_certs(void)
kfree(mok);
}
- dbx = get_cert_list(L"dbx", &secure_var, &dbxsize);
- if (!dbx) {
- pr_info("Couldn't get UEFI dbx list\n");
- } else {
+ dbx = get_cert_list(L"dbx", &secure_var, &dbxsize, "UEFI:dbx");
+ if (dbx) {
rc = parse_efi_signature_list("UEFI:dbx",
dbx, dbxsize,
get_handler_for_dbx);
--
2.16.4
^ permalink raw reply related
* [PATCH 1/2 v2] efi: add a function to convert the status code to a string
From: Lee, Chun-Yi @ 2019-12-13 9:06 UTC (permalink / raw)
To: Ard Biesheuvel, James Morris, Serge E . Hallyn, David Howells,
Josh Boyer, Nayna Jain, Mimi Zohar
Cc: linux-efi, linux-security-module, linux-kernel, Lee, Chun-Yi
In-Reply-To: <20191213090646.12329-1-jlee@suse.com>
This function can be used to convert EFI status code to a string
to improve the readability of log.
v2:
Moved the convert function to efi.c
Signed-off-by: "Lee, Chun-Yi" <jlee@suse.com>
---
drivers/firmware/efi/efi.c | 32 ++++++++++++++++++++++++++++++++
include/linux/efi.h | 1 +
2 files changed, 33 insertions(+)
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index e98bbf8e56d9..8bdc1c17eb5d 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -954,6 +954,38 @@ int efi_status_to_err(efi_status_t status)
return err;
}
+#define EFI_STATUS_STR(_status) \
+ EFI_##_status : return "EFI_" __stringify(_status)
+
+const char *efi_status_to_str(efi_status_t status)
+{
+ switch (status) {
+ case EFI_STATUS_STR(SUCCESS);
+ case EFI_STATUS_STR(LOAD_ERROR);
+ case EFI_STATUS_STR(INVALID_PARAMETER);
+ case EFI_STATUS_STR(UNSUPPORTED);
+ case EFI_STATUS_STR(BAD_BUFFER_SIZE);
+ case EFI_STATUS_STR(BUFFER_TOO_SMALL);
+ case EFI_STATUS_STR(NOT_READY);
+ case EFI_STATUS_STR(DEVICE_ERROR);
+ case EFI_STATUS_STR(WRITE_PROTECTED);
+ case EFI_STATUS_STR(OUT_OF_RESOURCES);
+ case EFI_STATUS_STR(NOT_FOUND);
+ case EFI_STATUS_STR(ABORTED);
+ case EFI_STATUS_STR(SECURITY_VIOLATION);
+ }
+ /*
+ * There are two possibilities for this message to be exposed:
+ * - Caller feeds a unknown status code from firmware.
+ * - A new status code be defined in efi.h but we forgot to update
+ * this function.
+ */
+ pr_warn("Unknown efi status: 0x%lx\n", status);
+
+ return "Unknown efi status";
+}
+EXPORT_SYMBOL(efi_status_to_str);
+
static DEFINE_SPINLOCK(efi_mem_reserve_persistent_lock);
static struct linux_efi_memreserve *efi_memreserve_root __ro_after_init;
diff --git a/include/linux/efi.h b/include/linux/efi.h
index d87acf62958e..2c6848d2b112 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1228,6 +1228,7 @@ efi_capsule_pending(int *reset_type)
#endif
extern int efi_status_to_err(efi_status_t status);
+extern const char *efi_status_to_str(efi_status_t status);
/*
* Variable Attributes
--
2.16.4
^ permalink raw reply related
* [PATCH 0/2 v2] efi: cosmetic patches for the error messages when
From: Lee, Chun-Yi @ 2019-12-13 9:06 UTC (permalink / raw)
To: Ard Biesheuvel, James Morris, Serge E . Hallyn, David Howells,
Josh Boyer, Nayna Jain, Mimi Zohar
Cc: linux-efi, linux-security-module, linux-kernel, Lee, Chun-Yi
When loading certificates list from EFI variables, the error
messages and efi status codes always be emitted to dmesg. It looks
ugly:
[ 2.335031] Couldn't get size: 0x800000000000000e
[ 2.335032] Couldn't get UEFI MokListRT
[ 2.339985] Couldn't get size: 0x800000000000000e
[ 2.339987] Couldn't get UEFI dbx list
This cosmetic patch set moved the above messages to the error
handling code path. And, it adds a function to convert EFI status
code to a string for improving the readability of debug log. The function
can also be used in other EFI logs.
v2:
The convert function be moved to efi.c
Lee, Chun-Yi (2):
efi: add a function to convert the status code to a string
efi: show error messages only when loading certificates is failed
drivers/firmware/efi/efi.c | 32 +++++++++++++++++++++
include/linux/efi.h | 1 +
security/integrity/platform_certs/load_uefi.c | 41 ++++++++++++++-------------
3 files changed, 55 insertions(+), 19 deletions(-)
--
2.16.4
^ permalink raw reply
* Re: [PATCH] security: only build lsm_audit if CONFIG_SECURITY=y
From: James Morris @ 2019-12-12 22:04 UTC (permalink / raw)
To: Paul Moore
Cc: Stephen Smalley, selinux, linux-security-module, linux-next,
jamorris
In-Reply-To: <CAHC9VhRnqfuVUTDZA+8G-_OTqqN8M7XJhOpiO1m3t0XhY584Xw@mail.gmail.com>
On Tue, 10 Dec 2019, Paul Moore wrote:
> On Tue, Dec 10, 2019 at 11:55 AM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> > The lsm_audit code is only required when CONFIG_SECURITY is enabled.
> > It does not have a build dependency on CONFIG_AUDIT since audit.h
> > provides trivial static inlines for audit_log*() when CONFIG_AUDIT
> > is disabled. Hence, the Makefile should only add lsm_audit to the
> > obj lists based on CONFIG_SECURITY, not CONFIG_AUDIT.
> >
> > Fixes: 59438b46471a ("security,lockdown,selinux: implement SELinux lockdown")
> > Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
> > ---
> > security/Makefile | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
>
> Merged into selinux/next in order to fix the linux-next build
> breakage. James, if you would prefer a different fix, let us know.
LGTM
>
> > diff --git a/security/Makefile b/security/Makefile
> > index be1dd9d2cb2f..746438499029 100644
> > --- a/security/Makefile
> > +++ b/security/Makefile
> > @@ -22,7 +22,7 @@ obj-$(CONFIG_SECURITY) += security.o
> > obj-$(CONFIG_SECURITYFS) += inode.o
> > obj-$(CONFIG_SECURITY_SELINUX) += selinux/
> > obj-$(CONFIG_SECURITY_SMACK) += smack/
> > -obj-$(CONFIG_AUDIT) += lsm_audit.o
> > +obj-$(CONFIG_SECURITY) += lsm_audit.o
> > obj-$(CONFIG_SECURITY_TOMOYO) += tomoyo/
> > obj-$(CONFIG_SECURITY_APPARMOR) += apparmor/
> > obj-$(CONFIG_SECURITY_YAMA) += yama/
> > --
> > 2.23.0
>
> --
> paul moore
> www.paul-moore.com
>
--
James Morris
<jamorris@linuxonhyperv.com>
^ permalink raw reply
* Re: [PATCH] LSM: allow an LSM to disable all hooks at once
From: Paul Moore @ 2019-12-12 19:01 UTC (permalink / raw)
To: Stephen Smalley
Cc: Ondrej Mosnacek, Casey Schaufler, Kees Cook,
Linux Security Module list, James Morris, Serge E. Hallyn,
SElinux list, John Johansen, Micah Morton, Tetsuo Handa
In-Reply-To: <83af5f0f-d3ec-7827-92e5-2db0997b9d22@tycho.nsa.gov>
On Thu, Dec 12, 2019 at 1:48 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> On 12/12/19 1:18 PM, Paul Moore wrote:
> > On Thu, Dec 12, 2019 at 1:10 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> >>
> >> On 12/12/19 1:09 PM, Paul Moore wrote:
> >>> On Thu, Dec 12, 2019 at 12:57 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> >>>> On 12/12/19 12:54 PM, Paul Moore wrote:
> >>>>> On Thu, Dec 12, 2019 at 8:14 AM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> >>>>>> On 12/12/19 6:49 AM, Ondrej Mosnacek wrote:
> >>>>>>> On Wed, Dec 11, 2019 at 8:12 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> >>>>>>>> On 12/11/19 1:35 PM, Casey Schaufler wrote:
> >>>>>>>>> On 12/11/2019 8:42 AM, Kees Cook wrote:
> >>>>>>>>>> On Wed, Dec 11, 2019 at 09:29:10AM -0500, Stephen Smalley wrote:
> >>>>>>>>>>> On 12/11/19 9:08 AM, Ondrej Mosnacek wrote:
> >>>>>
> >>>>> ...
> >>>>>
> >>>>>>>> selinux_state.initialized reflects whether a policy has
> >>>>>>>> been loaded. With a few exceptions in certain hook functions, it is
> >>>>>>>> only checked by the security server service functions
> >>>>>>>> (security/selinux/ss/services.c) prior to accessing the policydb. So
> >>>>>>>> there is a lot of SELinux processing that would still occur in that
> >>>>>>>> situation unless we added if (!selinux_state.initialized) return 0;
> >>>>>>>> checks to all the hook functions, which would create the same exposure
> >>>>>>>> and would further break the SELinux-enabled case (we need to perform
> >>>>>>>> some SELinux processing pre-policy-load to allocate blobs and track what
> >>>>>>>> tasks and objects require delayed security initialization when policy
> >>>>>>>> load finally occurs).
> >>>>>>>
> >>>>>>> I think what Casey was suggesting is to add another flag that would
> >>>>>>> switch from "no policy loaded, but we expect it to be loaded
> >>>>>>> eventually" to "no policy loaded and we don't expect/allow it to be
> >>>>>>> loaded any more", which is essentially equivalent to checking
> >>>>>>> selinux_enabled in each hook, which you had already brought up.
> >>>>>>
> >>>>>> Yep. if (!selinux_enabled) return 0; or if (selinux_state.disabled)
> >>>>>> return 0; under #ifdef CONFIG_SECURITY_SELINUX_DISABLE in every hook
> >>>>>> might be the best option until it can be removed altogether; avoids
> >>>>>> impacting the LSM framework or any other security module, preserves the
> >>>>>> existing functionality, fairly low overhead on the SELinux-disabled case.
> >>>>>
> >>>>> Just so I'm understanding this thread correctly, the above change
> >>>>> (adding enabled checks to each SELinux hook implementation) is only
> >>>>> until Fedora can figure out a way to deprecate and remove the runtime
> >>>>> disable?
> >>>>
> >>>> That's my understanding. In the interim, Android kernels should already
> >>>> be disabling CONFIG_SECURITY_SELINUX_DISABLE and other distros may
> >>>> choose to disable it as long as they don't care about supporting SELinux
> >>>> runtime disable.
> >>>
> >>> Okay, I just wanted to make sure I wasn't missing something.
> >>> Honestly, I'd rather Fedora just go ahead and do whatever it is they
> >>> need to do to turn off CONFIG_SECURITY_SELINUX_DISABLE (it sounds like
> >>> they have a plan and are working on it), I'm not overly excited about
> >>> temporarily cluttering up the code with additional "enabled" checks
> >>> when the status quo works, even if it is less than ideal.
> >>
> >> The status quo is producing kernel crashes, courtesy of LSM stacking
> >> changes...
> >
> > How prevalent are these crashes?
> >
> > This also only happens when disabling SELinux at runtime, yes?
> > Something we've advised against for some time now and are working to
> > eliminate? Let's just get rid of the runtime disable *soon*, and if
> > we need a stop-gap fix let's just go with the hook reordering since
> > that seems to minimize the impact, if not resolve it.
>
> Not optimistic given that the original bug was opened 2.5+ years ago,
> commenters identified fairly significant challenges to removing the
> support, and no visible progress was ever made. I suppose the hook
> reordering will do but seems fragile and hacky. Whatever.
Based on Ondrej's comments in this thread it sounded as if there was
some renewed interest in actually eliminating it from Fedora this
time. If that's not the case, perhaps it's time to start that effort
back up, and if we can't ever get away from the runtime disable then
we can take the step of admitting that everything is terrible and we
can consider some of the options presented in this thread.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH] LSM: allow an LSM to disable all hooks at once
From: Stephen Smalley @ 2019-12-12 18:48 UTC (permalink / raw)
To: Paul Moore
Cc: Ondrej Mosnacek, Casey Schaufler, Kees Cook,
Linux Security Module list, James Morris, Serge E. Hallyn,
SElinux list, John Johansen, Micah Morton, Tetsuo Handa
In-Reply-To: <CAHC9VhQG9zZEL53XRdLHdmFJDpg8qAd9p61Xkm5AdSgM=-5eAg@mail.gmail.com>
On 12/12/19 1:18 PM, Paul Moore wrote:
> On Thu, Dec 12, 2019 at 1:10 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
>>
>> On 12/12/19 1:09 PM, Paul Moore wrote:
>>> On Thu, Dec 12, 2019 at 12:57 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
>>>> On 12/12/19 12:54 PM, Paul Moore wrote:
>>>>> On Thu, Dec 12, 2019 at 8:14 AM Stephen Smalley <sds@tycho.nsa.gov> wrote:
>>>>>> On 12/12/19 6:49 AM, Ondrej Mosnacek wrote:
>>>>>>> On Wed, Dec 11, 2019 at 8:12 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
>>>>>>>> On 12/11/19 1:35 PM, Casey Schaufler wrote:
>>>>>>>>> On 12/11/2019 8:42 AM, Kees Cook wrote:
>>>>>>>>>> On Wed, Dec 11, 2019 at 09:29:10AM -0500, Stephen Smalley wrote:
>>>>>>>>>>> On 12/11/19 9:08 AM, Ondrej Mosnacek wrote:
>>>>>
>>>>> ...
>>>>>
>>>>>>>> selinux_state.initialized reflects whether a policy has
>>>>>>>> been loaded. With a few exceptions in certain hook functions, it is
>>>>>>>> only checked by the security server service functions
>>>>>>>> (security/selinux/ss/services.c) prior to accessing the policydb. So
>>>>>>>> there is a lot of SELinux processing that would still occur in that
>>>>>>>> situation unless we added if (!selinux_state.initialized) return 0;
>>>>>>>> checks to all the hook functions, which would create the same exposure
>>>>>>>> and would further break the SELinux-enabled case (we need to perform
>>>>>>>> some SELinux processing pre-policy-load to allocate blobs and track what
>>>>>>>> tasks and objects require delayed security initialization when policy
>>>>>>>> load finally occurs).
>>>>>>>
>>>>>>> I think what Casey was suggesting is to add another flag that would
>>>>>>> switch from "no policy loaded, but we expect it to be loaded
>>>>>>> eventually" to "no policy loaded and we don't expect/allow it to be
>>>>>>> loaded any more", which is essentially equivalent to checking
>>>>>>> selinux_enabled in each hook, which you had already brought up.
>>>>>>
>>>>>> Yep. if (!selinux_enabled) return 0; or if (selinux_state.disabled)
>>>>>> return 0; under #ifdef CONFIG_SECURITY_SELINUX_DISABLE in every hook
>>>>>> might be the best option until it can be removed altogether; avoids
>>>>>> impacting the LSM framework or any other security module, preserves the
>>>>>> existing functionality, fairly low overhead on the SELinux-disabled case.
>>>>>
>>>>> Just so I'm understanding this thread correctly, the above change
>>>>> (adding enabled checks to each SELinux hook implementation) is only
>>>>> until Fedora can figure out a way to deprecate and remove the runtime
>>>>> disable?
>>>>
>>>> That's my understanding. In the interim, Android kernels should already
>>>> be disabling CONFIG_SECURITY_SELINUX_DISABLE and other distros may
>>>> choose to disable it as long as they don't care about supporting SELinux
>>>> runtime disable.
>>>
>>> Okay, I just wanted to make sure I wasn't missing something.
>>> Honestly, I'd rather Fedora just go ahead and do whatever it is they
>>> need to do to turn off CONFIG_SECURITY_SELINUX_DISABLE (it sounds like
>>> they have a plan and are working on it), I'm not overly excited about
>>> temporarily cluttering up the code with additional "enabled" checks
>>> when the status quo works, even if it is less than ideal.
>>
>> The status quo is producing kernel crashes, courtesy of LSM stacking
>> changes...
>
> How prevalent are these crashes?
>
> This also only happens when disabling SELinux at runtime, yes?
> Something we've advised against for some time now and are working to
> eliminate? Let's just get rid of the runtime disable *soon*, and if
> we need a stop-gap fix let's just go with the hook reordering since
> that seems to minimize the impact, if not resolve it.
Not optimistic given that the original bug was opened 2.5+ years ago,
commenters identified fairly significant challenges to removing the
support, and no visible progress was ever made. I suppose the hook
reordering will do but seems fragile and hacky. Whatever.
> I'm not going to comment on the stacking changes.
^ permalink raw reply
* Re: [PATCH] LSM: allow an LSM to disable all hooks at once
From: Casey Schaufler @ 2019-12-12 18:28 UTC (permalink / raw)
To: Stephen Smalley, Paul Moore, Ondrej Mosnacek
Cc: Kees Cook, Linux Security Module list, James Morris,
Serge E. Hallyn, SElinux list, John Johansen, Micah Morton,
Tetsuo Handa, Casey Schaufler
In-Reply-To: <83d047ce-0ca0-4152-1da7-32798c500aab@tycho.nsa.gov>
On 12/12/2019 10:11 AM, Stephen Smalley wrote:
> On 12/12/19 1:09 PM, Paul Moore wrote:
>> On Thu, Dec 12, 2019 at 12:57 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
>>> On 12/12/19 12:54 PM, Paul Moore wrote:
>>>> On Thu, Dec 12, 2019 at 8:14 AM Stephen Smalley <sds@tycho.nsa.gov> wrote:
>>>>> On 12/12/19 6:49 AM, Ondrej Mosnacek wrote:
>>>>>> On Wed, Dec 11, 2019 at 8:12 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
>>>>>>> On 12/11/19 1:35 PM, Casey Schaufler wrote:
>>>>>>>> On 12/11/2019 8:42 AM, Kees Cook wrote:
>>>>>>>>> On Wed, Dec 11, 2019 at 09:29:10AM -0500, Stephen Smalley wrote:
>>>>>>>>>> On 12/11/19 9:08 AM, Ondrej Mosnacek wrote:
>>>>
>>>> ...
>>>>
>>>>>>> selinux_state.initialized reflects whether a policy has
>>>>>>> been loaded. With a few exceptions in certain hook functions, it is
>>>>>>> only checked by the security server service functions
>>>>>>> (security/selinux/ss/services.c) prior to accessing the policydb. So
>>>>>>> there is a lot of SELinux processing that would still occur in that
>>>>>>> situation unless we added if (!selinux_state.initialized) return 0;
>>>>>>> checks to all the hook functions, which would create the same exposure
>>>>>>> and would further break the SELinux-enabled case (we need to perform
>>>>>>> some SELinux processing pre-policy-load to allocate blobs and track what
>>>>>>> tasks and objects require delayed security initialization when policy
>>>>>>> load finally occurs).
>>>>>>
>>>>>> I think what Casey was suggesting is to add another flag that would
>>>>>> switch from "no policy loaded, but we expect it to be loaded
>>>>>> eventually" to "no policy loaded and we don't expect/allow it to be
>>>>>> loaded any more", which is essentially equivalent to checking
>>>>>> selinux_enabled in each hook, which you had already brought up.
>>>>>
>>>>> Yep. if (!selinux_enabled) return 0; or if (selinux_state.disabled)
>>>>> return 0; under #ifdef CONFIG_SECURITY_SELINUX_DISABLE in every hook
>>>>> might be the best option until it can be removed altogether; avoids
>>>>> impacting the LSM framework or any other security module, preserves the
>>>>> existing functionality, fairly low overhead on the SELinux-disabled case.
>>>>
>>>> Just so I'm understanding this thread correctly, the above change
>>>> (adding enabled checks to each SELinux hook implementation) is only
>>>> until Fedora can figure out a way to deprecate and remove the runtime
>>>> disable?
>>>
>>> That's my understanding. In the interim, Android kernels should already
>>> be disabling CONFIG_SECURITY_SELINUX_DISABLE and other distros may
>>> choose to disable it as long as they don't care about supporting SELinux
>>> runtime disable.
>>
>> Okay, I just wanted to make sure I wasn't missing something.
>> Honestly, I'd rather Fedora just go ahead and do whatever it is they
>> need to do to turn off CONFIG_SECURITY_SELINUX_DISABLE (it sounds like
>> they have a plan and are working on it), I'm not overly excited about
>> temporarily cluttering up the code with additional "enabled" checks
>> when the status quo works, even if it is less than ideal.
>
> The status quo is producing kernel crashes, courtesy of LSM stacking changes...
... that went in 4+ years ago and are just now showing problems.
Either something's changed or no one has really cared in the interim.
^ permalink raw reply
* Re: [PATCH] LSM: allow an LSM to disable all hooks at once
From: Paul Moore @ 2019-12-12 18:18 UTC (permalink / raw)
To: Stephen Smalley
Cc: Ondrej Mosnacek, Casey Schaufler, Kees Cook,
Linux Security Module list, James Morris, Serge E. Hallyn,
SElinux list, John Johansen, Micah Morton, Tetsuo Handa
In-Reply-To: <83d047ce-0ca0-4152-1da7-32798c500aab@tycho.nsa.gov>
On Thu, Dec 12, 2019 at 1:10 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
>
> On 12/12/19 1:09 PM, Paul Moore wrote:
> > On Thu, Dec 12, 2019 at 12:57 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> >> On 12/12/19 12:54 PM, Paul Moore wrote:
> >>> On Thu, Dec 12, 2019 at 8:14 AM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> >>>> On 12/12/19 6:49 AM, Ondrej Mosnacek wrote:
> >>>>> On Wed, Dec 11, 2019 at 8:12 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> >>>>>> On 12/11/19 1:35 PM, Casey Schaufler wrote:
> >>>>>>> On 12/11/2019 8:42 AM, Kees Cook wrote:
> >>>>>>>> On Wed, Dec 11, 2019 at 09:29:10AM -0500, Stephen Smalley wrote:
> >>>>>>>>> On 12/11/19 9:08 AM, Ondrej Mosnacek wrote:
> >>>
> >>> ...
> >>>
> >>>>>> selinux_state.initialized reflects whether a policy has
> >>>>>> been loaded. With a few exceptions in certain hook functions, it is
> >>>>>> only checked by the security server service functions
> >>>>>> (security/selinux/ss/services.c) prior to accessing the policydb. So
> >>>>>> there is a lot of SELinux processing that would still occur in that
> >>>>>> situation unless we added if (!selinux_state.initialized) return 0;
> >>>>>> checks to all the hook functions, which would create the same exposure
> >>>>>> and would further break the SELinux-enabled case (we need to perform
> >>>>>> some SELinux processing pre-policy-load to allocate blobs and track what
> >>>>>> tasks and objects require delayed security initialization when policy
> >>>>>> load finally occurs).
> >>>>>
> >>>>> I think what Casey was suggesting is to add another flag that would
> >>>>> switch from "no policy loaded, but we expect it to be loaded
> >>>>> eventually" to "no policy loaded and we don't expect/allow it to be
> >>>>> loaded any more", which is essentially equivalent to checking
> >>>>> selinux_enabled in each hook, which you had already brought up.
> >>>>
> >>>> Yep. if (!selinux_enabled) return 0; or if (selinux_state.disabled)
> >>>> return 0; under #ifdef CONFIG_SECURITY_SELINUX_DISABLE in every hook
> >>>> might be the best option until it can be removed altogether; avoids
> >>>> impacting the LSM framework or any other security module, preserves the
> >>>> existing functionality, fairly low overhead on the SELinux-disabled case.
> >>>
> >>> Just so I'm understanding this thread correctly, the above change
> >>> (adding enabled checks to each SELinux hook implementation) is only
> >>> until Fedora can figure out a way to deprecate and remove the runtime
> >>> disable?
> >>
> >> That's my understanding. In the interim, Android kernels should already
> >> be disabling CONFIG_SECURITY_SELINUX_DISABLE and other distros may
> >> choose to disable it as long as they don't care about supporting SELinux
> >> runtime disable.
> >
> > Okay, I just wanted to make sure I wasn't missing something.
> > Honestly, I'd rather Fedora just go ahead and do whatever it is they
> > need to do to turn off CONFIG_SECURITY_SELINUX_DISABLE (it sounds like
> > they have a plan and are working on it), I'm not overly excited about
> > temporarily cluttering up the code with additional "enabled" checks
> > when the status quo works, even if it is less than ideal.
>
> The status quo is producing kernel crashes, courtesy of LSM stacking
> changes...
How prevalent are these crashes?
This also only happens when disabling SELinux at runtime, yes?
Something we've advised against for some time now and are working to
eliminate? Let's just get rid of the runtime disable *soon*, and if
we need a stop-gap fix let's just go with the hook reordering since
that seems to minimize the impact, if not resolve it.
I'm not going to comment on the stacking changes.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH] LSM: allow an LSM to disable all hooks at once
From: Stephen Smalley @ 2019-12-12 18:11 UTC (permalink / raw)
To: Paul Moore, Ondrej Mosnacek
Cc: Casey Schaufler, Kees Cook, Linux Security Module list,
James Morris, Serge E. Hallyn, SElinux list, John Johansen,
Micah Morton, Tetsuo Handa
In-Reply-To: <CAHC9VhT24b6YYTcE-h9pS9HnJ35unW_14EYLcNBBd-xUa=1L9A@mail.gmail.com>
On 12/12/19 1:09 PM, Paul Moore wrote:
> On Thu, Dec 12, 2019 at 12:57 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
>> On 12/12/19 12:54 PM, Paul Moore wrote:
>>> On Thu, Dec 12, 2019 at 8:14 AM Stephen Smalley <sds@tycho.nsa.gov> wrote:
>>>> On 12/12/19 6:49 AM, Ondrej Mosnacek wrote:
>>>>> On Wed, Dec 11, 2019 at 8:12 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
>>>>>> On 12/11/19 1:35 PM, Casey Schaufler wrote:
>>>>>>> On 12/11/2019 8:42 AM, Kees Cook wrote:
>>>>>>>> On Wed, Dec 11, 2019 at 09:29:10AM -0500, Stephen Smalley wrote:
>>>>>>>>> On 12/11/19 9:08 AM, Ondrej Mosnacek wrote:
>>>
>>> ...
>>>
>>>>>> selinux_state.initialized reflects whether a policy has
>>>>>> been loaded. With a few exceptions in certain hook functions, it is
>>>>>> only checked by the security server service functions
>>>>>> (security/selinux/ss/services.c) prior to accessing the policydb. So
>>>>>> there is a lot of SELinux processing that would still occur in that
>>>>>> situation unless we added if (!selinux_state.initialized) return 0;
>>>>>> checks to all the hook functions, which would create the same exposure
>>>>>> and would further break the SELinux-enabled case (we need to perform
>>>>>> some SELinux processing pre-policy-load to allocate blobs and track what
>>>>>> tasks and objects require delayed security initialization when policy
>>>>>> load finally occurs).
>>>>>
>>>>> I think what Casey was suggesting is to add another flag that would
>>>>> switch from "no policy loaded, but we expect it to be loaded
>>>>> eventually" to "no policy loaded and we don't expect/allow it to be
>>>>> loaded any more", which is essentially equivalent to checking
>>>>> selinux_enabled in each hook, which you had already brought up.
>>>>
>>>> Yep. if (!selinux_enabled) return 0; or if (selinux_state.disabled)
>>>> return 0; under #ifdef CONFIG_SECURITY_SELINUX_DISABLE in every hook
>>>> might be the best option until it can be removed altogether; avoids
>>>> impacting the LSM framework or any other security module, preserves the
>>>> existing functionality, fairly low overhead on the SELinux-disabled case.
>>>
>>> Just so I'm understanding this thread correctly, the above change
>>> (adding enabled checks to each SELinux hook implementation) is only
>>> until Fedora can figure out a way to deprecate and remove the runtime
>>> disable?
>>
>> That's my understanding. In the interim, Android kernels should already
>> be disabling CONFIG_SECURITY_SELINUX_DISABLE and other distros may
>> choose to disable it as long as they don't care about supporting SELinux
>> runtime disable.
>
> Okay, I just wanted to make sure I wasn't missing something.
> Honestly, I'd rather Fedora just go ahead and do whatever it is they
> need to do to turn off CONFIG_SECURITY_SELINUX_DISABLE (it sounds like
> they have a plan and are working on it), I'm not overly excited about
> temporarily cluttering up the code with additional "enabled" checks
> when the status quo works, even if it is less than ideal.
The status quo is producing kernel crashes, courtesy of LSM stacking
changes...
^ permalink raw reply
* Re: [PATCH] LSM: allow an LSM to disable all hooks at once
From: Paul Moore @ 2019-12-12 18:09 UTC (permalink / raw)
To: Stephen Smalley, Ondrej Mosnacek
Cc: Casey Schaufler, Kees Cook, Linux Security Module list,
James Morris, Serge E. Hallyn, SElinux list, John Johansen,
Micah Morton, Tetsuo Handa
In-Reply-To: <0f0778af-73c2-3c75-30c0-da5eae203032@tycho.nsa.gov>
On Thu, Dec 12, 2019 at 12:57 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> On 12/12/19 12:54 PM, Paul Moore wrote:
> > On Thu, Dec 12, 2019 at 8:14 AM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> >> On 12/12/19 6:49 AM, Ondrej Mosnacek wrote:
> >>> On Wed, Dec 11, 2019 at 8:12 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> >>>> On 12/11/19 1:35 PM, Casey Schaufler wrote:
> >>>>> On 12/11/2019 8:42 AM, Kees Cook wrote:
> >>>>>> On Wed, Dec 11, 2019 at 09:29:10AM -0500, Stephen Smalley wrote:
> >>>>>>> On 12/11/19 9:08 AM, Ondrej Mosnacek wrote:
> >
> > ...
> >
> >>>> selinux_state.initialized reflects whether a policy has
> >>>> been loaded. With a few exceptions in certain hook functions, it is
> >>>> only checked by the security server service functions
> >>>> (security/selinux/ss/services.c) prior to accessing the policydb. So
> >>>> there is a lot of SELinux processing that would still occur in that
> >>>> situation unless we added if (!selinux_state.initialized) return 0;
> >>>> checks to all the hook functions, which would create the same exposure
> >>>> and would further break the SELinux-enabled case (we need to perform
> >>>> some SELinux processing pre-policy-load to allocate blobs and track what
> >>>> tasks and objects require delayed security initialization when policy
> >>>> load finally occurs).
> >>>
> >>> I think what Casey was suggesting is to add another flag that would
> >>> switch from "no policy loaded, but we expect it to be loaded
> >>> eventually" to "no policy loaded and we don't expect/allow it to be
> >>> loaded any more", which is essentially equivalent to checking
> >>> selinux_enabled in each hook, which you had already brought up.
> >>
> >> Yep. if (!selinux_enabled) return 0; or if (selinux_state.disabled)
> >> return 0; under #ifdef CONFIG_SECURITY_SELINUX_DISABLE in every hook
> >> might be the best option until it can be removed altogether; avoids
> >> impacting the LSM framework or any other security module, preserves the
> >> existing functionality, fairly low overhead on the SELinux-disabled case.
> >
> > Just so I'm understanding this thread correctly, the above change
> > (adding enabled checks to each SELinux hook implementation) is only
> > until Fedora can figure out a way to deprecate and remove the runtime
> > disable?
>
> That's my understanding. In the interim, Android kernels should already
> be disabling CONFIG_SECURITY_SELINUX_DISABLE and other distros may
> choose to disable it as long as they don't care about supporting SELinux
> runtime disable.
Okay, I just wanted to make sure I wasn't missing something.
Honestly, I'd rather Fedora just go ahead and do whatever it is they
need to do to turn off CONFIG_SECURITY_SELINUX_DISABLE (it sounds like
they have a plan and are working on it), I'm not overly excited about
temporarily cluttering up the code with additional "enabled" checks
when the status quo works, even if it is less than ideal.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH] LSM: allow an LSM to disable all hooks at once
From: Stephen Smalley @ 2019-12-12 17:57 UTC (permalink / raw)
To: Paul Moore
Cc: Ondrej Mosnacek, Casey Schaufler, Kees Cook,
Linux Security Module list, James Morris, Serge E. Hallyn,
SElinux list, John Johansen, Micah Morton, Tetsuo Handa
In-Reply-To: <CAHC9VhT_CBQN+aFWjpPixi9Ok3Z7bQ-053AHg4pvqVtn-RdVVA@mail.gmail.com>
On 12/12/19 12:54 PM, Paul Moore wrote:
> On Thu, Dec 12, 2019 at 8:14 AM Stephen Smalley <sds@tycho.nsa.gov> wrote:
>> On 12/12/19 6:49 AM, Ondrej Mosnacek wrote:
>>> On Wed, Dec 11, 2019 at 8:12 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
>>>> On 12/11/19 1:35 PM, Casey Schaufler wrote:
>>>>> On 12/11/2019 8:42 AM, Kees Cook wrote:
>>>>>> On Wed, Dec 11, 2019 at 09:29:10AM -0500, Stephen Smalley wrote:
>>>>>>> On 12/11/19 9:08 AM, Ondrej Mosnacek wrote:
>
> ...
>
>>>> selinux_state.initialized reflects whether a policy has
>>>> been loaded. With a few exceptions in certain hook functions, it is
>>>> only checked by the security server service functions
>>>> (security/selinux/ss/services.c) prior to accessing the policydb. So
>>>> there is a lot of SELinux processing that would still occur in that
>>>> situation unless we added if (!selinux_state.initialized) return 0;
>>>> checks to all the hook functions, which would create the same exposure
>>>> and would further break the SELinux-enabled case (we need to perform
>>>> some SELinux processing pre-policy-load to allocate blobs and track what
>>>> tasks and objects require delayed security initialization when policy
>>>> load finally occurs).
>>>
>>> I think what Casey was suggesting is to add another flag that would
>>> switch from "no policy loaded, but we expect it to be loaded
>>> eventually" to "no policy loaded and we don't expect/allow it to be
>>> loaded any more", which is essentially equivalent to checking
>>> selinux_enabled in each hook, which you had already brought up.
>>
>> Yep. if (!selinux_enabled) return 0; or if (selinux_state.disabled)
>> return 0; under #ifdef CONFIG_SECURITY_SELINUX_DISABLE in every hook
>> might be the best option until it can be removed altogether; avoids
>> impacting the LSM framework or any other security module, preserves the
>> existing functionality, fairly low overhead on the SELinux-disabled case.
>
> Just so I'm understanding this thread correctly, the above change
> (adding enabled checks to each SELinux hook implementation) is only
> until Fedora can figure out a way to deprecate and remove the runtime
> disable?
That's my understanding. In the interim, Android kernels should already
be disabling CONFIG_SECURITY_SELINUX_DISABLE and other distros may
choose to disable it as long as they don't care about supporting SELinux
runtime disable.
^ permalink raw reply
* Re: [PATCH] LSM: allow an LSM to disable all hooks at once
From: Paul Moore @ 2019-12-12 17:54 UTC (permalink / raw)
To: Stephen Smalley
Cc: Ondrej Mosnacek, Casey Schaufler, Kees Cook,
Linux Security Module list, James Morris, Serge E. Hallyn,
SElinux list, John Johansen, Micah Morton, Tetsuo Handa
In-Reply-To: <1f613260-d315-6925-d069-e92b872b8610@tycho.nsa.gov>
On Thu, Dec 12, 2019 at 8:14 AM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> On 12/12/19 6:49 AM, Ondrej Mosnacek wrote:
> > On Wed, Dec 11, 2019 at 8:12 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> >> On 12/11/19 1:35 PM, Casey Schaufler wrote:
> >>> On 12/11/2019 8:42 AM, Kees Cook wrote:
> >>>> On Wed, Dec 11, 2019 at 09:29:10AM -0500, Stephen Smalley wrote:
> >>>>> On 12/11/19 9:08 AM, Ondrej Mosnacek wrote:
...
> >> selinux_state.initialized reflects whether a policy has
> >> been loaded. With a few exceptions in certain hook functions, it is
> >> only checked by the security server service functions
> >> (security/selinux/ss/services.c) prior to accessing the policydb. So
> >> there is a lot of SELinux processing that would still occur in that
> >> situation unless we added if (!selinux_state.initialized) return 0;
> >> checks to all the hook functions, which would create the same exposure
> >> and would further break the SELinux-enabled case (we need to perform
> >> some SELinux processing pre-policy-load to allocate blobs and track what
> >> tasks and objects require delayed security initialization when policy
> >> load finally occurs).
> >
> > I think what Casey was suggesting is to add another flag that would
> > switch from "no policy loaded, but we expect it to be loaded
> > eventually" to "no policy loaded and we don't expect/allow it to be
> > loaded any more", which is essentially equivalent to checking
> > selinux_enabled in each hook, which you had already brought up.
>
> Yep. if (!selinux_enabled) return 0; or if (selinux_state.disabled)
> return 0; under #ifdef CONFIG_SECURITY_SELINUX_DISABLE in every hook
> might be the best option until it can be removed altogether; avoids
> impacting the LSM framework or any other security module, preserves the
> existing functionality, fairly low overhead on the SELinux-disabled case.
Just so I'm understanding this thread correctly, the above change
(adding enabled checks to each SELinux hook implementation) is only
until Fedora can figure out a way to deprecate and remove the runtime
disable?
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH] LSM: allow an LSM to disable all hooks at once
From: Stephen Smalley @ 2019-12-12 17:39 UTC (permalink / raw)
To: Ondrej Mosnacek
Cc: Casey Schaufler, Kees Cook, Linux Security Module list,
James Morris, Serge E. Hallyn, SElinux list, Paul Moore,
John Johansen, Micah Morton, Tetsuo Handa
In-Reply-To: <CAFqZXNsZvTfeL_ST7FSxbgM28E3RzKrF1f4JqYUhVY7++01NMw@mail.gmail.com>
On 12/12/19 11:03 AM, Ondrej Mosnacek wrote:
> On Thu, Dec 12, 2019 at 2:14 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
>> On 12/12/19 6:49 AM, Ondrej Mosnacek wrote:
>>> On Wed, Dec 11, 2019 at 8:12 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
>>>> On 12/11/19 1:35 PM, Casey Schaufler wrote:
>>>>> On 12/11/2019 8:42 AM, Kees Cook wrote:
>>>>>> On Wed, Dec 11, 2019 at 09:29:10AM -0500, Stephen Smalley wrote:
>>>>>>> On 12/11/19 9:08 AM, Ondrej Mosnacek wrote:
>>>>>>>> Instead of deleting the hooks from each list one-by-one (which creates
>>>>>>>> some bad race conditions), allow an LSM to provide a reference to its
>>>>>>>> "enabled" variable and check this variable before calling the hook.
>>>>>>>>
>>>>>>>> As a nice side effect, this allows marking the hooks (and other stuff)
>>>>>>>> __ro_after_init unconditionally. Since SECURITY_WRITABLE_HOOKS no longer
>>>>>>>> makes sense, remove it and instead use SECURITY_SELINUX_DISABLE directly
>>>>>>>> for turning on the runtime disable functionality, to emphasize that this
>>>>>>>> is only used by SELinux and is meant to be removed in the future.
>>>>>>> Is this fundamentally different/better than adding if (!selinux_enabled)
>>>>>>> return 0; to the beginning of every SELinux hook function? And as I noted
>>>>>>> to Casey in the earlier thread, that provides an additional easy target to
>>>>>>> kernel exploit writers for neutering SELinux with a single kernel write
>>>>>>> vulnerability. OTOH, they already have selinux_state.enforcing and friends,
>>>>>>> and this new one would only be if SECURITY_SELINUX_DISABLE=y.
>>>>>> Yeah, I agree here -- we specifically do not want there to be a trivial
>>>>>> way to disable LSMs at runtime. CONFIG_SECURITY_SELINUX_DISABLE should
>>>>>> be considered deprecated IMO, and we don't want to widen its features.
>>>>>
>>>>> In /etc/selinux/config SELINUX=disabled is documented as "No SELinux
>>>>> policy is loaded". How about if instead of blocking policy load and
>>>>> removing the hooks it just blocked policy load? It may be appropriate
>>>>> to tweak the code a bit to perform better in the no-policy loaded
>>>>> case, but my understanding is that the system should work. That would
>>>>> address backward compatibility concerns and allow removal of
>>>>> security_delete_hooks(). I don't think this would have the same
>>>>> exposure of resetting selinux_enabled.
>>>>
>>>> I think that comment stems from before runtime disable was first
>>>> implemented in the kernel, when the only option was to leave SELinux
>>>> enabled with no policy loaded. Fedora didn't consider that (or
>>>> selinux=0) to be acceptable alternatives, which is why we have runtime
>>>> disable today.
>>>
>>> Do you happen to remember the reasons why it wasn't acceptable? We are
>>> ready to start pushing for disabling SECURITY_SELINUX_DISABLE in
>>> Fedora, but we're not sure why it is so crucial. Knowing what we need
>>> to address before disabling/removing it would help a lot.
>>
>> IIRC, they considered the selinux=0 kernel boot parameter to be
>> inadequate because of the difficulty in changing kernel boot parameters
>> on certain platforms (IBM?). The no-policy-loaded alternative still
>> left a lot of SELinux processing in place, so users would still end up
>> paying memory and runtime overheads for no benefit if we only skipped
>> policy load.
>
> Thanks, I was worried that there was also something more tricky than
> this. We could make adding-removing the kernel parameter easier on
> Fedora by creating and maintaining a tool that would be able to do it
> reliably across the supported arches. That shouldn't be too hard,
> hopefully.
>
>>
>>>> selinux_state.initialized reflects whether a policy has
>>>> been loaded. With a few exceptions in certain hook functions, it is
>>>> only checked by the security server service functions
>>>> (security/selinux/ss/services.c) prior to accessing the policydb. So
>>>> there is a lot of SELinux processing that would still occur in that
>>>> situation unless we added if (!selinux_state.initialized) return 0;
>>>> checks to all the hook functions, which would create the same exposure
>>>> and would further break the SELinux-enabled case (we need to perform
>>>> some SELinux processing pre-policy-load to allocate blobs and track what
>>>> tasks and objects require delayed security initialization when policy
>>>> load finally occurs).
>>>
>>> I think what Casey was suggesting is to add another flag that would
>>> switch from "no policy loaded, but we expect it to be loaded
>>> eventually" to "no policy loaded and we don't expect/allow it to be
>>> loaded any more", which is essentially equivalent to checking
>>> selinux_enabled in each hook, which you had already brought up.
>>
>> Yep. if (!selinux_enabled) return 0; or if (selinux_state.disabled)
>> return 0; under #ifdef CONFIG_SECURITY_SELINUX_DISABLE in every hook
>> might be the best option until it can be removed altogether; avoids
>> impacting the LSM framework or any other security module, preserves the
>> existing functionality, fairly low overhead on the SELinux-disabled case.
>
> OK, so I'll put together another patch that removes all the
> security_delete_hooks() stuff and adds the checks.
>
>>
>> NB selinux_enabled was originally just for selinux=0 handling and thus
>> remains global (not per selinux-namespace). selinux_state.disabled is
>> only for runtime disable via selinuxfs, which could be applied per
>> selinux-namespace if/when selinux namespaces are ever completed and
>> merged. Aside from clearing selinux_enabled in selinux_disable() and
>> logging selinux_enabled in sel_write_enforce() - which seems pointless
>> by the way, there are no other uses of selinux_enabled outside of __init
>> code AFAICS. I think at one time selinux_enabled was exported for use
>> by other kernel code related to SECMARK or elsewhere but that was
>> eliminated/generalized for other security modules. So it seems like we
>> could always make selinux_enabled itself ro_after_init, stop clearing it
>> in selinux_disable() since nothing will be testing it, and just use
>> selinux_state.disabled in the hooks (and possibly in the
>> sel_write_enforce audit log).
>
> Yes, that sounds reasonable.
And maybe we should rename selinux_enabled to selinux_enabled_boot for
clarity and to match selinux_enforcing_boot (similarly renamed earlier).
^ permalink raw reply
* Re: [PATCH] LSM: allow an LSM to disable all hooks at once
From: Casey Schaufler @ 2019-12-12 16:51 UTC (permalink / raw)
To: Ondrej Mosnacek, Stephen Smalley
Cc: Kees Cook, Linux Security Module list, James Morris,
Serge E. Hallyn, SElinux list, Paul Moore, John Johansen,
Micah Morton, Tetsuo Handa, Casey Schaufler
In-Reply-To: <CAFqZXNsZvTfeL_ST7FSxbgM28E3RzKrF1f4JqYUhVY7++01NMw@mail.gmail.com>
On 12/12/2019 8:03 AM, Ondrej Mosnacek wrote:
> On Thu, Dec 12, 2019 at 2:14 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
>> On 12/12/19 6:49 AM, Ondrej Mosnacek wrote:
>>> On Wed, Dec 11, 2019 at 8:12 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
>>>> On 12/11/19 1:35 PM, Casey Schaufler wrote:
>>>>> On 12/11/2019 8:42 AM, Kees Cook wrote:
>>>>>> On Wed, Dec 11, 2019 at 09:29:10AM -0500, Stephen Smalley wrote:
>>>>>>> On 12/11/19 9:08 AM, Ondrej Mosnacek wrote:
>>>>>>>> Instead of deleting the hooks from each list one-by-one (which creates
>>>>>>>> some bad race conditions), allow an LSM to provide a reference to its
>>>>>>>> "enabled" variable and check this variable before calling the hook.
>>>>>>>>
>>>>>>>> As a nice side effect, this allows marking the hooks (and other stuff)
>>>>>>>> __ro_after_init unconditionally. Since SECURITY_WRITABLE_HOOKS no longer
>>>>>>>> makes sense, remove it and instead use SECURITY_SELINUX_DISABLE directly
>>>>>>>> for turning on the runtime disable functionality, to emphasize that this
>>>>>>>> is only used by SELinux and is meant to be removed in the future.
>>>>>>> Is this fundamentally different/better than adding if (!selinux_enabled)
>>>>>>> return 0; to the beginning of every SELinux hook function? And as I noted
>>>>>>> to Casey in the earlier thread, that provides an additional easy target to
>>>>>>> kernel exploit writers for neutering SELinux with a single kernel write
>>>>>>> vulnerability. OTOH, they already have selinux_state.enforcing and friends,
>>>>>>> and this new one would only be if SECURITY_SELINUX_DISABLE=y.
>>>>>> Yeah, I agree here -- we specifically do not want there to be a trivial
>>>>>> way to disable LSMs at runtime. CONFIG_SECURITY_SELINUX_DISABLE should
>>>>>> be considered deprecated IMO, and we don't want to widen its features.
>>>>> In /etc/selinux/config SELINUX=disabled is documented as "No SELinux
>>>>> policy is loaded". How about if instead of blocking policy load and
>>>>> removing the hooks it just blocked policy load? It may be appropriate
>>>>> to tweak the code a bit to perform better in the no-policy loaded
>>>>> case, but my understanding is that the system should work. That would
>>>>> address backward compatibility concerns and allow removal of
>>>>> security_delete_hooks(). I don't think this would have the same
>>>>> exposure of resetting selinux_enabled.
>>>> I think that comment stems from before runtime disable was first
>>>> implemented in the kernel, when the only option was to leave SELinux
>>>> enabled with no policy loaded. Fedora didn't consider that (or
>>>> selinux=0) to be acceptable alternatives, which is why we have runtime
>>>> disable today.
>>> Do you happen to remember the reasons why it wasn't acceptable? We are
>>> ready to start pushing for disabling SECURITY_SELINUX_DISABLE in
>>> Fedora, but we're not sure why it is so crucial. Knowing what we need
>>> to address before disabling/removing it would help a lot.
>> IIRC, they considered the selinux=0 kernel boot parameter to be
>> inadequate because of the difficulty in changing kernel boot parameters
>> on certain platforms (IBM?). The no-policy-loaded alternative still
>> left a lot of SELinux processing in place, so users would still end up
>> paying memory and runtime overheads for no benefit if we only skipped
>> policy load.
> Thanks, I was worried that there was also something more tricky than
> this. We could make adding-removing the kernel parameter easier on
> Fedora by creating and maintaining a tool that would be able to do it
> reliably across the supported arches. That shouldn't be too hard,
> hopefully.
>
>>>> selinux_state.initialized reflects whether a policy has
>>>> been loaded. With a few exceptions in certain hook functions, it is
>>>> only checked by the security server service functions
>>>> (security/selinux/ss/services.c) prior to accessing the policydb. So
>>>> there is a lot of SELinux processing that would still occur in that
>>>> situation unless we added if (!selinux_state.initialized) return 0;
>>>> checks to all the hook functions, which would create the same exposure
>>>> and would further break the SELinux-enabled case (we need to perform
>>>> some SELinux processing pre-policy-load to allocate blobs and track what
>>>> tasks and objects require delayed security initialization when policy
>>>> load finally occurs).
>>> I think what Casey was suggesting is to add another flag that would
>>> switch from "no policy loaded, but we expect it to be loaded
>>> eventually" to "no policy loaded and we don't expect/allow it to be
>>> loaded any more", which is essentially equivalent to checking
>>> selinux_enabled in each hook, which you had already brought up.
>> Yep. if (!selinux_enabled) return 0; or if (selinux_state.disabled)
>> return 0; under #ifdef CONFIG_SECURITY_SELINUX_DISABLE in every hook
>> might be the best option until it can be removed altogether; avoids
>> impacting the LSM framework or any other security module, preserves the
>> existing functionality, fairly low overhead on the SELinux-disabled case.
> OK, so I'll put together another patch that removes all the
> security_delete_hooks() stuff and adds the checks.
I endorse this approach enthusiastically.
>
>> NB selinux_enabled was originally just for selinux=0 handling and thus
>> remains global (not per selinux-namespace). selinux_state.disabled is
>> only for runtime disable via selinuxfs, which could be applied per
>> selinux-namespace if/when selinux namespaces are ever completed and
>> merged. Aside from clearing selinux_enabled in selinux_disable() and
>> logging selinux_enabled in sel_write_enforce() - which seems pointless
>> by the way, there are no other uses of selinux_enabled outside of __init
>> code AFAICS. I think at one time selinux_enabled was exported for use
>> by other kernel code related to SECMARK or elsewhere but that was
>> eliminated/generalized for other security modules. So it seems like we
>> could always make selinux_enabled itself ro_after_init, stop clearing it
>> in selinux_disable() since nothing will be testing it, and just use
>> selinux_state.disabled in the hooks (and possibly in the
>> sel_write_enforce audit log).
> Yes, that sounds reasonable.
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox