* 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
* Re: [PATCH] LSM: allow an LSM to disable all hooks at once
From: Ondrej Mosnacek @ 2019-12-12 16:03 UTC (permalink / raw)
To: Stephen Smalley
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: <1f613260-d315-6925-d069-e92b872b8610@tycho.nsa.gov>
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.
--
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.
^ permalink raw reply
* Re: [PATCH 1/2] efi: add a function for transferring status to string
From: Joey Lee @ 2019-12-12 14:24 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+Gu83Ndu8XWDAUTmHu6udRCXbodqzTyq5wZJvfGiLfidwbw@mail.gmail.com>
Hi Ard,
On Thu, Dec 12, 2019 at 11:20:48AM +0000, Ard Biesheuvel wrote:
> On Thu, 12 Dec 2019 at 10:38, Lee, Chun-Yi <joeyli.kernel@gmail.com> wrote:
> >
> > This function can be used to transfer EFI status code to string
> > to improve the readability of debug log.
> >
> > Signed-off-by: "Lee, Chun-Yi" <jlee@suse.com>
>
> I think I mentioned this the last time you sent this patch: by making
> this a static inline, those strings will be copied into each object
> file that uses this routine.
> Instead, please make it an ordinary function.
>
Sorry for I just sent a old version patch. I will send a new one.
Thanks a lot!
Joey Lee
> > ---
> > include/linux/efi.h | 26 ++++++++++++++++++++++++++
> > 1 file changed, 26 insertions(+)
> >
> > diff --git a/include/linux/efi.h b/include/linux/efi.h
> > index d87acf62958e..08daf4cdd807 100644
> > --- a/include/linux/efi.h
> > +++ b/include/linux/efi.h
> > @@ -42,6 +42,32 @@
> > #define EFI_ABORTED (21 | (1UL << (BITS_PER_LONG-1)))
> > #define EFI_SECURITY_VIOLATION (26 | (1UL << (BITS_PER_LONG-1)))
> >
> > +#define EFI_STATUS_STR(_status) \
> > + case EFI_##_status: \
> > + return "EFI_" __stringify(_status);
> > +
> > +static inline char *
> > +efi_status_to_str(unsigned long status)
> > +{
> > + switch (status) {
> > + EFI_STATUS_STR(SUCCESS)
> > + EFI_STATUS_STR(LOAD_ERROR)
> > + EFI_STATUS_STR(INVALID_PARAMETER)
> > + EFI_STATUS_STR(UNSUPPORTED)
> > + EFI_STATUS_STR(BAD_BUFFER_SIZE)
> > + EFI_STATUS_STR(BUFFER_TOO_SMALL)
> > + EFI_STATUS_STR(NOT_READY)
> > + EFI_STATUS_STR(DEVICE_ERROR)
> > + EFI_STATUS_STR(WRITE_PROTECTED)
> > + EFI_STATUS_STR(OUT_OF_RESOURCES)
> > + EFI_STATUS_STR(NOT_FOUND)
> > + EFI_STATUS_STR(ABORTED)
> > + EFI_STATUS_STR(SECURITY_VIOLATION)
> > + }
> > +
> > + return "";
> > +}
> > +
> > typedef unsigned long efi_status_t;
> > typedef u8 efi_bool_t;
> > typedef u16 efi_char16_t; /* UNICODE character */
> > --
> > 2.16.4
> >
^ permalink raw reply
* Re: [PATCH v1 0/3] Introduce CAP_SYS_PERFMON capability for secure Perf users groups
From: Stephen Smalley @ 2019-12-12 14:24 UTC (permalink / raw)
To: Andi Kleen, Casey Schaufler
Cc: Alexey Budankov, Peter Zijlstra, Arnaldo Carvalho de Melo,
Ingo Molnar, Jiri Olsa, elena.reshetova, Alexander Shishkin,
Jann Horn, Kees Cook, Stephane Eranian, Namhyung Kim,
linux-security-module, selinux, linux-kernel, Serge E. Hallyn,
James Morris
In-Reply-To: <20191211203648.GA862919@tassilo.jf.intel.com>
On 12/11/19 3:36 PM, Andi Kleen wrote:
>>> In this circumstances CAP_SYS_PERFMON looks like smart balanced advancement that
>>> trade-offs between perf_events subsystem extensions, required level of control
>>> and configurability of perf_events, existing users adoption effort, and it brings
>>> security hardening benefits of decreasing attack surface for the existing users
>>> and use cases.
>>
>> I'm not 100% opposed to CAP_SYS_PERFMON. I am 100% opposed to new capabilities
>> that have a single use. Surely there are other CAP_SYS_ADMIN users that [cs]ould
>> be converted to CAP_SYS_PERFMON as well. If there is a class of system performance
>> privileged operations, say a dozen or so, you may have a viable argument.
>
> perf events is not a single use. It has a bazillion of sub functionalities,
> including hardware tracing, software tracing, pmu counters, software counters,
> uncore counters, break points and various other stuff in its PMU drivers.
>
> See it more as a whole quite heterogenous driver subsystem.
>
> I guess CAP_SYS_PERFMON is not a good name because perf is much more
> than just Perfmon. Perhaps call it CAP_SYS_PERF_EVENTS
That seems misleading since it isn't being checked for all perf_events
operations IIUC (CAP_SYS_ADMIN is still required for some?) and it is
even more specialized than CAP_SYS_PERFMON, making it less likely that
we could ever use this capability as a check for other kernel
performance monitoring facilities beyond perf_events.
I'm not as opposed to fine-grained capabilities as Casey is but I do
recognize that there are a limited number of available bits (although we
do have a fair number of unused ones currently given the extension to
64-bits) and that it would be easy to consume them all if we allocated
one for every kernel feature. That said, this might be a sufficiently
important use case to justify it.
Obviously I'd encourage you to consider leveraging SELinux as well but I
understand that you are looking for a solution that doesn't depend on a
distro using a particular LSM or a particular policy. I will note that
SELinux doesn't suffer from the limited bits problem because one can
always define a new SELinux security class with its own access vector
permissions bitmap, as has been done for the recently added LSM/SELinux
perf_event hooks.
I don't know who actually gets to decide when/if a new capability is
allocated. Maybe Serge and/or James as capabilities and LSM maintainers.
I have no objections to these patches from a SELinux POV.
^ permalink raw reply
* Re: [PATCH] LSM: allow an LSM to disable all hooks at once
From: Stephen Smalley @ 2019-12-12 13:14 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: <CAFqZXNtjELoG_5GK5c4XpH8Be3NfsKMZdZvrJKPpnTLPKKgZ9A@mail.gmail.com>
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.
>> 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.
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).
^ permalink raw reply
* Re: [PATCH] LSM: allow an LSM to disable all hooks at once
From: Ondrej Mosnacek @ 2019-12-12 11:58 UTC (permalink / raw)
To: Tetsuo Handa
Cc: Linux Security Module list, James Morris, Serge E. Hallyn,
Casey Schaufler, SElinux list, Paul Moore, Stephen Smalley,
John Johansen, Kees Cook, Micah Morton
In-Reply-To: <66706222-b6af-5099-e485-b99391ad70b3@i-love.sakura.ne.jp>
On Thu, Dec 12, 2019 at 11:31 AM Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
> On 2019/12/11 23:08, Ondrej Mosnacek wrote:
> > 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.
>
> I don't like unconditionally marking __ro_after_init. I'm currently waiting for
> Casey's stacking work to complete. I haven't given up dynamically loadable LSM modules.
>
> In order to allow loading LSM modules after boot, I have to add lines
> 1093-1173, 1190-1195, 1208-1211, 1217-1220 in
> https://osdn.net/projects/akari/scm/svn/blobs/head/trunk/akari/lsm-4.12.c .
> I suggest grouping __lsm_ro_after_init variables into a special section and
> implementing a legal method for temporarily making that section read-write.
> Then, architectures with that method will be able to use __ro_after_init marking.
I'd say the burden of implementing this would lie on the arms of
whoever prepares the patches for dynamic load/unload. However, *if*
this patch is going to go anywhere, I could at least keep
__lsm_ro_after_init (now as just an alias for __ro_after_init) so its
definition can be easily changed later.
--
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-12 11:49 UTC (permalink / raw)
To: Stephen Smalley
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: <2fdb09e7-6668-cb1b-8a2d-1550278ae803@tycho.nsa.gov>
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.
> 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.
--
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-12 11:48 UTC (permalink / raw)
To: Stephen Smalley
Cc: Linux Security Module list, James Morris, Serge E. Hallyn,
Casey Schaufler, SElinux list, Paul Moore, John Johansen,
Kees Cook, Micah Morton, Tetsuo Handa
In-Reply-To: <677be2d4-8247-3c2b-ac13-def725cffaeb@tycho.nsa.gov>
On Wed, Dec 11, 2019 at 3:29 PM Stephen Smalley <sds@tycho.nsa.gov> 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?
It saves us from maintaining the invariant that each hook has to begin
with said condition and it avoids one extra indirect jump. Whether
that's a compelling advantage, I don't know...
> 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.
I don't think that makes the situation too much worse, but others may
disagree...
--
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.
^ permalink raw reply
* Re: [PATCH 1/2] efi: add a function for transferring status to string
From: Ard Biesheuvel @ 2019-12-12 11:20 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: <20191212093812.10518-2-jlee@suse.com>
On Thu, 12 Dec 2019 at 10:38, Lee, Chun-Yi <joeyli.kernel@gmail.com> wrote:
>
> This function can be used to transfer EFI status code to string
> to improve the readability of debug log.
>
> Signed-off-by: "Lee, Chun-Yi" <jlee@suse.com>
I think I mentioned this the last time you sent this patch: by making
this a static inline, those strings will be copied into each object
file that uses this routine.
Instead, please make it an ordinary function.
> ---
> include/linux/efi.h | 26 ++++++++++++++++++++++++++
> 1 file changed, 26 insertions(+)
>
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index d87acf62958e..08daf4cdd807 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -42,6 +42,32 @@
> #define EFI_ABORTED (21 | (1UL << (BITS_PER_LONG-1)))
> #define EFI_SECURITY_VIOLATION (26 | (1UL << (BITS_PER_LONG-1)))
>
> +#define EFI_STATUS_STR(_status) \
> + case EFI_##_status: \
> + return "EFI_" __stringify(_status);
> +
> +static inline char *
> +efi_status_to_str(unsigned long status)
> +{
> + switch (status) {
> + EFI_STATUS_STR(SUCCESS)
> + EFI_STATUS_STR(LOAD_ERROR)
> + EFI_STATUS_STR(INVALID_PARAMETER)
> + EFI_STATUS_STR(UNSUPPORTED)
> + EFI_STATUS_STR(BAD_BUFFER_SIZE)
> + EFI_STATUS_STR(BUFFER_TOO_SMALL)
> + EFI_STATUS_STR(NOT_READY)
> + EFI_STATUS_STR(DEVICE_ERROR)
> + EFI_STATUS_STR(WRITE_PROTECTED)
> + EFI_STATUS_STR(OUT_OF_RESOURCES)
> + EFI_STATUS_STR(NOT_FOUND)
> + EFI_STATUS_STR(ABORTED)
> + EFI_STATUS_STR(SECURITY_VIOLATION)
> + }
> +
> + return "";
> +}
> +
> typedef unsigned long efi_status_t;
> typedef u8 efi_bool_t;
> typedef u16 efi_char16_t; /* UNICODE character */
> --
> 2.16.4
>
^ permalink raw reply
* Re: [PATCH] LSM: allow an LSM to disable all hooks at once
From: Tetsuo Handa @ 2019-12-12 10:30 UTC (permalink / raw)
To: Ondrej Mosnacek, linux-security-module, James Morris,
Serge E. Hallyn
Cc: Casey Schaufler, selinux, Paul Moore, Stephen Smalley,
John Johansen, Kees Cook, Micah Morton
In-Reply-To: <20191211140833.939845-1-omosnace@redhat.com>
On 2019/12/11 23:08, Ondrej Mosnacek wrote:
> 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.
I don't like unconditionally marking __ro_after_init. I'm currently waiting for
Casey's stacking work to complete. I haven't given up dynamically loadable LSM modules.
In order to allow loading LSM modules after boot, I have to add lines
1093-1173, 1190-1195, 1208-1211, 1217-1220 in
https://osdn.net/projects/akari/scm/svn/blobs/head/trunk/akari/lsm-4.12.c .
I suggest grouping __lsm_ro_after_init variables into a special section and
implementing a legal method for temporarily making that section read-write.
Then, architectures with that method will be able to use __ro_after_init marking.
^ permalink raw reply
* [PATCH 2/2] efi: show error messages only when loading certificates is failed
From: Lee, Chun-Yi @ 2019-12-12 9:38 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: <20191212093812.10518-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 | 41 ++++++++++++++-------------
1 file changed, 22 insertions(+), 19 deletions(-)
diff --git a/security/integrity/platform_certs/load_uefi.c b/security/integrity/platform_certs/load_uefi.c
index 81b19c52832b..3b766831d2c5 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,31 @@ 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 (0x%lx)\n",
+ source, efi_status_to_str(status), status);
+ return NULL;
}
/*
@@ -153,10 +162,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 +173,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 +182,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] efi: add a function for transferring status to string
From: Lee, Chun-Yi @ 2019-12-12 9:38 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: <20191212093812.10518-1-jlee@suse.com>
This function can be used to transfer EFI status code to string
to improve the readability of debug log.
Signed-off-by: "Lee, Chun-Yi" <jlee@suse.com>
---
include/linux/efi.h | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)
diff --git a/include/linux/efi.h b/include/linux/efi.h
index d87acf62958e..08daf4cdd807 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -42,6 +42,32 @@
#define EFI_ABORTED (21 | (1UL << (BITS_PER_LONG-1)))
#define EFI_SECURITY_VIOLATION (26 | (1UL << (BITS_PER_LONG-1)))
+#define EFI_STATUS_STR(_status) \
+ case EFI_##_status: \
+ return "EFI_" __stringify(_status);
+
+static inline char *
+efi_status_to_str(unsigned long status)
+{
+ switch (status) {
+ EFI_STATUS_STR(SUCCESS)
+ EFI_STATUS_STR(LOAD_ERROR)
+ EFI_STATUS_STR(INVALID_PARAMETER)
+ EFI_STATUS_STR(UNSUPPORTED)
+ EFI_STATUS_STR(BAD_BUFFER_SIZE)
+ EFI_STATUS_STR(BUFFER_TOO_SMALL)
+ EFI_STATUS_STR(NOT_READY)
+ EFI_STATUS_STR(DEVICE_ERROR)
+ EFI_STATUS_STR(WRITE_PROTECTED)
+ EFI_STATUS_STR(OUT_OF_RESOURCES)
+ EFI_STATUS_STR(NOT_FOUND)
+ EFI_STATUS_STR(ABORTED)
+ EFI_STATUS_STR(SECURITY_VIOLATION)
+ }
+
+ return "";
+}
+
typedef unsigned long efi_status_t;
typedef u8 efi_bool_t;
typedef u16 efi_char16_t; /* UNICODE character */
--
2.16.4
^ permalink raw reply related
* [PATCH 0/2] efi: cosmetic patches for the error messages when loading certificates
From: Lee, Chun-Yi @ 2019-12-12 9:38 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
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 set moved the above messages to the error
handling code path. And, it adds a function to transfer EFI status
code to string to improve the readability of debug log. The function
can also be used in other EFI log.
Lee, Chun-Yi (2):
efi: add a function for transferring status to string
efi: show error messages only when loading certificates is failed
include/linux/efi.h | 26 +++++++++++++++++
security/integrity/platform_certs/load_uefi.c | 41 ++++++++++++++-------------
2 files changed, 48 insertions(+), 19 deletions(-)
--
2.16.4
^ permalink raw reply
* Re: [PATCH v1 0/3] Introduce CAP_SYS_PERFMON capability for secure Perf users groups
From: Casey Schaufler @ 2019-12-11 21:25 UTC (permalink / raw)
To: Andi Kleen
Cc: Alexey Budankov, Peter Zijlstra, Arnaldo Carvalho de Melo,
Ingo Molnar, Jiri Olsa, elena.reshetova, Alexander Shishkin,
Jann Horn, Kees Cook, Stephane Eranian, Namhyung Kim,
linux-security-module, selinux, linux-kernel, Casey Schaufler
In-Reply-To: <20191211203648.GA862919@tassilo.jf.intel.com>
On 12/11/2019 12:36 PM, Andi Kleen wrote:
>>> In this circumstances CAP_SYS_PERFMON looks like smart balanced advancement that
>>> trade-offs between perf_events subsystem extensions, required level of control
>>> and configurability of perf_events, existing users adoption effort, and it brings
>>> security hardening benefits of decreasing attack surface for the existing users
>>> and use cases.
>> I'm not 100% opposed to CAP_SYS_PERFMON. I am 100% opposed to new capabilities
>> that have a single use. Surely there are other CAP_SYS_ADMIN users that [cs]ould
>> be converted to CAP_SYS_PERFMON as well. If there is a class of system performance
>> privileged operations, say a dozen or so, you may have a viable argument.
> perf events is not a single use.
If it is only being called in two places, it is single use.
> It has a bazillion of sub functionalities,
> including hardware tracing, software tracing, pmu counters, software counters,
> uncore counters, break points and various other stuff in its PMU drivers.
>
> See it more as a whole quite heterogenous driver subsystem.
>
> I guess CAP_SYS_PERFMON is not a good name because perf is much more
> than just Perfmon. Perhaps call it CAP_SYS_PERF_EVENTS
>
> -Andi
^ permalink raw reply
* Re: [PATCH v1 0/3] Introduce CAP_SYS_PERFMON capability for secure Perf users groups
From: Andi Kleen @ 2019-12-11 20:36 UTC (permalink / raw)
To: Casey Schaufler
Cc: Alexey Budankov, Peter Zijlstra, Arnaldo Carvalho de Melo,
Ingo Molnar, Jiri Olsa, elena.reshetova, Alexander Shishkin,
Jann Horn, Kees Cook, Stephane Eranian, Namhyung Kim,
linux-security-module, selinux, linux-kernel
In-Reply-To: <16e9e399-2ebf-261f-eee5-cf9ace2a82b9@schaufler-ca.com>
> > In this circumstances CAP_SYS_PERFMON looks like smart balanced advancement that
> > trade-offs between perf_events subsystem extensions, required level of control
> > and configurability of perf_events, existing users adoption effort, and it brings
> > security hardening benefits of decreasing attack surface for the existing users
> > and use cases.
>
> I'm not 100% opposed to CAP_SYS_PERFMON. I am 100% opposed to new capabilities
> that have a single use. Surely there are other CAP_SYS_ADMIN users that [cs]ould
> be converted to CAP_SYS_PERFMON as well. If there is a class of system performance
> privileged operations, say a dozen or so, you may have a viable argument.
perf events is not a single use. It has a bazillion of sub functionalities,
including hardware tracing, software tracing, pmu counters, software counters,
uncore counters, break points and various other stuff in its PMU drivers.
See it more as a whole quite heterogenous driver subsystem.
I guess CAP_SYS_PERFMON is not a good name because perf is much more
than just Perfmon. Perhaps call it CAP_SYS_PERF_EVENTS
-Andi
^ permalink raw reply
* Re: [PATCH] LSM: allow an LSM to disable all hooks at once
From: Stephen Smalley @ 2019-12-11 19:12 UTC (permalink / raw)
To: Casey Schaufler, Kees Cook
Cc: Ondrej Mosnacek, linux-security-module, James Morris,
Serge E. Hallyn, selinux, Paul Moore, John Johansen, Micah Morton,
Tetsuo Handa
In-Reply-To: <356c555a-d4ab-84fb-0165-f7672bc1ee63@schaufler-ca.com>
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. 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).
>
>
>> -Kees
>>
>>>> Fixes: b1d9e6b0646d ("LSM: Switch to lists of hooks")
>>>> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
>>>> ---
>>>>
>>>> This is an alternative to [1]. It turned out to be less simple than I
>>>> had hoped, but OTOH the hook arrays can now be unconditionally made
>>>> __ro_after_init so may be still worth it.
>>>>
>>>> Compile- and boot- tested with SECURITY_SELINUX_DISABLE set to =y and
>>>> =n; SELinux enabled. Changes to other LSMs only compile-tested (but
>>>> those are trivial).
>>>>
>>>> [1] https://lore.kernel.org/selinux/20191209075756.123157-1-omosnace@redhat.com/T/
>>>>
>>>> include/linux/lsm_hooks.h | 46 +++++++++----------------------
>>>> security/Kconfig | 5 ----
>>>> security/apparmor/lsm.c | 14 ++++++----
>>>> security/commoncap.c | 11 +++++---
>>>> security/loadpin/loadpin.c | 10 +++++--
>>>> security/lockdown/lockdown.c | 11 +++++---
>>>> security/safesetid/lsm.c | 9 +++++--
>>>> security/security.c | 52 +++++++++++++++++++++---------------
>>>> security/selinux/Kconfig | 5 ++--
>>>> security/selinux/hooks.c | 28 ++++++++++++++-----
>>>> security/smack/smack_lsm.c | 11 +++++---
>>>> security/tomoyo/tomoyo.c | 13 ++++++---
>>>> security/yama/yama_lsm.c | 10 +++++--
>>>> 13 files changed, 133 insertions(+), 92 deletions(-)
>>>>
>>>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
>>>> index 20d8cf194fb7..91b77ebcb822 100644
>>>> --- a/include/linux/lsm_hooks.h
>>>> +++ b/include/linux/lsm_hooks.h
>>>> @@ -27,7 +27,6 @@
>>>> #include <linux/security.h>
>>>> #include <linux/init.h>
>>>> -#include <linux/rculist.h>
>>>> /**
>>>> * union security_list_options - Linux Security Module hook function list
>>>> @@ -2086,6 +2085,9 @@ struct security_hook_list {
>>>> struct hlist_head *head;
>>>> union security_list_options hook;
>>>> char *lsm;
>>>> +#ifdef CONFIG_SECURITY_SELINUX_DISABLE
>>>> + const int *enabled;
>>>> +#endif
>>>> } __randomize_layout;
>>>> /*
>>>> @@ -2112,8 +2114,16 @@ struct lsm_blob_sizes {
>>>> extern struct security_hook_heads security_hook_heads;
>>>> extern char *lsm_names;
>>>> -extern void security_add_hooks(struct security_hook_list *hooks, int count,
>>>> - char *lsm);
>>>> +struct security_hook_array {
>>>> + struct security_hook_list *hooks;
>>>> + int count;
>>>> + char *lsm;
>>>> +#ifdef CONFIG_SECURITY_SELINUX_DISABLE
>>>> + const int *enabled;
>>>> +#endif
>>>> +};
>>>> +
>>>> +extern void security_add_hook_array(const struct security_hook_array *array);
>>>> #define LSM_FLAG_LEGACY_MAJOR BIT(0)
>>>> #define LSM_FLAG_EXCLUSIVE BIT(1)
>>>> @@ -2145,36 +2155,6 @@ extern struct lsm_info __start_early_lsm_info[], __end_early_lsm_info[];
>>>> __used __section(.early_lsm_info.init) \
>>>> __aligned(sizeof(unsigned long))
>>>> -#ifdef CONFIG_SECURITY_SELINUX_DISABLE
>>>> -/*
>>>> - * Assuring the safety of deleting a security module is up to
>>>> - * the security module involved. This may entail ordering the
>>>> - * module's hook list in a particular way, refusing to disable
>>>> - * the module once a policy is loaded or any number of other
>>>> - * actions better imagined than described.
>>>> - *
>>>> - * The name of the configuration option reflects the only module
>>>> - * that currently uses the mechanism. Any developer who thinks
>>>> - * disabling their module is a good idea needs to be at least as
>>>> - * careful as the SELinux team.
>>>> - */
>>>> -static inline void security_delete_hooks(struct security_hook_list *hooks,
>>>> - int count)
>>>> -{
>>>> - int i;
>>>> -
>>>> - for (i = 0; i < count; i++)
>>>> - hlist_del_rcu(&hooks[i].list);
>>>> -}
>>>> -#endif /* CONFIG_SECURITY_SELINUX_DISABLE */
>>>> -
>>>> -/* Currently required to handle SELinux runtime hook disable. */
>>>> -#ifdef CONFIG_SECURITY_WRITABLE_HOOKS
>>>> -#define __lsm_ro_after_init
>>>> -#else
>>>> -#define __lsm_ro_after_init __ro_after_init
>>>> -#endif /* CONFIG_SECURITY_WRITABLE_HOOKS */
>>>> -
>>>> extern int lsm_inode_alloc(struct inode *inode);
>>>> #endif /* ! __LINUX_LSM_HOOKS_H */
>>>> diff --git a/security/Kconfig b/security/Kconfig
>>>> index 2a1a2d396228..456764990a13 100644
>>>> --- a/security/Kconfig
>>>> +++ b/security/Kconfig
>>>> @@ -32,11 +32,6 @@ config SECURITY
>>>> If you are unsure how to answer this question, answer N.
>>>> -config SECURITY_WRITABLE_HOOKS
>>>> - depends on SECURITY
>>>> - bool
>>>> - default n
>>>> -
>>>> config SECURITYFS
>>>> bool "Enable the securityfs filesystem"
>>>> help
>>>> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
>>>> index b621ad74f54a..a27f48670b37 100644
>>>> --- a/security/apparmor/lsm.c
>>>> +++ b/security/apparmor/lsm.c
>>>> @@ -1158,13 +1158,13 @@ static int apparmor_inet_conn_request(struct sock *sk, struct sk_buff *skb,
>>>> /*
>>>> * The cred blob is a pointer to, not an instance of, an aa_task_ctx.
>>>> */
>>>> -struct lsm_blob_sizes apparmor_blob_sizes __lsm_ro_after_init = {
>>>> +struct lsm_blob_sizes apparmor_blob_sizes __ro_after_init = {
>>>> .lbs_cred = sizeof(struct aa_task_ctx *),
>>>> .lbs_file = sizeof(struct aa_file_ctx),
>>>> .lbs_task = sizeof(struct aa_task_ctx),
>>>> };
>>>> -static struct security_hook_list apparmor_hooks[] __lsm_ro_after_init = {
>>>> +static struct security_hook_list apparmor_hooks[] __ro_after_init = {
>>>> LSM_HOOK_INIT(ptrace_access_check, apparmor_ptrace_access_check),
>>>> LSM_HOOK_INIT(ptrace_traceme, apparmor_ptrace_traceme),
>>>> LSM_HOOK_INIT(capget, apparmor_capget),
>>>> @@ -1368,7 +1368,7 @@ static const struct kernel_param_ops param_ops_aaintbool = {
>>>> .get = param_get_aaintbool
>>>> };
>>>> /* Boot time disable flag */
>>>> -static int apparmor_enabled __lsm_ro_after_init = 1;
>>>> +static int apparmor_enabled __ro_after_init = 1;
>>>> module_param_named(enabled, apparmor_enabled, aaintbool, 0444);
>>>> static int __init apparmor_enabled_setup(char *str)
>>>> @@ -1829,6 +1829,11 @@ __initcall(apparmor_nf_ip_init);
>>>> static int __init apparmor_init(void)
>>>> {
>>>> + struct security_hook_array hook_array = {
>>>> + .hooks = apparmor_hooks,
>>>> + .count = ARRAY_SIZE(apparmor_hooks),
>>>> + .lsm = "apparmor",
>>>> + };
>>>> int error;
>>>> aa_secids_init();
>>>> @@ -1864,8 +1869,7 @@ static int __init apparmor_init(void)
>>>> aa_free_root_ns();
>>>> goto buffers_out;
>>>> }
>>>> - security_add_hooks(apparmor_hooks, ARRAY_SIZE(apparmor_hooks),
>>>> - "apparmor");
>>>> + security_add_hook_array(&hook_array);
>>>> /* Report that AppArmor successfully initialized */
>>>> apparmor_initialized = 1;
>>>> diff --git a/security/commoncap.c b/security/commoncap.c
>>>> index f4ee0ae106b2..6e9f4b6b6b1d 100644
>>>> --- a/security/commoncap.c
>>>> +++ b/security/commoncap.c
>>>> @@ -1339,7 +1339,7 @@ int cap_mmap_file(struct file *file, unsigned long reqprot,
>>>> #ifdef CONFIG_SECURITY
>>>> -static struct security_hook_list capability_hooks[] __lsm_ro_after_init = {
>>>> +static struct security_hook_list capability_hooks[] __ro_after_init = {
>>>> LSM_HOOK_INIT(capable, cap_capable),
>>>> LSM_HOOK_INIT(settime, cap_settime),
>>>> LSM_HOOK_INIT(ptrace_access_check, cap_ptrace_access_check),
>>>> @@ -1362,8 +1362,13 @@ static struct security_hook_list capability_hooks[] __lsm_ro_after_init = {
>>>> static int __init capability_init(void)
>>>> {
>>>> - security_add_hooks(capability_hooks, ARRAY_SIZE(capability_hooks),
>>>> - "capability");
>>>> + struct security_hook_array hook_array = {
>>>> + .hooks = capability_hooks,
>>>> + .count = ARRAY_SIZE(capability_hooks),
>>>> + .lsm = "capability",
>>>> + };
>>>> +
>>>> + security_add_hook_array(&hook_array);
>>>> return 0;
>>>> }
>>>> diff --git a/security/loadpin/loadpin.c b/security/loadpin/loadpin.c
>>>> index ee5cb944f4ad..5fadd4969d90 100644
>>>> --- a/security/loadpin/loadpin.c
>>>> +++ b/security/loadpin/loadpin.c
>>>> @@ -180,7 +180,7 @@ static int loadpin_load_data(enum kernel_load_data_id id)
>>>> return loadpin_read_file(NULL, (enum kernel_read_file_id) id);
>>>> }
>>>> -static struct security_hook_list loadpin_hooks[] __lsm_ro_after_init = {
>>>> +static struct security_hook_list loadpin_hooks[] __ro_after_init = {
>>>> LSM_HOOK_INIT(sb_free_security, loadpin_sb_free_security),
>>>> LSM_HOOK_INIT(kernel_read_file, loadpin_read_file),
>>>> LSM_HOOK_INIT(kernel_load_data, loadpin_load_data),
>>>> @@ -224,10 +224,16 @@ static void __init parse_exclude(void)
>>>> static int __init loadpin_init(void)
>>>> {
>>>> + struct security_hook_array hook_array = {
>>>> + .hooks = loadpin_hooks,
>>>> + .count = ARRAY_SIZE(loadpin_hooks),
>>>> + .lsm = "loadpin",
>>>> + };
>>>> +
>>>> pr_info("ready to pin (currently %senforcing)\n",
>>>> enforce ? "" : "not ");
>>>> parse_exclude();
>>>> - security_add_hooks(loadpin_hooks, ARRAY_SIZE(loadpin_hooks), "loadpin");
>>>> + security_add_hook_array(&hook_array);
>>>> return 0;
>>>> }
>>>> diff --git a/security/lockdown/lockdown.c b/security/lockdown/lockdown.c
>>>> index 5a952617a0eb..bcfa0ff4ee63 100644
>>>> --- a/security/lockdown/lockdown.c
>>>> +++ b/security/lockdown/lockdown.c
>>>> @@ -71,19 +71,24 @@ static int lockdown_is_locked_down(enum lockdown_reason what)
>>>> return 0;
>>>> }
>>>> -static struct security_hook_list lockdown_hooks[] __lsm_ro_after_init = {
>>>> +static struct security_hook_list lockdown_hooks[] __ro_after_init = {
>>>> LSM_HOOK_INIT(locked_down, lockdown_is_locked_down),
>>>> };
>>>> static int __init lockdown_lsm_init(void)
>>>> {
>>>> + struct security_hook_array hook_array = {
>>>> + .hooks = lockdown_hooks,
>>>> + .count = ARRAY_SIZE(lockdown_hooks),
>>>> + .lsm = "lockdown",
>>>> + };
>>>> +
>>>> #if defined(CONFIG_LOCK_DOWN_KERNEL_FORCE_INTEGRITY)
>>>> lock_kernel_down("Kernel configuration", LOCKDOWN_INTEGRITY_MAX);
>>>> #elif defined(CONFIG_LOCK_DOWN_KERNEL_FORCE_CONFIDENTIALITY)
>>>> lock_kernel_down("Kernel configuration", LOCKDOWN_CONFIDENTIALITY_MAX);
>>>> #endif
>>>> - security_add_hooks(lockdown_hooks, ARRAY_SIZE(lockdown_hooks),
>>>> - "lockdown");
>>>> + security_add_hook_array(&hook_array);
>>>> return 0;
>>>> }
>>>> diff --git a/security/safesetid/lsm.c b/security/safesetid/lsm.c
>>>> index 7760019ad35d..4e36e53f033d 100644
>>>> --- a/security/safesetid/lsm.c
>>>> +++ b/security/safesetid/lsm.c
>>>> @@ -156,8 +156,13 @@ static struct security_hook_list safesetid_security_hooks[] = {
>>>> static int __init safesetid_security_init(void)
>>>> {
>>>> - security_add_hooks(safesetid_security_hooks,
>>>> - ARRAY_SIZE(safesetid_security_hooks), "safesetid");
>>>> + struct security_hook_array hook_array = {
>>>> + .hooks = safesetid_security_hooks,
>>>> + .count = ARRAY_SIZE(safesetid_security_hooks),
>>>> + .lsm = "safesetid",
>>>> + };
>>>> +
>>>> + security_add_hook_array(&hook_array);
>>>> /* Report that SafeSetID successfully initialized */
>>>> safesetid_initialized = 1;
>>>> diff --git a/security/security.c b/security/security.c
>>>> index 2b5473d92416..a5dd348bd37e 100644
>>>> --- a/security/security.c
>>>> +++ b/security/security.c
>>>> @@ -68,14 +68,14 @@ const char *const lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1] = {
>>>> [LOCKDOWN_CONFIDENTIALITY_MAX] = "confidentiality",
>>>> };
>>>> -struct security_hook_heads security_hook_heads __lsm_ro_after_init;
>>>> +struct security_hook_heads security_hook_heads __ro_after_init;
>>>> static BLOCKING_NOTIFIER_HEAD(blocking_lsm_notifier_chain);
>>>> static struct kmem_cache *lsm_file_cache;
>>>> static struct kmem_cache *lsm_inode_cache;
>>>> char *lsm_names;
>>>> -static struct lsm_blob_sizes blob_sizes __lsm_ro_after_init;
>>>> +static struct lsm_blob_sizes blob_sizes __ro_after_init;
>>>> /* Boot-time LSM user choice */
>>>> static __initdata const char *chosen_lsm_order;
>>>> @@ -467,20 +467,20 @@ static int lsm_append(const char *new, char **result)
>>>> /**
>>>> * security_add_hooks - Add a modules hooks to the hook lists.
>>>> - * @hooks: the hooks to add
>>>> - * @count: the number of hooks to add
>>>> - * @lsm: the name of the security module
>>>> + * @array: the struct describing hooks to add
>>>> *
>>>> * Each LSM has to register its hooks with the infrastructure.
>>>> */
>>>> -void __init security_add_hooks(struct security_hook_list *hooks, int count,
>>>> - char *lsm)
>>>> +void __init security_add_hook_array(const struct security_hook_array *array)
>>>> {
>>>> int i;
>>>> - for (i = 0; i < count; i++) {
>>>> - hooks[i].lsm = lsm;
>>>> - hlist_add_tail_rcu(&hooks[i].list, hooks[i].head);
>>>> + for (i = 0; i < array->count; i++) {
>>>> + array->hooks[i].lsm = array->lsm;
>>>> +#ifdef CONFIG_SECURITY_SELINUX_DISABLE
>>>> + array->hooks[i].enabled = array->enabled;
>>>> +#endif
>>>> + hlist_add_tail_rcu(&array->hooks[i].list, array->hooks[i].head);
>>>> }
>>>> /*
>>>> @@ -488,7 +488,7 @@ void __init security_add_hooks(struct security_hook_list *hooks, int count,
>>>> * and fix this up afterwards.
>>>> */
>>>> if (slab_is_available()) {
>>>> - if (lsm_append(lsm, &lsm_names) < 0)
>>>> + if (lsm_append(array->lsm, &lsm_names) < 0)
>>>> panic("%s - Cannot get early memory.\n", __func__);
>>>> }
>>>> }
>>>> @@ -679,11 +679,22 @@ static void __init lsm_early_task(struct task_struct *task)
>>>> * This is a hook that returns a value.
>>>> */
>>>> +#define for_each_hook(p, func) \
>>>> + hlist_for_each_entry(p, &security_hook_heads.func, list)
>>>> +
>>>> +#ifdef CONFIG_SECURITY_SELINUX_DISABLE
>>>> +#define for_each_enabled_hook(p, func) \
>>>> + for_each_hook(p, func) \
>>>> + if (!(p)->enabled || READ_ONCE(*(p)->enabled))
>>>> +#else
>>>> +#define for_each_enabled_hook for_each_hook
>>>> +#endif
>>>> +
>>>> #define call_void_hook(FUNC, ...) \
>>>> do { \
>>>> struct security_hook_list *P; \
>>>> \
>>>> - hlist_for_each_entry(P, &security_hook_heads.FUNC, list) \
>>>> + for_each_enabled_hook(P, FUNC) \
>>>> P->hook.FUNC(__VA_ARGS__); \
>>>> } while (0)
>>>> @@ -692,7 +703,7 @@ static void __init lsm_early_task(struct task_struct *task)
>>>> do { \
>>>> struct security_hook_list *P; \
>>>> \
>>>> - hlist_for_each_entry(P, &security_hook_heads.FUNC, list) { \
>>>> + for_each_enabled_hook(P, FUNC) { \
>>>> RC = P->hook.FUNC(__VA_ARGS__); \
>>>> if (RC != 0) \
>>>> break; \
>>>> @@ -795,7 +806,7 @@ int security_vm_enough_memory_mm(struct mm_struct *mm, long pages)
>>>> * agree that it should be set it will. If any module
>>>> * thinks it should not be set it won't.
>>>> */
>>>> - hlist_for_each_entry(hp, &security_hook_heads.vm_enough_memory, list) {
>>>> + for_each_enabled_hook(hp, vm_enough_memory) {
>>>> rc = hp->hook.vm_enough_memory(mm, pages);
>>>> if (rc <= 0) {
>>>> cap_sys_admin = 0;
>>>> @@ -1343,7 +1354,7 @@ int security_inode_getsecurity(struct inode *inode, const char *name, void **buf
>>>> /*
>>>> * Only one module will provide an attribute with a given name.
>>>> */
>>>> - hlist_for_each_entry(hp, &security_hook_heads.inode_getsecurity, list) {
>>>> + for_each_enabled_hook(hp, inode_getsecurity) {
>>>> rc = hp->hook.inode_getsecurity(inode, name, buffer, alloc);
>>>> if (rc != -EOPNOTSUPP)
>>>> return rc;
>>>> @@ -1361,7 +1372,7 @@ int security_inode_setsecurity(struct inode *inode, const char *name, const void
>>>> /*
>>>> * Only one module will provide an attribute with a given name.
>>>> */
>>>> - hlist_for_each_entry(hp, &security_hook_heads.inode_setsecurity, list) {
>>>> + for_each_enabled_hook(hp, inode_setsecurity) {
>>>> rc = hp->hook.inode_setsecurity(inode, name, value, size,
>>>> flags);
>>>> if (rc != -EOPNOTSUPP)
>>>> @@ -1744,7 +1755,7 @@ int security_task_prctl(int option, unsigned long arg2, unsigned long arg3,
>>>> int rc = -ENOSYS;
>>>> struct security_hook_list *hp;
>>>> - hlist_for_each_entry(hp, &security_hook_heads.task_prctl, list) {
>>>> + for_each_enabled_hook(hp, task_prctl) {
>>>> thisrc = hp->hook.task_prctl(option, arg2, arg3, arg4, arg5);
>>>> if (thisrc != -ENOSYS) {
>>>> rc = thisrc;
>>>> @@ -1913,7 +1924,7 @@ int security_getprocattr(struct task_struct *p, const char *lsm, char *name,
>>>> {
>>>> struct security_hook_list *hp;
>>>> - hlist_for_each_entry(hp, &security_hook_heads.getprocattr, list) {
>>>> + for_each_enabled_hook(hp, getprocattr) {
>>>> if (lsm != NULL && strcmp(lsm, hp->lsm))
>>>> continue;
>>>> return hp->hook.getprocattr(p, name, value);
>>>> @@ -1926,7 +1937,7 @@ int security_setprocattr(const char *lsm, const char *name, void *value,
>>>> {
>>>> struct security_hook_list *hp;
>>>> - hlist_for_each_entry(hp, &security_hook_heads.setprocattr, list) {
>>>> + for_each_enabled_hook(hp, setprocattr) {
>>>> if (lsm != NULL && strcmp(lsm, hp->lsm))
>>>> continue;
>>>> return hp->hook.setprocattr(name, value, size);
>>>> @@ -2327,8 +2338,7 @@ int security_xfrm_state_pol_flow_match(struct xfrm_state *x,
>>>> * For speed optimization, we explicitly break the loop rather than
>>>> * using the macro
>>>> */
>>>> - hlist_for_each_entry(hp, &security_hook_heads.xfrm_state_pol_flow_match,
>>>> - list) {
>>>> + for_each_enabled_hook(hp, xfrm_state_pol_flow_match) {
>>>> rc = hp->hook.xfrm_state_pol_flow_match(x, xp, fl);
>>>> break;
>>>> }
>>>> diff --git a/security/selinux/Kconfig b/security/selinux/Kconfig
>>>> index 996d35d950f7..f64290de1f8a 100644
>>>> --- a/security/selinux/Kconfig
>>>> +++ b/security/selinux/Kconfig
>>>> @@ -26,7 +26,6 @@ config SECURITY_SELINUX_BOOTPARAM
>>>> config SECURITY_SELINUX_DISABLE
>>>> bool "NSA SELinux runtime disable"
>>>> depends on SECURITY_SELINUX
>>>> - select SECURITY_WRITABLE_HOOKS
>>>> default n
>>>> help
>>>> This option enables writing to a selinuxfs node 'disable', which
>>>> @@ -37,8 +36,8 @@ config SECURITY_SELINUX_DISABLE
>>>> portability across platforms where boot parameters are difficult
>>>> to employ.
>>>> - NOTE: selecting this option will disable the '__ro_after_init'
>>>> - kernel hardening feature for security hooks. Please consider
>>>> + NOTE: Selecting this option might cause memory leaks and random
>>>> + crashes when the runtime disable is used. Please consider
>>>> using the selinux=0 boot parameter instead of enabling this
>>>> option.
>>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>>>> index 47626342b6e5..b76acd98dda5 100644
>>>> --- a/security/selinux/hooks.c
>>>> +++ b/security/selinux/hooks.c
>>>> @@ -123,7 +123,13 @@ __setup("enforcing=", enforcing_setup);
>>>> #define selinux_enforcing_boot 1
>>>> #endif
>>>> -int selinux_enabled __lsm_ro_after_init = 1;
>>>> +/* Currently required to handle SELinux runtime hook disable. */
>>>> +#ifdef CONFIG_SECURITY_SELINUX_DISABLE
>>>> +int selinux_enabled = 1;
>>>> +#else
>>>> +int selinux_enabled __ro_after_init = 1;
>>>> +#endif /* CONFIG_SECURITY_SELINUX_DISABLE */
>>>> +
>>>> #ifdef CONFIG_SECURITY_SELINUX_BOOTPARAM
>>>> static int __init selinux_enabled_setup(char *str)
>>>> {
>>>> @@ -6827,7 +6833,7 @@ static int selinux_lockdown(enum lockdown_reason what)
>>>> LOCKDOWN__CONFIDENTIALITY, &ad);
>>>> }
>>>> -struct lsm_blob_sizes selinux_blob_sizes __lsm_ro_after_init = {
>>>> +struct lsm_blob_sizes selinux_blob_sizes __ro_after_init = {
>>>> .lbs_cred = sizeof(struct task_security_struct),
>>>> .lbs_file = sizeof(struct file_security_struct),
>>>> .lbs_inode = sizeof(struct inode_security_struct),
>>>> @@ -6896,7 +6902,7 @@ static int selinux_perf_event_write(struct perf_event *event)
>>>> }
>>>> #endif
>>>> -static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
>>>> +static struct security_hook_list selinux_hooks[] __ro_after_init = {
>>>> LSM_HOOK_INIT(binder_set_context_mgr, selinux_binder_set_context_mgr),
>>>> LSM_HOOK_INIT(binder_transaction, selinux_binder_transaction),
>>>> LSM_HOOK_INIT(binder_transfer_binder, selinux_binder_transfer_binder),
>>>> @@ -7145,6 +7151,15 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
>>>> static __init int selinux_init(void)
>>>> {
>>>> + struct security_hook_array hook_array = {
>>>> + .hooks = selinux_hooks,
>>>> + .count = ARRAY_SIZE(selinux_hooks),
>>>> + .lsm = "selinux",
>>>> +#ifdef CONFIG_SECURITY_SELINUX_DISABLE
>>>> + .enabled = &selinux_enabled,
>>>> +#endif
>>>> + };
>>>> +
>>>> pr_info("SELinux: Initializing.\n");
>>>> memset(&selinux_state, 0, sizeof(selinux_state));
>>>> @@ -7166,7 +7181,7 @@ static __init int selinux_init(void)
>>>> hashtab_cache_init();
>>>> - security_add_hooks(selinux_hooks, ARRAY_SIZE(selinux_hooks), "selinux");
>>>> + security_add_hook_array(&hook_array);
>>>> if (avc_add_callback(selinux_netcache_avc_callback, AVC_CALLBACK_RESET))
>>>> panic("SELinux: Unable to register AVC netcache callback\n");
>>>> @@ -7319,9 +7334,10 @@ int selinux_disable(struct selinux_state *state)
>>>> pr_info("SELinux: Disabled at runtime.\n");
>>>> - selinux_enabled = 0;
>>>> + /* Unregister netfilter hooks. */
>>>> + selinux_nf_ip_exit();
>>>> - security_delete_hooks(selinux_hooks, ARRAY_SIZE(selinux_hooks));
>>>> + WRITE_ONCE(selinux_enabled, 0);
>>>> /* Try to destroy the avc node cache */
>>>> avc_disable();
>>>> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
>>>> index ecea41ce919b..c21dda12bc4b 100644
>>>> --- a/security/smack/smack_lsm.c
>>>> +++ b/security/smack/smack_lsm.c
>>>> @@ -4583,7 +4583,7 @@ static int smack_dentry_create_files_as(struct dentry *dentry, int mode,
>>>> return 0;
>>>> }
>>>> -struct lsm_blob_sizes smack_blob_sizes __lsm_ro_after_init = {
>>>> +struct lsm_blob_sizes smack_blob_sizes __ro_after_init = {
>>>> .lbs_cred = sizeof(struct task_smack),
>>>> .lbs_file = sizeof(struct smack_known *),
>>>> .lbs_inode = sizeof(struct inode_smack),
>>>> @@ -4591,7 +4591,7 @@ struct lsm_blob_sizes smack_blob_sizes __lsm_ro_after_init = {
>>>> .lbs_msg_msg = sizeof(struct smack_known *),
>>>> };
>>>> -static struct security_hook_list smack_hooks[] __lsm_ro_after_init = {
>>>> +static struct security_hook_list smack_hooks[] __ro_after_init = {
>>>> LSM_HOOK_INIT(ptrace_access_check, smack_ptrace_access_check),
>>>> LSM_HOOK_INIT(ptrace_traceme, smack_ptrace_traceme),
>>>> LSM_HOOK_INIT(syslog, smack_syslog),
>>>> @@ -4765,6 +4765,11 @@ static __init void init_smack_known_list(void)
>>>> */
>>>> static __init int smack_init(void)
>>>> {
>>>> + struct security_hook_array hook_array = {
>>>> + .hooks = smack_hooks,
>>>> + .count = ARRAY_SIZE(smack_hooks),
>>>> + .lsm = "smack",
>>>> + };
>>>> struct cred *cred = (struct cred *) current->cred;
>>>> struct task_smack *tsp;
>>>> @@ -4787,7 +4792,7 @@ static __init int smack_init(void)
>>>> /*
>>>> * Register with LSM
>>>> */
>>>> - security_add_hooks(smack_hooks, ARRAY_SIZE(smack_hooks), "smack");
>>>> + security_add_hook_array(&hook_array);
>>>> smack_enabled = 1;
>>>> pr_info("Smack: Initializing.\n");
>>>> diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
>>>> index 716c92ec941a..a4075379246d 100644
>>>> --- a/security/tomoyo/tomoyo.c
>>>> +++ b/security/tomoyo/tomoyo.c
>>>> @@ -486,7 +486,7 @@ static int tomoyo_socket_sendmsg(struct socket *sock, struct msghdr *msg,
>>>> return tomoyo_socket_sendmsg_permission(sock, msg, size);
>>>> }
>>>> -struct lsm_blob_sizes tomoyo_blob_sizes __lsm_ro_after_init = {
>>>> +struct lsm_blob_sizes tomoyo_blob_sizes __ro_after_init = {
>>>> .lbs_task = sizeof(struct tomoyo_task),
>>>> };
>>>> @@ -533,7 +533,7 @@ static void tomoyo_task_free(struct task_struct *task)
>>>> * tomoyo_security_ops is a "struct security_operations" which is used for
>>>> * registering TOMOYO.
>>>> */
>>>> -static struct security_hook_list tomoyo_hooks[] __lsm_ro_after_init = {
>>>> +static struct security_hook_list tomoyo_hooks[] __ro_after_init = {
>>>> LSM_HOOK_INIT(cred_prepare, tomoyo_cred_prepare),
>>>> LSM_HOOK_INIT(bprm_committed_creds, tomoyo_bprm_committed_creds),
>>>> LSM_HOOK_INIT(task_alloc, tomoyo_task_alloc),
>>>> @@ -569,7 +569,7 @@ static struct security_hook_list tomoyo_hooks[] __lsm_ro_after_init = {
>>>> /* Lock for GC. */
>>>> DEFINE_SRCU(tomoyo_ss);
>>>> -int tomoyo_enabled __lsm_ro_after_init = 1;
>>>> +int tomoyo_enabled __ro_after_init = 1;
>>>> /**
>>>> * tomoyo_init - Register TOMOYO Linux as a LSM module.
>>>> @@ -578,10 +578,15 @@ int tomoyo_enabled __lsm_ro_after_init = 1;
>>>> */
>>>> static int __init tomoyo_init(void)
>>>> {
>>>> + struct security_hook_array hook_array = {
>>>> + .hooks = tomoyo_hooks,
>>>> + .count = ARRAY_SIZE(tomoyo_hooks),
>>>> + .lsm = "tomoyo",
>>>> + };
>>>> struct tomoyo_task *s = tomoyo_task(current);
>>>> /* register ourselves with the security framework */
>>>> - security_add_hooks(tomoyo_hooks, ARRAY_SIZE(tomoyo_hooks), "tomoyo");
>>>> + security_add_hook_array(&hook_array);
>>>> pr_info("TOMOYO Linux initialized\n");
>>>> s->domain_info = &tomoyo_kernel_domain;
>>>> atomic_inc(&tomoyo_kernel_domain.users);
>>>> diff --git a/security/yama/yama_lsm.c b/security/yama/yama_lsm.c
>>>> index 94dc346370b1..ed752f6eafcf 100644
>>>> --- a/security/yama/yama_lsm.c
>>>> +++ b/security/yama/yama_lsm.c
>>>> @@ -421,7 +421,7 @@ static int yama_ptrace_traceme(struct task_struct *parent)
>>>> return rc;
>>>> }
>>>> -static struct security_hook_list yama_hooks[] __lsm_ro_after_init = {
>>>> +static struct security_hook_list yama_hooks[] __ro_after_init = {
>>>> LSM_HOOK_INIT(ptrace_access_check, yama_ptrace_access_check),
>>>> LSM_HOOK_INIT(ptrace_traceme, yama_ptrace_traceme),
>>>> LSM_HOOK_INIT(task_prctl, yama_task_prctl),
>>>> @@ -476,8 +476,14 @@ static inline void yama_init_sysctl(void) { }
>>>> static int __init yama_init(void)
>>>> {
>>>> + struct security_hook_array hook_array = {
>>>> + .hooks = yama_hooks,
>>>> + .count = ARRAY_SIZE(yama_hooks),
>>>> + .lsm = "yama",
>>>> + };
>>>> +
>>>> pr_info("Yama: becoming mindful.\n");
>>>> - security_add_hooks(yama_hooks, ARRAY_SIZE(yama_hooks), "yama");
>>>> + security_add_hook_array(&hook_array);
>>>> yama_init_sysctl();
>>>> return 0;
>>>> }
>>>>
>
^ permalink raw reply
* Re: [PATCH v1 0/3] Introduce CAP_SYS_PERFMON capability for secure Perf users groups
From: Stephane Eranian @ 2019-12-11 19:04 UTC (permalink / raw)
To: Casey Schaufler
Cc: Alexey Budankov, Peter Zijlstra, Arnaldo Carvalho de Melo,
Ingo Molnar, Jiri Olsa, Andi Kleen, elena.reshetova,
Alexander Shishkin, Jann Horn, Kees Cook, Namhyung Kim,
linux-security-module, selinux, linux-kernel
In-Reply-To: <a81248c5-971a-9d3f-6df4-e6335384fe7f@schaufler-ca.com>
On Thu, Dec 5, 2019 at 9:35 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> On 12/5/2019 9:05 AM, Alexey Budankov wrote:
> > Hello Casey,
> >
> > On 05.12.2019 19:49, Casey Schaufler wrote:
> >> On 12/5/2019 8:15 AM, Alexey Budankov wrote:
> >>> Currently access to perf_events functionality [1] beyond the scope permitted
> >>> by perf_event_paranoid [1] kernel setting is allowed to a privileged process
> >>> [2] with CAP_SYS_ADMIN capability enabled in the process effective set [3].
> >>>
> >>> This patch set introduces CAP_SYS_PERFMON capability devoted to secure performance
> >>> monitoring activity so that CAP_SYS_PERFMON would assist CAP_SYS_ADMIN in its
> >>> governing role for perf_events based performance monitoring of a system.
> >>>
> >>> CAP_SYS_PERFMON aims to harden system security and integrity when monitoring
> >>> performance using perf_events subsystem by processes and Perf privileged users
> >>> [2], thus decreasing attack surface that is available to CAP_SYS_ADMIN
> >>> privileged processes [3].
> >> Are there use cases where you would need CAP_SYS_PERFMON where you
> >> would not also need CAP_SYS_ADMIN? If you separate a new capability
> > Actually, there are. Perf tool that has record, stat and top modes could run with
> > CAP_SYS_PERFMON capability as mentioned below and provide system wide performance
> > data. Currently for that to work the tool needs to be granted with CAP_SYS_ADMIN.
>
> The question isn't whether the tool could use the capability, it's whether
> the tool would also need CAP_SYS_ADMIN to be useful. Are there existing
> tools that could stop using CAP_SYS_ADMIN in favor of CAP_SYS_PERFMON?
The answer is yes. I have recently been alerted to a problem with
paranoid=2 and the
popular rr debugger (https://rr-project.org/). This debugger uses
several perf_events
features, including profiling of PMU events and tracepoints
(context-switches). With
paranoid=2, it does not work anymore. We would need a privilege between regular
user and admin to make it work again. Note that context switches
tracepoint is only
applied to self (not system-wide).
> My bet is that any tool that does performance monitoring is going to need
> CAP_SYS_ADMIN for other reasons.
>
> >
> >> from CAP_SYS_ADMIN but always have to use CAP_SYS_ADMIN in conjunction
> >> with the new capability it is all rather pointless.
> >>
> >> The scope you've defined for this CAP_SYS_PERFMON is very small.
> >> Is there a larger set of privilege checks that might be applicable
> >> for it?
> > CAP_SYS_PERFMON could be applied broadly, though, this patch set enables record
> > and stat mode use cases for system wide performance monitoring in kernel and
> > user modes.
>
> The granularity of capabilities is something we have to watch
> very carefully. Sure, CAP_SYS_ADMIN covers a lot of things, but
> if we broke it up "properly" we'd have hundreds of capabilities.
> If you want control that finely we have SELinux.
>
> >
> > Thanks,
> > Alexey
> >
> >>
> >>
> >>> CAP_SYS_PERFMON aims to take over CAP_SYS_ADMIN credentials related to
> >>> performance monitoring functionality of perf_events and balance amount of
> >>> CAP_SYS_ADMIN credentials in accordance with the recommendations provided in
> >>> the man page for CAP_SYS_ADMIN [3]: "Note: this capability is overloaded;
> >>> see Notes to kernel developers, below."
> >>>
> >>> For backward compatibility reasons performance monitoring functionality of
> >>> perf_events subsystem remains available under CAP_SYS_ADMIN but its usage for
> >>> secure performance monitoring use cases is discouraged with respect to the
> >>> introduced CAP_SYS_PERFMON capability.
> >>>
> >>> In the suggested implementation CAP_SYS_PERFMON enables Perf privileged users
> >>> [2] to conduct secure performance monitoring using perf_events in the scope
> >>> of available online CPUs when executing code in kernel and user modes.
> >>>
> >>> Possible alternative solution to this capabilities balancing, system security
> >>> hardening task could be to use the existing CAP_SYS_PTRACE capability to govern
> >>> perf_events' performance monitoring functionality, since process debugging is
> >>> similar to performance monitoring with respect to providing insights into
> >>> process memory and execution details. However CAP_SYS_PTRACE still provides
> >>> users with more credentials than are required for secure performance monitoring
> >>> using perf_events subsystem and this excess is avoided by using the dedicated
> >>> CAP_SYS_PERFMON capability.
> >>>
> >>> libcap library utilities [4], [5] and Perf tool can be used to apply
> >>> CAP_SYS_PERFMON capability for secure performance monitoring beyond the scope
> >>> permitted by system wide perf_event_paranoid kernel setting and below are the
> >>> steps to evaluate the advancement suggested by the patch set:
> >>>
> >>> - patch, build and boot the kernel
> >>> - patch, build Perf tool e.g. to /home/user/perf
> >>> ...
> >>> # git clone git://git.kernel.org/pub/scm/libs/libcap/libcap.git libcap
> >>> # pushd libcap
> >>> # patch libcap/include/uapi/linux/capabilities.h with [PATCH 1/3]
> >>> # make
> >>> # pushd progs
> >>> # ./setcap "cap_sys_perfmon,cap_sys_ptrace,cap_syslog=ep" /home/user/perf
> >>> # ./setcap -v "cap_sys_perfmon,cap_sys_ptrace,cap_syslog=ep" /home/user/perf
> >>> /home/user/perf: OK
> >>> # ./getcap /home/user/perf
> >>> /home/user/perf = cap_sys_ptrace,cap_syslog,cap_sys_perfmon+ep
> >>> # echo 2 > /proc/sys/kernel/perf_event_paranoid
> >>> # cat /proc/sys/kernel/perf_event_paranoid
> >>> 2
> >>> ...
> >>> $ /home/user/perf top
> >>> ... works as expected ...
> >>> $ cat /proc/`pidof perf`/status
> >>> Name: perf
> >>> Umask: 0002
> >>> State: S (sleeping)
> >>> Tgid: 2958
> >>> Ngid: 0
> >>> Pid: 2958
> >>> PPid: 9847
> >>> TracerPid: 0
> >>> Uid: 500 500 500 500
> >>> Gid: 500 500 500 500
> >>> FDSize: 256
> >>> ...
> >>> CapInh: 0000000000000000
> >>> CapPrm: 0000004400080000
> >>> CapEff: 0000004400080000 => 01000100 00000000 00001000 00000000 00000000
> >>> cap_sys_perfmon,cap_sys_ptrace,cap_syslog
> >>> CapBnd: 0000007fffffffff
> >>> CapAmb: 0000000000000000
> >>> NoNewPrivs: 0
> >>> Seccomp: 0
> >>> Speculation_Store_Bypass: thread vulnerable
> >>> Cpus_allowed: ff
> >>> Cpus_allowed_list: 0-7
> >>> ...
> >>>
> >>> Usage of cap_sys_perfmon effectively avoids unused credentials excess:
> >>> - with cap_sys_admin:
> >>> CapEff: 0000007fffffffff => 01111111 11111111 11111111 11111111 11111111
> >>> - with cap_sys_perfmon:
> >>> CapEff: 0000004400080000 => 01000100 00000000 00001000 00000000 00000000
> >>> 38 34 19
> >>> sys_perfmon syslog sys_ptrace
> >>>
> >>> The patch set is for tip perf/core repository:
> >>> git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip perf/core
> >>> tip sha1: ceb9e77324fa661b1001a0ae66f061b5fcb4e4e6
> >>>
> >>> [1] http://man7.org/linux/man-pages/man2/perf_event_open.2.html
> >>> [2] https://www.kernel.org/doc/html/latest/admin-guide/perf-security.html
> >>> [3] http://man7.org/linux/man-pages/man7/capabilities.7.html
> >>> [4] http://man7.org/linux/man-pages/man8/setcap.8.html
> >>> [5] https://git.kernel.org/pub/scm/libs/libcap/libcap.git
> >>> [6] https://sites.google.com/site/fullycapable/, posix_1003.1e-990310.pdf
> >>>
> >>> ---
> >>> Alexey Budankov (3):
> >>> capabilities: introduce CAP_SYS_PERFMON to kernel and user space
> >>> perf/core: apply CAP_SYS_PERFMON to CPUs and kernel monitoring
> >>> perf tool: extend Perf tool with CAP_SYS_PERFMON support
> >>>
> >>> include/linux/perf_event.h | 6 ++++--
> >>> include/uapi/linux/capability.h | 10 +++++++++-
> >>> security/selinux/include/classmap.h | 4 ++--
> >>> tools/perf/design.txt | 3 ++-
> >>> tools/perf/util/cap.h | 4 ++++
> >>> tools/perf/util/evsel.c | 10 +++++-----
> >>> tools/perf/util/util.c | 15 +++++++++++++--
> >>> 7 files changed, 39 insertions(+), 13 deletions(-)
> >>>
> >>
>
^ permalink raw reply
* Re: [PATCH] LSM: allow an LSM to disable all hooks at once
From: Casey Schaufler @ 2019-12-11 18:35 UTC (permalink / raw)
To: Kees Cook, Stephen Smalley
Cc: Ondrej Mosnacek, linux-security-module, James Morris,
Serge E. Hallyn, selinux, Paul Moore, John Johansen, Micah Morton,
Tetsuo Handa, Casey Schaufler
In-Reply-To: <201912110840.62A4E64BA@keescook>
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.
> -Kees
>
>>> Fixes: b1d9e6b0646d ("LSM: Switch to lists of hooks")
>>> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
>>> ---
>>>
>>> This is an alternative to [1]. It turned out to be less simple than I
>>> had hoped, but OTOH the hook arrays can now be unconditionally made
>>> __ro_after_init so may be still worth it.
>>>
>>> Compile- and boot- tested with SECURITY_SELINUX_DISABLE set to =y and
>>> =n; SELinux enabled. Changes to other LSMs only compile-tested (but
>>> those are trivial).
>>>
>>> [1] https://lore.kernel.org/selinux/20191209075756.123157-1-omosnace@redhat.com/T/
>>>
>>> include/linux/lsm_hooks.h | 46 +++++++++----------------------
>>> security/Kconfig | 5 ----
>>> security/apparmor/lsm.c | 14 ++++++----
>>> security/commoncap.c | 11 +++++---
>>> security/loadpin/loadpin.c | 10 +++++--
>>> security/lockdown/lockdown.c | 11 +++++---
>>> security/safesetid/lsm.c | 9 +++++--
>>> security/security.c | 52 +++++++++++++++++++++---------------
>>> security/selinux/Kconfig | 5 ++--
>>> security/selinux/hooks.c | 28 ++++++++++++++-----
>>> security/smack/smack_lsm.c | 11 +++++---
>>> security/tomoyo/tomoyo.c | 13 ++++++---
>>> security/yama/yama_lsm.c | 10 +++++--
>>> 13 files changed, 133 insertions(+), 92 deletions(-)
>>>
>>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
>>> index 20d8cf194fb7..91b77ebcb822 100644
>>> --- a/include/linux/lsm_hooks.h
>>> +++ b/include/linux/lsm_hooks.h
>>> @@ -27,7 +27,6 @@
>>> #include <linux/security.h>
>>> #include <linux/init.h>
>>> -#include <linux/rculist.h>
>>> /**
>>> * union security_list_options - Linux Security Module hook function list
>>> @@ -2086,6 +2085,9 @@ struct security_hook_list {
>>> struct hlist_head *head;
>>> union security_list_options hook;
>>> char *lsm;
>>> +#ifdef CONFIG_SECURITY_SELINUX_DISABLE
>>> + const int *enabled;
>>> +#endif
>>> } __randomize_layout;
>>> /*
>>> @@ -2112,8 +2114,16 @@ struct lsm_blob_sizes {
>>> extern struct security_hook_heads security_hook_heads;
>>> extern char *lsm_names;
>>> -extern void security_add_hooks(struct security_hook_list *hooks, int count,
>>> - char *lsm);
>>> +struct security_hook_array {
>>> + struct security_hook_list *hooks;
>>> + int count;
>>> + char *lsm;
>>> +#ifdef CONFIG_SECURITY_SELINUX_DISABLE
>>> + const int *enabled;
>>> +#endif
>>> +};
>>> +
>>> +extern void security_add_hook_array(const struct security_hook_array *array);
>>> #define LSM_FLAG_LEGACY_MAJOR BIT(0)
>>> #define LSM_FLAG_EXCLUSIVE BIT(1)
>>> @@ -2145,36 +2155,6 @@ extern struct lsm_info __start_early_lsm_info[], __end_early_lsm_info[];
>>> __used __section(.early_lsm_info.init) \
>>> __aligned(sizeof(unsigned long))
>>> -#ifdef CONFIG_SECURITY_SELINUX_DISABLE
>>> -/*
>>> - * Assuring the safety of deleting a security module is up to
>>> - * the security module involved. This may entail ordering the
>>> - * module's hook list in a particular way, refusing to disable
>>> - * the module once a policy is loaded or any number of other
>>> - * actions better imagined than described.
>>> - *
>>> - * The name of the configuration option reflects the only module
>>> - * that currently uses the mechanism. Any developer who thinks
>>> - * disabling their module is a good idea needs to be at least as
>>> - * careful as the SELinux team.
>>> - */
>>> -static inline void security_delete_hooks(struct security_hook_list *hooks,
>>> - int count)
>>> -{
>>> - int i;
>>> -
>>> - for (i = 0; i < count; i++)
>>> - hlist_del_rcu(&hooks[i].list);
>>> -}
>>> -#endif /* CONFIG_SECURITY_SELINUX_DISABLE */
>>> -
>>> -/* Currently required to handle SELinux runtime hook disable. */
>>> -#ifdef CONFIG_SECURITY_WRITABLE_HOOKS
>>> -#define __lsm_ro_after_init
>>> -#else
>>> -#define __lsm_ro_after_init __ro_after_init
>>> -#endif /* CONFIG_SECURITY_WRITABLE_HOOKS */
>>> -
>>> extern int lsm_inode_alloc(struct inode *inode);
>>> #endif /* ! __LINUX_LSM_HOOKS_H */
>>> diff --git a/security/Kconfig b/security/Kconfig
>>> index 2a1a2d396228..456764990a13 100644
>>> --- a/security/Kconfig
>>> +++ b/security/Kconfig
>>> @@ -32,11 +32,6 @@ config SECURITY
>>> If you are unsure how to answer this question, answer N.
>>> -config SECURITY_WRITABLE_HOOKS
>>> - depends on SECURITY
>>> - bool
>>> - default n
>>> -
>>> config SECURITYFS
>>> bool "Enable the securityfs filesystem"
>>> help
>>> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
>>> index b621ad74f54a..a27f48670b37 100644
>>> --- a/security/apparmor/lsm.c
>>> +++ b/security/apparmor/lsm.c
>>> @@ -1158,13 +1158,13 @@ static int apparmor_inet_conn_request(struct sock *sk, struct sk_buff *skb,
>>> /*
>>> * The cred blob is a pointer to, not an instance of, an aa_task_ctx.
>>> */
>>> -struct lsm_blob_sizes apparmor_blob_sizes __lsm_ro_after_init = {
>>> +struct lsm_blob_sizes apparmor_blob_sizes __ro_after_init = {
>>> .lbs_cred = sizeof(struct aa_task_ctx *),
>>> .lbs_file = sizeof(struct aa_file_ctx),
>>> .lbs_task = sizeof(struct aa_task_ctx),
>>> };
>>> -static struct security_hook_list apparmor_hooks[] __lsm_ro_after_init = {
>>> +static struct security_hook_list apparmor_hooks[] __ro_after_init = {
>>> LSM_HOOK_INIT(ptrace_access_check, apparmor_ptrace_access_check),
>>> LSM_HOOK_INIT(ptrace_traceme, apparmor_ptrace_traceme),
>>> LSM_HOOK_INIT(capget, apparmor_capget),
>>> @@ -1368,7 +1368,7 @@ static const struct kernel_param_ops param_ops_aaintbool = {
>>> .get = param_get_aaintbool
>>> };
>>> /* Boot time disable flag */
>>> -static int apparmor_enabled __lsm_ro_after_init = 1;
>>> +static int apparmor_enabled __ro_after_init = 1;
>>> module_param_named(enabled, apparmor_enabled, aaintbool, 0444);
>>> static int __init apparmor_enabled_setup(char *str)
>>> @@ -1829,6 +1829,11 @@ __initcall(apparmor_nf_ip_init);
>>> static int __init apparmor_init(void)
>>> {
>>> + struct security_hook_array hook_array = {
>>> + .hooks = apparmor_hooks,
>>> + .count = ARRAY_SIZE(apparmor_hooks),
>>> + .lsm = "apparmor",
>>> + };
>>> int error;
>>> aa_secids_init();
>>> @@ -1864,8 +1869,7 @@ static int __init apparmor_init(void)
>>> aa_free_root_ns();
>>> goto buffers_out;
>>> }
>>> - security_add_hooks(apparmor_hooks, ARRAY_SIZE(apparmor_hooks),
>>> - "apparmor");
>>> + security_add_hook_array(&hook_array);
>>> /* Report that AppArmor successfully initialized */
>>> apparmor_initialized = 1;
>>> diff --git a/security/commoncap.c b/security/commoncap.c
>>> index f4ee0ae106b2..6e9f4b6b6b1d 100644
>>> --- a/security/commoncap.c
>>> +++ b/security/commoncap.c
>>> @@ -1339,7 +1339,7 @@ int cap_mmap_file(struct file *file, unsigned long reqprot,
>>> #ifdef CONFIG_SECURITY
>>> -static struct security_hook_list capability_hooks[] __lsm_ro_after_init = {
>>> +static struct security_hook_list capability_hooks[] __ro_after_init = {
>>> LSM_HOOK_INIT(capable, cap_capable),
>>> LSM_HOOK_INIT(settime, cap_settime),
>>> LSM_HOOK_INIT(ptrace_access_check, cap_ptrace_access_check),
>>> @@ -1362,8 +1362,13 @@ static struct security_hook_list capability_hooks[] __lsm_ro_after_init = {
>>> static int __init capability_init(void)
>>> {
>>> - security_add_hooks(capability_hooks, ARRAY_SIZE(capability_hooks),
>>> - "capability");
>>> + struct security_hook_array hook_array = {
>>> + .hooks = capability_hooks,
>>> + .count = ARRAY_SIZE(capability_hooks),
>>> + .lsm = "capability",
>>> + };
>>> +
>>> + security_add_hook_array(&hook_array);
>>> return 0;
>>> }
>>> diff --git a/security/loadpin/loadpin.c b/security/loadpin/loadpin.c
>>> index ee5cb944f4ad..5fadd4969d90 100644
>>> --- a/security/loadpin/loadpin.c
>>> +++ b/security/loadpin/loadpin.c
>>> @@ -180,7 +180,7 @@ static int loadpin_load_data(enum kernel_load_data_id id)
>>> return loadpin_read_file(NULL, (enum kernel_read_file_id) id);
>>> }
>>> -static struct security_hook_list loadpin_hooks[] __lsm_ro_after_init = {
>>> +static struct security_hook_list loadpin_hooks[] __ro_after_init = {
>>> LSM_HOOK_INIT(sb_free_security, loadpin_sb_free_security),
>>> LSM_HOOK_INIT(kernel_read_file, loadpin_read_file),
>>> LSM_HOOK_INIT(kernel_load_data, loadpin_load_data),
>>> @@ -224,10 +224,16 @@ static void __init parse_exclude(void)
>>> static int __init loadpin_init(void)
>>> {
>>> + struct security_hook_array hook_array = {
>>> + .hooks = loadpin_hooks,
>>> + .count = ARRAY_SIZE(loadpin_hooks),
>>> + .lsm = "loadpin",
>>> + };
>>> +
>>> pr_info("ready to pin (currently %senforcing)\n",
>>> enforce ? "" : "not ");
>>> parse_exclude();
>>> - security_add_hooks(loadpin_hooks, ARRAY_SIZE(loadpin_hooks), "loadpin");
>>> + security_add_hook_array(&hook_array);
>>> return 0;
>>> }
>>> diff --git a/security/lockdown/lockdown.c b/security/lockdown/lockdown.c
>>> index 5a952617a0eb..bcfa0ff4ee63 100644
>>> --- a/security/lockdown/lockdown.c
>>> +++ b/security/lockdown/lockdown.c
>>> @@ -71,19 +71,24 @@ static int lockdown_is_locked_down(enum lockdown_reason what)
>>> return 0;
>>> }
>>> -static struct security_hook_list lockdown_hooks[] __lsm_ro_after_init = {
>>> +static struct security_hook_list lockdown_hooks[] __ro_after_init = {
>>> LSM_HOOK_INIT(locked_down, lockdown_is_locked_down),
>>> };
>>> static int __init lockdown_lsm_init(void)
>>> {
>>> + struct security_hook_array hook_array = {
>>> + .hooks = lockdown_hooks,
>>> + .count = ARRAY_SIZE(lockdown_hooks),
>>> + .lsm = "lockdown",
>>> + };
>>> +
>>> #if defined(CONFIG_LOCK_DOWN_KERNEL_FORCE_INTEGRITY)
>>> lock_kernel_down("Kernel configuration", LOCKDOWN_INTEGRITY_MAX);
>>> #elif defined(CONFIG_LOCK_DOWN_KERNEL_FORCE_CONFIDENTIALITY)
>>> lock_kernel_down("Kernel configuration", LOCKDOWN_CONFIDENTIALITY_MAX);
>>> #endif
>>> - security_add_hooks(lockdown_hooks, ARRAY_SIZE(lockdown_hooks),
>>> - "lockdown");
>>> + security_add_hook_array(&hook_array);
>>> return 0;
>>> }
>>> diff --git a/security/safesetid/lsm.c b/security/safesetid/lsm.c
>>> index 7760019ad35d..4e36e53f033d 100644
>>> --- a/security/safesetid/lsm.c
>>> +++ b/security/safesetid/lsm.c
>>> @@ -156,8 +156,13 @@ static struct security_hook_list safesetid_security_hooks[] = {
>>> static int __init safesetid_security_init(void)
>>> {
>>> - security_add_hooks(safesetid_security_hooks,
>>> - ARRAY_SIZE(safesetid_security_hooks), "safesetid");
>>> + struct security_hook_array hook_array = {
>>> + .hooks = safesetid_security_hooks,
>>> + .count = ARRAY_SIZE(safesetid_security_hooks),
>>> + .lsm = "safesetid",
>>> + };
>>> +
>>> + security_add_hook_array(&hook_array);
>>> /* Report that SafeSetID successfully initialized */
>>> safesetid_initialized = 1;
>>> diff --git a/security/security.c b/security/security.c
>>> index 2b5473d92416..a5dd348bd37e 100644
>>> --- a/security/security.c
>>> +++ b/security/security.c
>>> @@ -68,14 +68,14 @@ const char *const lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1] = {
>>> [LOCKDOWN_CONFIDENTIALITY_MAX] = "confidentiality",
>>> };
>>> -struct security_hook_heads security_hook_heads __lsm_ro_after_init;
>>> +struct security_hook_heads security_hook_heads __ro_after_init;
>>> static BLOCKING_NOTIFIER_HEAD(blocking_lsm_notifier_chain);
>>> static struct kmem_cache *lsm_file_cache;
>>> static struct kmem_cache *lsm_inode_cache;
>>> char *lsm_names;
>>> -static struct lsm_blob_sizes blob_sizes __lsm_ro_after_init;
>>> +static struct lsm_blob_sizes blob_sizes __ro_after_init;
>>> /* Boot-time LSM user choice */
>>> static __initdata const char *chosen_lsm_order;
>>> @@ -467,20 +467,20 @@ static int lsm_append(const char *new, char **result)
>>> /**
>>> * security_add_hooks - Add a modules hooks to the hook lists.
>>> - * @hooks: the hooks to add
>>> - * @count: the number of hooks to add
>>> - * @lsm: the name of the security module
>>> + * @array: the struct describing hooks to add
>>> *
>>> * Each LSM has to register its hooks with the infrastructure.
>>> */
>>> -void __init security_add_hooks(struct security_hook_list *hooks, int count,
>>> - char *lsm)
>>> +void __init security_add_hook_array(const struct security_hook_array *array)
>>> {
>>> int i;
>>> - for (i = 0; i < count; i++) {
>>> - hooks[i].lsm = lsm;
>>> - hlist_add_tail_rcu(&hooks[i].list, hooks[i].head);
>>> + for (i = 0; i < array->count; i++) {
>>> + array->hooks[i].lsm = array->lsm;
>>> +#ifdef CONFIG_SECURITY_SELINUX_DISABLE
>>> + array->hooks[i].enabled = array->enabled;
>>> +#endif
>>> + hlist_add_tail_rcu(&array->hooks[i].list, array->hooks[i].head);
>>> }
>>> /*
>>> @@ -488,7 +488,7 @@ void __init security_add_hooks(struct security_hook_list *hooks, int count,
>>> * and fix this up afterwards.
>>> */
>>> if (slab_is_available()) {
>>> - if (lsm_append(lsm, &lsm_names) < 0)
>>> + if (lsm_append(array->lsm, &lsm_names) < 0)
>>> panic("%s - Cannot get early memory.\n", __func__);
>>> }
>>> }
>>> @@ -679,11 +679,22 @@ static void __init lsm_early_task(struct task_struct *task)
>>> * This is a hook that returns a value.
>>> */
>>> +#define for_each_hook(p, func) \
>>> + hlist_for_each_entry(p, &security_hook_heads.func, list)
>>> +
>>> +#ifdef CONFIG_SECURITY_SELINUX_DISABLE
>>> +#define for_each_enabled_hook(p, func) \
>>> + for_each_hook(p, func) \
>>> + if (!(p)->enabled || READ_ONCE(*(p)->enabled))
>>> +#else
>>> +#define for_each_enabled_hook for_each_hook
>>> +#endif
>>> +
>>> #define call_void_hook(FUNC, ...) \
>>> do { \
>>> struct security_hook_list *P; \
>>> \
>>> - hlist_for_each_entry(P, &security_hook_heads.FUNC, list) \
>>> + for_each_enabled_hook(P, FUNC) \
>>> P->hook.FUNC(__VA_ARGS__); \
>>> } while (0)
>>> @@ -692,7 +703,7 @@ static void __init lsm_early_task(struct task_struct *task)
>>> do { \
>>> struct security_hook_list *P; \
>>> \
>>> - hlist_for_each_entry(P, &security_hook_heads.FUNC, list) { \
>>> + for_each_enabled_hook(P, FUNC) { \
>>> RC = P->hook.FUNC(__VA_ARGS__); \
>>> if (RC != 0) \
>>> break; \
>>> @@ -795,7 +806,7 @@ int security_vm_enough_memory_mm(struct mm_struct *mm, long pages)
>>> * agree that it should be set it will. If any module
>>> * thinks it should not be set it won't.
>>> */
>>> - hlist_for_each_entry(hp, &security_hook_heads.vm_enough_memory, list) {
>>> + for_each_enabled_hook(hp, vm_enough_memory) {
>>> rc = hp->hook.vm_enough_memory(mm, pages);
>>> if (rc <= 0) {
>>> cap_sys_admin = 0;
>>> @@ -1343,7 +1354,7 @@ int security_inode_getsecurity(struct inode *inode, const char *name, void **buf
>>> /*
>>> * Only one module will provide an attribute with a given name.
>>> */
>>> - hlist_for_each_entry(hp, &security_hook_heads.inode_getsecurity, list) {
>>> + for_each_enabled_hook(hp, inode_getsecurity) {
>>> rc = hp->hook.inode_getsecurity(inode, name, buffer, alloc);
>>> if (rc != -EOPNOTSUPP)
>>> return rc;
>>> @@ -1361,7 +1372,7 @@ int security_inode_setsecurity(struct inode *inode, const char *name, const void
>>> /*
>>> * Only one module will provide an attribute with a given name.
>>> */
>>> - hlist_for_each_entry(hp, &security_hook_heads.inode_setsecurity, list) {
>>> + for_each_enabled_hook(hp, inode_setsecurity) {
>>> rc = hp->hook.inode_setsecurity(inode, name, value, size,
>>> flags);
>>> if (rc != -EOPNOTSUPP)
>>> @@ -1744,7 +1755,7 @@ int security_task_prctl(int option, unsigned long arg2, unsigned long arg3,
>>> int rc = -ENOSYS;
>>> struct security_hook_list *hp;
>>> - hlist_for_each_entry(hp, &security_hook_heads.task_prctl, list) {
>>> + for_each_enabled_hook(hp, task_prctl) {
>>> thisrc = hp->hook.task_prctl(option, arg2, arg3, arg4, arg5);
>>> if (thisrc != -ENOSYS) {
>>> rc = thisrc;
>>> @@ -1913,7 +1924,7 @@ int security_getprocattr(struct task_struct *p, const char *lsm, char *name,
>>> {
>>> struct security_hook_list *hp;
>>> - hlist_for_each_entry(hp, &security_hook_heads.getprocattr, list) {
>>> + for_each_enabled_hook(hp, getprocattr) {
>>> if (lsm != NULL && strcmp(lsm, hp->lsm))
>>> continue;
>>> return hp->hook.getprocattr(p, name, value);
>>> @@ -1926,7 +1937,7 @@ int security_setprocattr(const char *lsm, const char *name, void *value,
>>> {
>>> struct security_hook_list *hp;
>>> - hlist_for_each_entry(hp, &security_hook_heads.setprocattr, list) {
>>> + for_each_enabled_hook(hp, setprocattr) {
>>> if (lsm != NULL && strcmp(lsm, hp->lsm))
>>> continue;
>>> return hp->hook.setprocattr(name, value, size);
>>> @@ -2327,8 +2338,7 @@ int security_xfrm_state_pol_flow_match(struct xfrm_state *x,
>>> * For speed optimization, we explicitly break the loop rather than
>>> * using the macro
>>> */
>>> - hlist_for_each_entry(hp, &security_hook_heads.xfrm_state_pol_flow_match,
>>> - list) {
>>> + for_each_enabled_hook(hp, xfrm_state_pol_flow_match) {
>>> rc = hp->hook.xfrm_state_pol_flow_match(x, xp, fl);
>>> break;
>>> }
>>> diff --git a/security/selinux/Kconfig b/security/selinux/Kconfig
>>> index 996d35d950f7..f64290de1f8a 100644
>>> --- a/security/selinux/Kconfig
>>> +++ b/security/selinux/Kconfig
>>> @@ -26,7 +26,6 @@ config SECURITY_SELINUX_BOOTPARAM
>>> config SECURITY_SELINUX_DISABLE
>>> bool "NSA SELinux runtime disable"
>>> depends on SECURITY_SELINUX
>>> - select SECURITY_WRITABLE_HOOKS
>>> default n
>>> help
>>> This option enables writing to a selinuxfs node 'disable', which
>>> @@ -37,8 +36,8 @@ config SECURITY_SELINUX_DISABLE
>>> portability across platforms where boot parameters are difficult
>>> to employ.
>>> - NOTE: selecting this option will disable the '__ro_after_init'
>>> - kernel hardening feature for security hooks. Please consider
>>> + NOTE: Selecting this option might cause memory leaks and random
>>> + crashes when the runtime disable is used. Please consider
>>> using the selinux=0 boot parameter instead of enabling this
>>> option.
>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>>> index 47626342b6e5..b76acd98dda5 100644
>>> --- a/security/selinux/hooks.c
>>> +++ b/security/selinux/hooks.c
>>> @@ -123,7 +123,13 @@ __setup("enforcing=", enforcing_setup);
>>> #define selinux_enforcing_boot 1
>>> #endif
>>> -int selinux_enabled __lsm_ro_after_init = 1;
>>> +/* Currently required to handle SELinux runtime hook disable. */
>>> +#ifdef CONFIG_SECURITY_SELINUX_DISABLE
>>> +int selinux_enabled = 1;
>>> +#else
>>> +int selinux_enabled __ro_after_init = 1;
>>> +#endif /* CONFIG_SECURITY_SELINUX_DISABLE */
>>> +
>>> #ifdef CONFIG_SECURITY_SELINUX_BOOTPARAM
>>> static int __init selinux_enabled_setup(char *str)
>>> {
>>> @@ -6827,7 +6833,7 @@ static int selinux_lockdown(enum lockdown_reason what)
>>> LOCKDOWN__CONFIDENTIALITY, &ad);
>>> }
>>> -struct lsm_blob_sizes selinux_blob_sizes __lsm_ro_after_init = {
>>> +struct lsm_blob_sizes selinux_blob_sizes __ro_after_init = {
>>> .lbs_cred = sizeof(struct task_security_struct),
>>> .lbs_file = sizeof(struct file_security_struct),
>>> .lbs_inode = sizeof(struct inode_security_struct),
>>> @@ -6896,7 +6902,7 @@ static int selinux_perf_event_write(struct perf_event *event)
>>> }
>>> #endif
>>> -static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
>>> +static struct security_hook_list selinux_hooks[] __ro_after_init = {
>>> LSM_HOOK_INIT(binder_set_context_mgr, selinux_binder_set_context_mgr),
>>> LSM_HOOK_INIT(binder_transaction, selinux_binder_transaction),
>>> LSM_HOOK_INIT(binder_transfer_binder, selinux_binder_transfer_binder),
>>> @@ -7145,6 +7151,15 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
>>> static __init int selinux_init(void)
>>> {
>>> + struct security_hook_array hook_array = {
>>> + .hooks = selinux_hooks,
>>> + .count = ARRAY_SIZE(selinux_hooks),
>>> + .lsm = "selinux",
>>> +#ifdef CONFIG_SECURITY_SELINUX_DISABLE
>>> + .enabled = &selinux_enabled,
>>> +#endif
>>> + };
>>> +
>>> pr_info("SELinux: Initializing.\n");
>>> memset(&selinux_state, 0, sizeof(selinux_state));
>>> @@ -7166,7 +7181,7 @@ static __init int selinux_init(void)
>>> hashtab_cache_init();
>>> - security_add_hooks(selinux_hooks, ARRAY_SIZE(selinux_hooks), "selinux");
>>> + security_add_hook_array(&hook_array);
>>> if (avc_add_callback(selinux_netcache_avc_callback, AVC_CALLBACK_RESET))
>>> panic("SELinux: Unable to register AVC netcache callback\n");
>>> @@ -7319,9 +7334,10 @@ int selinux_disable(struct selinux_state *state)
>>> pr_info("SELinux: Disabled at runtime.\n");
>>> - selinux_enabled = 0;
>>> + /* Unregister netfilter hooks. */
>>> + selinux_nf_ip_exit();
>>> - security_delete_hooks(selinux_hooks, ARRAY_SIZE(selinux_hooks));
>>> + WRITE_ONCE(selinux_enabled, 0);
>>> /* Try to destroy the avc node cache */
>>> avc_disable();
>>> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
>>> index ecea41ce919b..c21dda12bc4b 100644
>>> --- a/security/smack/smack_lsm.c
>>> +++ b/security/smack/smack_lsm.c
>>> @@ -4583,7 +4583,7 @@ static int smack_dentry_create_files_as(struct dentry *dentry, int mode,
>>> return 0;
>>> }
>>> -struct lsm_blob_sizes smack_blob_sizes __lsm_ro_after_init = {
>>> +struct lsm_blob_sizes smack_blob_sizes __ro_after_init = {
>>> .lbs_cred = sizeof(struct task_smack),
>>> .lbs_file = sizeof(struct smack_known *),
>>> .lbs_inode = sizeof(struct inode_smack),
>>> @@ -4591,7 +4591,7 @@ struct lsm_blob_sizes smack_blob_sizes __lsm_ro_after_init = {
>>> .lbs_msg_msg = sizeof(struct smack_known *),
>>> };
>>> -static struct security_hook_list smack_hooks[] __lsm_ro_after_init = {
>>> +static struct security_hook_list smack_hooks[] __ro_after_init = {
>>> LSM_HOOK_INIT(ptrace_access_check, smack_ptrace_access_check),
>>> LSM_HOOK_INIT(ptrace_traceme, smack_ptrace_traceme),
>>> LSM_HOOK_INIT(syslog, smack_syslog),
>>> @@ -4765,6 +4765,11 @@ static __init void init_smack_known_list(void)
>>> */
>>> static __init int smack_init(void)
>>> {
>>> + struct security_hook_array hook_array = {
>>> + .hooks = smack_hooks,
>>> + .count = ARRAY_SIZE(smack_hooks),
>>> + .lsm = "smack",
>>> + };
>>> struct cred *cred = (struct cred *) current->cred;
>>> struct task_smack *tsp;
>>> @@ -4787,7 +4792,7 @@ static __init int smack_init(void)
>>> /*
>>> * Register with LSM
>>> */
>>> - security_add_hooks(smack_hooks, ARRAY_SIZE(smack_hooks), "smack");
>>> + security_add_hook_array(&hook_array);
>>> smack_enabled = 1;
>>> pr_info("Smack: Initializing.\n");
>>> diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
>>> index 716c92ec941a..a4075379246d 100644
>>> --- a/security/tomoyo/tomoyo.c
>>> +++ b/security/tomoyo/tomoyo.c
>>> @@ -486,7 +486,7 @@ static int tomoyo_socket_sendmsg(struct socket *sock, struct msghdr *msg,
>>> return tomoyo_socket_sendmsg_permission(sock, msg, size);
>>> }
>>> -struct lsm_blob_sizes tomoyo_blob_sizes __lsm_ro_after_init = {
>>> +struct lsm_blob_sizes tomoyo_blob_sizes __ro_after_init = {
>>> .lbs_task = sizeof(struct tomoyo_task),
>>> };
>>> @@ -533,7 +533,7 @@ static void tomoyo_task_free(struct task_struct *task)
>>> * tomoyo_security_ops is a "struct security_operations" which is used for
>>> * registering TOMOYO.
>>> */
>>> -static struct security_hook_list tomoyo_hooks[] __lsm_ro_after_init = {
>>> +static struct security_hook_list tomoyo_hooks[] __ro_after_init = {
>>> LSM_HOOK_INIT(cred_prepare, tomoyo_cred_prepare),
>>> LSM_HOOK_INIT(bprm_committed_creds, tomoyo_bprm_committed_creds),
>>> LSM_HOOK_INIT(task_alloc, tomoyo_task_alloc),
>>> @@ -569,7 +569,7 @@ static struct security_hook_list tomoyo_hooks[] __lsm_ro_after_init = {
>>> /* Lock for GC. */
>>> DEFINE_SRCU(tomoyo_ss);
>>> -int tomoyo_enabled __lsm_ro_after_init = 1;
>>> +int tomoyo_enabled __ro_after_init = 1;
>>> /**
>>> * tomoyo_init - Register TOMOYO Linux as a LSM module.
>>> @@ -578,10 +578,15 @@ int tomoyo_enabled __lsm_ro_after_init = 1;
>>> */
>>> static int __init tomoyo_init(void)
>>> {
>>> + struct security_hook_array hook_array = {
>>> + .hooks = tomoyo_hooks,
>>> + .count = ARRAY_SIZE(tomoyo_hooks),
>>> + .lsm = "tomoyo",
>>> + };
>>> struct tomoyo_task *s = tomoyo_task(current);
>>> /* register ourselves with the security framework */
>>> - security_add_hooks(tomoyo_hooks, ARRAY_SIZE(tomoyo_hooks), "tomoyo");
>>> + security_add_hook_array(&hook_array);
>>> pr_info("TOMOYO Linux initialized\n");
>>> s->domain_info = &tomoyo_kernel_domain;
>>> atomic_inc(&tomoyo_kernel_domain.users);
>>> diff --git a/security/yama/yama_lsm.c b/security/yama/yama_lsm.c
>>> index 94dc346370b1..ed752f6eafcf 100644
>>> --- a/security/yama/yama_lsm.c
>>> +++ b/security/yama/yama_lsm.c
>>> @@ -421,7 +421,7 @@ static int yama_ptrace_traceme(struct task_struct *parent)
>>> return rc;
>>> }
>>> -static struct security_hook_list yama_hooks[] __lsm_ro_after_init = {
>>> +static struct security_hook_list yama_hooks[] __ro_after_init = {
>>> LSM_HOOK_INIT(ptrace_access_check, yama_ptrace_access_check),
>>> LSM_HOOK_INIT(ptrace_traceme, yama_ptrace_traceme),
>>> LSM_HOOK_INIT(task_prctl, yama_task_prctl),
>>> @@ -476,8 +476,14 @@ static inline void yama_init_sysctl(void) { }
>>> static int __init yama_init(void)
>>> {
>>> + struct security_hook_array hook_array = {
>>> + .hooks = yama_hooks,
>>> + .count = ARRAY_SIZE(yama_hooks),
>>> + .lsm = "yama",
>>> + };
>>> +
>>> pr_info("Yama: becoming mindful.\n");
>>> - security_add_hooks(yama_hooks, ARRAY_SIZE(yama_hooks), "yama");
>>> + security_add_hook_array(&hook_array);
>>> yama_init_sysctl();
>>> return 0;
>>> }
>>>
^ permalink raw reply
* Re: [PATCH v1 0/3] Introduce CAP_SYS_PERFMON capability for secure Perf users groups
From: Casey Schaufler @ 2019-12-11 18:09 UTC (permalink / raw)
To: Alexey Budankov, Peter Zijlstra, Arnaldo Carvalho de Melo,
Ingo Molnar
Cc: Jiri Olsa, Andi Kleen, elena.reshetova, Alexander Shishkin,
Jann Horn, Kees Cook, Stephane Eranian, Namhyung Kim,
linux-security-module, selinux, linux-kernel, Casey Schaufler
In-Reply-To: <ab206ef5-466e-7bce-3e5f-53da110bddb2@linux.intel.com>
On 12/11/2019 2:52 AM, Alexey Budankov wrote:
> On 05.12.2019 20:33, Casey Schaufler wrote:
>> On 12/5/2019 9:05 AM, Alexey Budankov wrote:
>>> Hello Casey,
>>>
>>> On 05.12.2019 19:49, Casey Schaufler wrote:
>>>> On 12/5/2019 8:15 AM, Alexey Budankov wrote:
>>>>> Currently access to perf_events functionality [1] beyond the scope permitted
>>>>> by perf_event_paranoid [1] kernel setting is allowed to a privileged process
>>>>> [2] with CAP_SYS_ADMIN capability enabled in the process effective set [3].
>>>>>
>>>>> This patch set introduces CAP_SYS_PERFMON capability devoted to secure performance
>>>>> monitoring activity so that CAP_SYS_PERFMON would assist CAP_SYS_ADMIN in its
>>>>> governing role for perf_events based performance monitoring of a system.
>>>>>
>>>>> CAP_SYS_PERFMON aims to harden system security and integrity when monitoring
>>>>> performance using perf_events subsystem by processes and Perf privileged users
>>>>> [2], thus decreasing attack surface that is available to CAP_SYS_ADMIN
>>>>> privileged processes [3].
>>>> Are there use cases where you would need CAP_SYS_PERFMON where you
>>>> would not also need CAP_SYS_ADMIN? If you separate a new capability
>>> Actually, there are. Perf tool that has record, stat and top modes could run with
>>> CAP_SYS_PERFMON capability as mentioned below and provide system wide performance
>>> data. Currently for that to work the tool needs to be granted with CAP_SYS_ADMIN.
>> The question isn't whether the tool could use the capability, it's whether
>> the tool would also need CAP_SYS_ADMIN to be useful. Are there existing
>> tools that could stop using CAP_SYS_ADMIN in favor of CAP_SYS_PERFMON?
>> My bet is that any tool that does performance monitoring is going to need
>> CAP_SYS_ADMIN for other reasons.
>>
>>>> from CAP_SYS_ADMIN but always have to use CAP_SYS_ADMIN in conjunction
>>>> with the new capability it is all rather pointless.
>>>>
>>>> The scope you've defined for this CAP_SYS_PERFMON is very small.
>>>> Is there a larger set of privilege checks that might be applicable
>>>> for it?
>>> CAP_SYS_PERFMON could be applied broadly, though, this patch set enables record
>>> and stat mode use cases for system wide performance monitoring in kernel and
>>> user modes.
>> The granularity of capabilities is something we have to watch
>> very carefully. Sure, CAP_SYS_ADMIN covers a lot of things, but
>> if we broke it up "properly" we'd have hundreds of capabilities.
> Fully agree and this broader discussion is really helpful to come up with
> properly balanced solution.
>
>> If you want control that finely we have SELinux.
> Undoubtedly, SELinux is the powerful, mature, whole level of functionality that
> could provide benefits not only for perf_events subsystem. However perf_events
> is built around capabilities to provide access control to its functionality,
> thus perf_events would require considerable rework prior it could be controlled
> thru SELinux. Then the adoption could also require changes to the installed
> infrastructure just for the sake of adopting alternative access control mechanism.
>
> On the other hand there are currently already existing users and use cases that
> are built around the CAP_SYS_ADMIN based access control, and Perf tool, which is
> the native Linux kernel observability and performance profiling tool, provides
> means to operate in restricted multiuser environments(HPC clusters, cloud and
> virtual environments) for groups of unprivileged users under admins control [1].
>
> In this circumstances CAP_SYS_PERFMON looks like smart balanced advancement that
> trade-offs between perf_events subsystem extensions, required level of control
> and configurability of perf_events, existing users adoption effort, and it brings
> security hardening benefits of decreasing attack surface for the existing users
> and use cases.
I'm not 100% opposed to CAP_SYS_PERFMON. I am 100% opposed to new capabilities
that have a single use. Surely there are other CAP_SYS_ADMIN users that [cs]ould
be converted to CAP_SYS_PERFMON as well. If there is a class of system performance
privileged operations, say a dozen or so, you may have a viable argument.
>
> Well, yes, it is really good that Linux nowadays provides a handful of various
> security assuring mechanisms but proper balance is what usually makes valuable
> features happen and its users happy and moves forward.
>
> Gratefully,
> Alexey
>
> [1] https://www.kernel.org/doc/html/latest/admin-guide/perf-security.html
^ permalink raw reply
* Re: [PATCH v1 0/3] Introduce CAP_SYS_PERFMON capability for secure Perf users groups
From: Alexey Budankov @ 2019-12-11 17:00 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Casey Schaufler, Arnaldo Carvalho de Melo, Ingo Molnar, Jiri Olsa,
Andi Kleen, elena.reshetova, Alexander Shishkin, Jann Horn,
Kees Cook, Stephane Eranian, Namhyung Kim, linux-security-module,
selinux, linux-kernel
In-Reply-To: <20191211152435.GN2827@hirez.programming.kicks-ass.net>
On 11.12.2019 18:24, Peter Zijlstra wrote:
> On Wed, Dec 11, 2019 at 01:52:15PM +0300, Alexey Budankov wrote:
>> Undoubtedly, SELinux is the powerful, mature, whole level of functionality that
>> could provide benefits not only for perf_events subsystem. However perf_events
>> is built around capabilities to provide access control to its functionality,
>> thus perf_events would require considerable rework prior it could be controlled
>> thru SELinux.
>
> You mean this:
>
> da97e18458fb ("perf_event: Add support for LSM and SELinux checks")
>
> ?
Yes, I do.
This feature greatly adds up into MAC access control [1], [2] for perf_events,
additionally to already existing DAC [3]. However, there is still the whole
other part of MAC story on the user space side.
Fortunately MAC and DAC access control mechanisms designed in the way they are
naturally layered and coexist in the system so I don't see any contradiction
in advancing either mechanism to meet the demand of possible diverse use cases.
There is no much rationale in providing favor to one or the other mechanism
because together they constitute complete integrity of security access control
and configurability for diverse use cases of perf_events.
>
>> Then the adoption could also require changes to the installed
>> infrastructure just for the sake of adopting alternative access control mechanism.
>
> This is still very much true.
It is just enough to imaging some HPC cluster or Cloud lab with
several hundreds of nodes to be upgraded.
Thanks,
Alexey
[1] https://en.wikipedia.org/wiki/Security-Enhanced_Linux
[2] https://en.wikipedia.org/wiki/Mandatory_access_control
[3] https://en.wikipedia.org/wiki/Discretionary_access_control
^ permalink raw reply
* Re: [PATCH] LSM: allow an LSM to disable all hooks at once
From: Kees Cook @ 2019-12-11 16:42 UTC (permalink / raw)
To: Stephen Smalley
Cc: Ondrej Mosnacek, linux-security-module, James Morris,
Serge E. Hallyn, Casey Schaufler, selinux, Paul Moore,
John Johansen, Micah Morton, Tetsuo Handa
In-Reply-To: <677be2d4-8247-3c2b-ac13-def725cffaeb@tycho.nsa.gov>
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.
-Kees
>
> >
> > Fixes: b1d9e6b0646d ("LSM: Switch to lists of hooks")
> > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > ---
> >
> > This is an alternative to [1]. It turned out to be less simple than I
> > had hoped, but OTOH the hook arrays can now be unconditionally made
> > __ro_after_init so may be still worth it.
> >
> > Compile- and boot- tested with SECURITY_SELINUX_DISABLE set to =y and
> > =n; SELinux enabled. Changes to other LSMs only compile-tested (but
> > those are trivial).
> >
> > [1] https://lore.kernel.org/selinux/20191209075756.123157-1-omosnace@redhat.com/T/
> >
> > include/linux/lsm_hooks.h | 46 +++++++++----------------------
> > security/Kconfig | 5 ----
> > security/apparmor/lsm.c | 14 ++++++----
> > security/commoncap.c | 11 +++++---
> > security/loadpin/loadpin.c | 10 +++++--
> > security/lockdown/lockdown.c | 11 +++++---
> > security/safesetid/lsm.c | 9 +++++--
> > security/security.c | 52 +++++++++++++++++++++---------------
> > security/selinux/Kconfig | 5 ++--
> > security/selinux/hooks.c | 28 ++++++++++++++-----
> > security/smack/smack_lsm.c | 11 +++++---
> > security/tomoyo/tomoyo.c | 13 ++++++---
> > security/yama/yama_lsm.c | 10 +++++--
> > 13 files changed, 133 insertions(+), 92 deletions(-)
> >
> > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> > index 20d8cf194fb7..91b77ebcb822 100644
> > --- a/include/linux/lsm_hooks.h
> > +++ b/include/linux/lsm_hooks.h
> > @@ -27,7 +27,6 @@
> > #include <linux/security.h>
> > #include <linux/init.h>
> > -#include <linux/rculist.h>
> > /**
> > * union security_list_options - Linux Security Module hook function list
> > @@ -2086,6 +2085,9 @@ struct security_hook_list {
> > struct hlist_head *head;
> > union security_list_options hook;
> > char *lsm;
> > +#ifdef CONFIG_SECURITY_SELINUX_DISABLE
> > + const int *enabled;
> > +#endif
> > } __randomize_layout;
> > /*
> > @@ -2112,8 +2114,16 @@ struct lsm_blob_sizes {
> > extern struct security_hook_heads security_hook_heads;
> > extern char *lsm_names;
> > -extern void security_add_hooks(struct security_hook_list *hooks, int count,
> > - char *lsm);
> > +struct security_hook_array {
> > + struct security_hook_list *hooks;
> > + int count;
> > + char *lsm;
> > +#ifdef CONFIG_SECURITY_SELINUX_DISABLE
> > + const int *enabled;
> > +#endif
> > +};
> > +
> > +extern void security_add_hook_array(const struct security_hook_array *array);
> > #define LSM_FLAG_LEGACY_MAJOR BIT(0)
> > #define LSM_FLAG_EXCLUSIVE BIT(1)
> > @@ -2145,36 +2155,6 @@ extern struct lsm_info __start_early_lsm_info[], __end_early_lsm_info[];
> > __used __section(.early_lsm_info.init) \
> > __aligned(sizeof(unsigned long))
> > -#ifdef CONFIG_SECURITY_SELINUX_DISABLE
> > -/*
> > - * Assuring the safety of deleting a security module is up to
> > - * the security module involved. This may entail ordering the
> > - * module's hook list in a particular way, refusing to disable
> > - * the module once a policy is loaded or any number of other
> > - * actions better imagined than described.
> > - *
> > - * The name of the configuration option reflects the only module
> > - * that currently uses the mechanism. Any developer who thinks
> > - * disabling their module is a good idea needs to be at least as
> > - * careful as the SELinux team.
> > - */
> > -static inline void security_delete_hooks(struct security_hook_list *hooks,
> > - int count)
> > -{
> > - int i;
> > -
> > - for (i = 0; i < count; i++)
> > - hlist_del_rcu(&hooks[i].list);
> > -}
> > -#endif /* CONFIG_SECURITY_SELINUX_DISABLE */
> > -
> > -/* Currently required to handle SELinux runtime hook disable. */
> > -#ifdef CONFIG_SECURITY_WRITABLE_HOOKS
> > -#define __lsm_ro_after_init
> > -#else
> > -#define __lsm_ro_after_init __ro_after_init
> > -#endif /* CONFIG_SECURITY_WRITABLE_HOOKS */
> > -
> > extern int lsm_inode_alloc(struct inode *inode);
> > #endif /* ! __LINUX_LSM_HOOKS_H */
> > diff --git a/security/Kconfig b/security/Kconfig
> > index 2a1a2d396228..456764990a13 100644
> > --- a/security/Kconfig
> > +++ b/security/Kconfig
> > @@ -32,11 +32,6 @@ config SECURITY
> > If you are unsure how to answer this question, answer N.
> > -config SECURITY_WRITABLE_HOOKS
> > - depends on SECURITY
> > - bool
> > - default n
> > -
> > config SECURITYFS
> > bool "Enable the securityfs filesystem"
> > help
> > diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> > index b621ad74f54a..a27f48670b37 100644
> > --- a/security/apparmor/lsm.c
> > +++ b/security/apparmor/lsm.c
> > @@ -1158,13 +1158,13 @@ static int apparmor_inet_conn_request(struct sock *sk, struct sk_buff *skb,
> > /*
> > * The cred blob is a pointer to, not an instance of, an aa_task_ctx.
> > */
> > -struct lsm_blob_sizes apparmor_blob_sizes __lsm_ro_after_init = {
> > +struct lsm_blob_sizes apparmor_blob_sizes __ro_after_init = {
> > .lbs_cred = sizeof(struct aa_task_ctx *),
> > .lbs_file = sizeof(struct aa_file_ctx),
> > .lbs_task = sizeof(struct aa_task_ctx),
> > };
> > -static struct security_hook_list apparmor_hooks[] __lsm_ro_after_init = {
> > +static struct security_hook_list apparmor_hooks[] __ro_after_init = {
> > LSM_HOOK_INIT(ptrace_access_check, apparmor_ptrace_access_check),
> > LSM_HOOK_INIT(ptrace_traceme, apparmor_ptrace_traceme),
> > LSM_HOOK_INIT(capget, apparmor_capget),
> > @@ -1368,7 +1368,7 @@ static const struct kernel_param_ops param_ops_aaintbool = {
> > .get = param_get_aaintbool
> > };
> > /* Boot time disable flag */
> > -static int apparmor_enabled __lsm_ro_after_init = 1;
> > +static int apparmor_enabled __ro_after_init = 1;
> > module_param_named(enabled, apparmor_enabled, aaintbool, 0444);
> > static int __init apparmor_enabled_setup(char *str)
> > @@ -1829,6 +1829,11 @@ __initcall(apparmor_nf_ip_init);
> > static int __init apparmor_init(void)
> > {
> > + struct security_hook_array hook_array = {
> > + .hooks = apparmor_hooks,
> > + .count = ARRAY_SIZE(apparmor_hooks),
> > + .lsm = "apparmor",
> > + };
> > int error;
> > aa_secids_init();
> > @@ -1864,8 +1869,7 @@ static int __init apparmor_init(void)
> > aa_free_root_ns();
> > goto buffers_out;
> > }
> > - security_add_hooks(apparmor_hooks, ARRAY_SIZE(apparmor_hooks),
> > - "apparmor");
> > + security_add_hook_array(&hook_array);
> > /* Report that AppArmor successfully initialized */
> > apparmor_initialized = 1;
> > diff --git a/security/commoncap.c b/security/commoncap.c
> > index f4ee0ae106b2..6e9f4b6b6b1d 100644
> > --- a/security/commoncap.c
> > +++ b/security/commoncap.c
> > @@ -1339,7 +1339,7 @@ int cap_mmap_file(struct file *file, unsigned long reqprot,
> > #ifdef CONFIG_SECURITY
> > -static struct security_hook_list capability_hooks[] __lsm_ro_after_init = {
> > +static struct security_hook_list capability_hooks[] __ro_after_init = {
> > LSM_HOOK_INIT(capable, cap_capable),
> > LSM_HOOK_INIT(settime, cap_settime),
> > LSM_HOOK_INIT(ptrace_access_check, cap_ptrace_access_check),
> > @@ -1362,8 +1362,13 @@ static struct security_hook_list capability_hooks[] __lsm_ro_after_init = {
> > static int __init capability_init(void)
> > {
> > - security_add_hooks(capability_hooks, ARRAY_SIZE(capability_hooks),
> > - "capability");
> > + struct security_hook_array hook_array = {
> > + .hooks = capability_hooks,
> > + .count = ARRAY_SIZE(capability_hooks),
> > + .lsm = "capability",
> > + };
> > +
> > + security_add_hook_array(&hook_array);
> > return 0;
> > }
> > diff --git a/security/loadpin/loadpin.c b/security/loadpin/loadpin.c
> > index ee5cb944f4ad..5fadd4969d90 100644
> > --- a/security/loadpin/loadpin.c
> > +++ b/security/loadpin/loadpin.c
> > @@ -180,7 +180,7 @@ static int loadpin_load_data(enum kernel_load_data_id id)
> > return loadpin_read_file(NULL, (enum kernel_read_file_id) id);
> > }
> > -static struct security_hook_list loadpin_hooks[] __lsm_ro_after_init = {
> > +static struct security_hook_list loadpin_hooks[] __ro_after_init = {
> > LSM_HOOK_INIT(sb_free_security, loadpin_sb_free_security),
> > LSM_HOOK_INIT(kernel_read_file, loadpin_read_file),
> > LSM_HOOK_INIT(kernel_load_data, loadpin_load_data),
> > @@ -224,10 +224,16 @@ static void __init parse_exclude(void)
> > static int __init loadpin_init(void)
> > {
> > + struct security_hook_array hook_array = {
> > + .hooks = loadpin_hooks,
> > + .count = ARRAY_SIZE(loadpin_hooks),
> > + .lsm = "loadpin",
> > + };
> > +
> > pr_info("ready to pin (currently %senforcing)\n",
> > enforce ? "" : "not ");
> > parse_exclude();
> > - security_add_hooks(loadpin_hooks, ARRAY_SIZE(loadpin_hooks), "loadpin");
> > + security_add_hook_array(&hook_array);
> > return 0;
> > }
> > diff --git a/security/lockdown/lockdown.c b/security/lockdown/lockdown.c
> > index 5a952617a0eb..bcfa0ff4ee63 100644
> > --- a/security/lockdown/lockdown.c
> > +++ b/security/lockdown/lockdown.c
> > @@ -71,19 +71,24 @@ static int lockdown_is_locked_down(enum lockdown_reason what)
> > return 0;
> > }
> > -static struct security_hook_list lockdown_hooks[] __lsm_ro_after_init = {
> > +static struct security_hook_list lockdown_hooks[] __ro_after_init = {
> > LSM_HOOK_INIT(locked_down, lockdown_is_locked_down),
> > };
> > static int __init lockdown_lsm_init(void)
> > {
> > + struct security_hook_array hook_array = {
> > + .hooks = lockdown_hooks,
> > + .count = ARRAY_SIZE(lockdown_hooks),
> > + .lsm = "lockdown",
> > + };
> > +
> > #if defined(CONFIG_LOCK_DOWN_KERNEL_FORCE_INTEGRITY)
> > lock_kernel_down("Kernel configuration", LOCKDOWN_INTEGRITY_MAX);
> > #elif defined(CONFIG_LOCK_DOWN_KERNEL_FORCE_CONFIDENTIALITY)
> > lock_kernel_down("Kernel configuration", LOCKDOWN_CONFIDENTIALITY_MAX);
> > #endif
> > - security_add_hooks(lockdown_hooks, ARRAY_SIZE(lockdown_hooks),
> > - "lockdown");
> > + security_add_hook_array(&hook_array);
> > return 0;
> > }
> > diff --git a/security/safesetid/lsm.c b/security/safesetid/lsm.c
> > index 7760019ad35d..4e36e53f033d 100644
> > --- a/security/safesetid/lsm.c
> > +++ b/security/safesetid/lsm.c
> > @@ -156,8 +156,13 @@ static struct security_hook_list safesetid_security_hooks[] = {
> > static int __init safesetid_security_init(void)
> > {
> > - security_add_hooks(safesetid_security_hooks,
> > - ARRAY_SIZE(safesetid_security_hooks), "safesetid");
> > + struct security_hook_array hook_array = {
> > + .hooks = safesetid_security_hooks,
> > + .count = ARRAY_SIZE(safesetid_security_hooks),
> > + .lsm = "safesetid",
> > + };
> > +
> > + security_add_hook_array(&hook_array);
> > /* Report that SafeSetID successfully initialized */
> > safesetid_initialized = 1;
> > diff --git a/security/security.c b/security/security.c
> > index 2b5473d92416..a5dd348bd37e 100644
> > --- a/security/security.c
> > +++ b/security/security.c
> > @@ -68,14 +68,14 @@ const char *const lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1] = {
> > [LOCKDOWN_CONFIDENTIALITY_MAX] = "confidentiality",
> > };
> > -struct security_hook_heads security_hook_heads __lsm_ro_after_init;
> > +struct security_hook_heads security_hook_heads __ro_after_init;
> > static BLOCKING_NOTIFIER_HEAD(blocking_lsm_notifier_chain);
> > static struct kmem_cache *lsm_file_cache;
> > static struct kmem_cache *lsm_inode_cache;
> > char *lsm_names;
> > -static struct lsm_blob_sizes blob_sizes __lsm_ro_after_init;
> > +static struct lsm_blob_sizes blob_sizes __ro_after_init;
> > /* Boot-time LSM user choice */
> > static __initdata const char *chosen_lsm_order;
> > @@ -467,20 +467,20 @@ static int lsm_append(const char *new, char **result)
> > /**
> > * security_add_hooks - Add a modules hooks to the hook lists.
> > - * @hooks: the hooks to add
> > - * @count: the number of hooks to add
> > - * @lsm: the name of the security module
> > + * @array: the struct describing hooks to add
> > *
> > * Each LSM has to register its hooks with the infrastructure.
> > */
> > -void __init security_add_hooks(struct security_hook_list *hooks, int count,
> > - char *lsm)
> > +void __init security_add_hook_array(const struct security_hook_array *array)
> > {
> > int i;
> > - for (i = 0; i < count; i++) {
> > - hooks[i].lsm = lsm;
> > - hlist_add_tail_rcu(&hooks[i].list, hooks[i].head);
> > + for (i = 0; i < array->count; i++) {
> > + array->hooks[i].lsm = array->lsm;
> > +#ifdef CONFIG_SECURITY_SELINUX_DISABLE
> > + array->hooks[i].enabled = array->enabled;
> > +#endif
> > + hlist_add_tail_rcu(&array->hooks[i].list, array->hooks[i].head);
> > }
> > /*
> > @@ -488,7 +488,7 @@ void __init security_add_hooks(struct security_hook_list *hooks, int count,
> > * and fix this up afterwards.
> > */
> > if (slab_is_available()) {
> > - if (lsm_append(lsm, &lsm_names) < 0)
> > + if (lsm_append(array->lsm, &lsm_names) < 0)
> > panic("%s - Cannot get early memory.\n", __func__);
> > }
> > }
> > @@ -679,11 +679,22 @@ static void __init lsm_early_task(struct task_struct *task)
> > * This is a hook that returns a value.
> > */
> > +#define for_each_hook(p, func) \
> > + hlist_for_each_entry(p, &security_hook_heads.func, list)
> > +
> > +#ifdef CONFIG_SECURITY_SELINUX_DISABLE
> > +#define for_each_enabled_hook(p, func) \
> > + for_each_hook(p, func) \
> > + if (!(p)->enabled || READ_ONCE(*(p)->enabled))
> > +#else
> > +#define for_each_enabled_hook for_each_hook
> > +#endif
> > +
> > #define call_void_hook(FUNC, ...) \
> > do { \
> > struct security_hook_list *P; \
> > \
> > - hlist_for_each_entry(P, &security_hook_heads.FUNC, list) \
> > + for_each_enabled_hook(P, FUNC) \
> > P->hook.FUNC(__VA_ARGS__); \
> > } while (0)
> > @@ -692,7 +703,7 @@ static void __init lsm_early_task(struct task_struct *task)
> > do { \
> > struct security_hook_list *P; \
> > \
> > - hlist_for_each_entry(P, &security_hook_heads.FUNC, list) { \
> > + for_each_enabled_hook(P, FUNC) { \
> > RC = P->hook.FUNC(__VA_ARGS__); \
> > if (RC != 0) \
> > break; \
> > @@ -795,7 +806,7 @@ int security_vm_enough_memory_mm(struct mm_struct *mm, long pages)
> > * agree that it should be set it will. If any module
> > * thinks it should not be set it won't.
> > */
> > - hlist_for_each_entry(hp, &security_hook_heads.vm_enough_memory, list) {
> > + for_each_enabled_hook(hp, vm_enough_memory) {
> > rc = hp->hook.vm_enough_memory(mm, pages);
> > if (rc <= 0) {
> > cap_sys_admin = 0;
> > @@ -1343,7 +1354,7 @@ int security_inode_getsecurity(struct inode *inode, const char *name, void **buf
> > /*
> > * Only one module will provide an attribute with a given name.
> > */
> > - hlist_for_each_entry(hp, &security_hook_heads.inode_getsecurity, list) {
> > + for_each_enabled_hook(hp, inode_getsecurity) {
> > rc = hp->hook.inode_getsecurity(inode, name, buffer, alloc);
> > if (rc != -EOPNOTSUPP)
> > return rc;
> > @@ -1361,7 +1372,7 @@ int security_inode_setsecurity(struct inode *inode, const char *name, const void
> > /*
> > * Only one module will provide an attribute with a given name.
> > */
> > - hlist_for_each_entry(hp, &security_hook_heads.inode_setsecurity, list) {
> > + for_each_enabled_hook(hp, inode_setsecurity) {
> > rc = hp->hook.inode_setsecurity(inode, name, value, size,
> > flags);
> > if (rc != -EOPNOTSUPP)
> > @@ -1744,7 +1755,7 @@ int security_task_prctl(int option, unsigned long arg2, unsigned long arg3,
> > int rc = -ENOSYS;
> > struct security_hook_list *hp;
> > - hlist_for_each_entry(hp, &security_hook_heads.task_prctl, list) {
> > + for_each_enabled_hook(hp, task_prctl) {
> > thisrc = hp->hook.task_prctl(option, arg2, arg3, arg4, arg5);
> > if (thisrc != -ENOSYS) {
> > rc = thisrc;
> > @@ -1913,7 +1924,7 @@ int security_getprocattr(struct task_struct *p, const char *lsm, char *name,
> > {
> > struct security_hook_list *hp;
> > - hlist_for_each_entry(hp, &security_hook_heads.getprocattr, list) {
> > + for_each_enabled_hook(hp, getprocattr) {
> > if (lsm != NULL && strcmp(lsm, hp->lsm))
> > continue;
> > return hp->hook.getprocattr(p, name, value);
> > @@ -1926,7 +1937,7 @@ int security_setprocattr(const char *lsm, const char *name, void *value,
> > {
> > struct security_hook_list *hp;
> > - hlist_for_each_entry(hp, &security_hook_heads.setprocattr, list) {
> > + for_each_enabled_hook(hp, setprocattr) {
> > if (lsm != NULL && strcmp(lsm, hp->lsm))
> > continue;
> > return hp->hook.setprocattr(name, value, size);
> > @@ -2327,8 +2338,7 @@ int security_xfrm_state_pol_flow_match(struct xfrm_state *x,
> > * For speed optimization, we explicitly break the loop rather than
> > * using the macro
> > */
> > - hlist_for_each_entry(hp, &security_hook_heads.xfrm_state_pol_flow_match,
> > - list) {
> > + for_each_enabled_hook(hp, xfrm_state_pol_flow_match) {
> > rc = hp->hook.xfrm_state_pol_flow_match(x, xp, fl);
> > break;
> > }
> > diff --git a/security/selinux/Kconfig b/security/selinux/Kconfig
> > index 996d35d950f7..f64290de1f8a 100644
> > --- a/security/selinux/Kconfig
> > +++ b/security/selinux/Kconfig
> > @@ -26,7 +26,6 @@ config SECURITY_SELINUX_BOOTPARAM
> > config SECURITY_SELINUX_DISABLE
> > bool "NSA SELinux runtime disable"
> > depends on SECURITY_SELINUX
> > - select SECURITY_WRITABLE_HOOKS
> > default n
> > help
> > This option enables writing to a selinuxfs node 'disable', which
> > @@ -37,8 +36,8 @@ config SECURITY_SELINUX_DISABLE
> > portability across platforms where boot parameters are difficult
> > to employ.
> > - NOTE: selecting this option will disable the '__ro_after_init'
> > - kernel hardening feature for security hooks. Please consider
> > + NOTE: Selecting this option might cause memory leaks and random
> > + crashes when the runtime disable is used. Please consider
> > using the selinux=0 boot parameter instead of enabling this
> > option.
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 47626342b6e5..b76acd98dda5 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -123,7 +123,13 @@ __setup("enforcing=", enforcing_setup);
> > #define selinux_enforcing_boot 1
> > #endif
> > -int selinux_enabled __lsm_ro_after_init = 1;
> > +/* Currently required to handle SELinux runtime hook disable. */
> > +#ifdef CONFIG_SECURITY_SELINUX_DISABLE
> > +int selinux_enabled = 1;
> > +#else
> > +int selinux_enabled __ro_after_init = 1;
> > +#endif /* CONFIG_SECURITY_SELINUX_DISABLE */
> > +
> > #ifdef CONFIG_SECURITY_SELINUX_BOOTPARAM
> > static int __init selinux_enabled_setup(char *str)
> > {
> > @@ -6827,7 +6833,7 @@ static int selinux_lockdown(enum lockdown_reason what)
> > LOCKDOWN__CONFIDENTIALITY, &ad);
> > }
> > -struct lsm_blob_sizes selinux_blob_sizes __lsm_ro_after_init = {
> > +struct lsm_blob_sizes selinux_blob_sizes __ro_after_init = {
> > .lbs_cred = sizeof(struct task_security_struct),
> > .lbs_file = sizeof(struct file_security_struct),
> > .lbs_inode = sizeof(struct inode_security_struct),
> > @@ -6896,7 +6902,7 @@ static int selinux_perf_event_write(struct perf_event *event)
> > }
> > #endif
> > -static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
> > +static struct security_hook_list selinux_hooks[] __ro_after_init = {
> > LSM_HOOK_INIT(binder_set_context_mgr, selinux_binder_set_context_mgr),
> > LSM_HOOK_INIT(binder_transaction, selinux_binder_transaction),
> > LSM_HOOK_INIT(binder_transfer_binder, selinux_binder_transfer_binder),
> > @@ -7145,6 +7151,15 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
> > static __init int selinux_init(void)
> > {
> > + struct security_hook_array hook_array = {
> > + .hooks = selinux_hooks,
> > + .count = ARRAY_SIZE(selinux_hooks),
> > + .lsm = "selinux",
> > +#ifdef CONFIG_SECURITY_SELINUX_DISABLE
> > + .enabled = &selinux_enabled,
> > +#endif
> > + };
> > +
> > pr_info("SELinux: Initializing.\n");
> > memset(&selinux_state, 0, sizeof(selinux_state));
> > @@ -7166,7 +7181,7 @@ static __init int selinux_init(void)
> > hashtab_cache_init();
> > - security_add_hooks(selinux_hooks, ARRAY_SIZE(selinux_hooks), "selinux");
> > + security_add_hook_array(&hook_array);
> > if (avc_add_callback(selinux_netcache_avc_callback, AVC_CALLBACK_RESET))
> > panic("SELinux: Unable to register AVC netcache callback\n");
> > @@ -7319,9 +7334,10 @@ int selinux_disable(struct selinux_state *state)
> > pr_info("SELinux: Disabled at runtime.\n");
> > - selinux_enabled = 0;
> > + /* Unregister netfilter hooks. */
> > + selinux_nf_ip_exit();
> > - security_delete_hooks(selinux_hooks, ARRAY_SIZE(selinux_hooks));
> > + WRITE_ONCE(selinux_enabled, 0);
> > /* Try to destroy the avc node cache */
> > avc_disable();
> > diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> > index ecea41ce919b..c21dda12bc4b 100644
> > --- a/security/smack/smack_lsm.c
> > +++ b/security/smack/smack_lsm.c
> > @@ -4583,7 +4583,7 @@ static int smack_dentry_create_files_as(struct dentry *dentry, int mode,
> > return 0;
> > }
> > -struct lsm_blob_sizes smack_blob_sizes __lsm_ro_after_init = {
> > +struct lsm_blob_sizes smack_blob_sizes __ro_after_init = {
> > .lbs_cred = sizeof(struct task_smack),
> > .lbs_file = sizeof(struct smack_known *),
> > .lbs_inode = sizeof(struct inode_smack),
> > @@ -4591,7 +4591,7 @@ struct lsm_blob_sizes smack_blob_sizes __lsm_ro_after_init = {
> > .lbs_msg_msg = sizeof(struct smack_known *),
> > };
> > -static struct security_hook_list smack_hooks[] __lsm_ro_after_init = {
> > +static struct security_hook_list smack_hooks[] __ro_after_init = {
> > LSM_HOOK_INIT(ptrace_access_check, smack_ptrace_access_check),
> > LSM_HOOK_INIT(ptrace_traceme, smack_ptrace_traceme),
> > LSM_HOOK_INIT(syslog, smack_syslog),
> > @@ -4765,6 +4765,11 @@ static __init void init_smack_known_list(void)
> > */
> > static __init int smack_init(void)
> > {
> > + struct security_hook_array hook_array = {
> > + .hooks = smack_hooks,
> > + .count = ARRAY_SIZE(smack_hooks),
> > + .lsm = "smack",
> > + };
> > struct cred *cred = (struct cred *) current->cred;
> > struct task_smack *tsp;
> > @@ -4787,7 +4792,7 @@ static __init int smack_init(void)
> > /*
> > * Register with LSM
> > */
> > - security_add_hooks(smack_hooks, ARRAY_SIZE(smack_hooks), "smack");
> > + security_add_hook_array(&hook_array);
> > smack_enabled = 1;
> > pr_info("Smack: Initializing.\n");
> > diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
> > index 716c92ec941a..a4075379246d 100644
> > --- a/security/tomoyo/tomoyo.c
> > +++ b/security/tomoyo/tomoyo.c
> > @@ -486,7 +486,7 @@ static int tomoyo_socket_sendmsg(struct socket *sock, struct msghdr *msg,
> > return tomoyo_socket_sendmsg_permission(sock, msg, size);
> > }
> > -struct lsm_blob_sizes tomoyo_blob_sizes __lsm_ro_after_init = {
> > +struct lsm_blob_sizes tomoyo_blob_sizes __ro_after_init = {
> > .lbs_task = sizeof(struct tomoyo_task),
> > };
> > @@ -533,7 +533,7 @@ static void tomoyo_task_free(struct task_struct *task)
> > * tomoyo_security_ops is a "struct security_operations" which is used for
> > * registering TOMOYO.
> > */
> > -static struct security_hook_list tomoyo_hooks[] __lsm_ro_after_init = {
> > +static struct security_hook_list tomoyo_hooks[] __ro_after_init = {
> > LSM_HOOK_INIT(cred_prepare, tomoyo_cred_prepare),
> > LSM_HOOK_INIT(bprm_committed_creds, tomoyo_bprm_committed_creds),
> > LSM_HOOK_INIT(task_alloc, tomoyo_task_alloc),
> > @@ -569,7 +569,7 @@ static struct security_hook_list tomoyo_hooks[] __lsm_ro_after_init = {
> > /* Lock for GC. */
> > DEFINE_SRCU(tomoyo_ss);
> > -int tomoyo_enabled __lsm_ro_after_init = 1;
> > +int tomoyo_enabled __ro_after_init = 1;
> > /**
> > * tomoyo_init - Register TOMOYO Linux as a LSM module.
> > @@ -578,10 +578,15 @@ int tomoyo_enabled __lsm_ro_after_init = 1;
> > */
> > static int __init tomoyo_init(void)
> > {
> > + struct security_hook_array hook_array = {
> > + .hooks = tomoyo_hooks,
> > + .count = ARRAY_SIZE(tomoyo_hooks),
> > + .lsm = "tomoyo",
> > + };
> > struct tomoyo_task *s = tomoyo_task(current);
> > /* register ourselves with the security framework */
> > - security_add_hooks(tomoyo_hooks, ARRAY_SIZE(tomoyo_hooks), "tomoyo");
> > + security_add_hook_array(&hook_array);
> > pr_info("TOMOYO Linux initialized\n");
> > s->domain_info = &tomoyo_kernel_domain;
> > atomic_inc(&tomoyo_kernel_domain.users);
> > diff --git a/security/yama/yama_lsm.c b/security/yama/yama_lsm.c
> > index 94dc346370b1..ed752f6eafcf 100644
> > --- a/security/yama/yama_lsm.c
> > +++ b/security/yama/yama_lsm.c
> > @@ -421,7 +421,7 @@ static int yama_ptrace_traceme(struct task_struct *parent)
> > return rc;
> > }
> > -static struct security_hook_list yama_hooks[] __lsm_ro_after_init = {
> > +static struct security_hook_list yama_hooks[] __ro_after_init = {
> > LSM_HOOK_INIT(ptrace_access_check, yama_ptrace_access_check),
> > LSM_HOOK_INIT(ptrace_traceme, yama_ptrace_traceme),
> > LSM_HOOK_INIT(task_prctl, yama_task_prctl),
> > @@ -476,8 +476,14 @@ static inline void yama_init_sysctl(void) { }
> > static int __init yama_init(void)
> > {
> > + struct security_hook_array hook_array = {
> > + .hooks = yama_hooks,
> > + .count = ARRAY_SIZE(yama_hooks),
> > + .lsm = "yama",
> > + };
> > +
> > pr_info("Yama: becoming mindful.\n");
> > - security_add_hooks(yama_hooks, ARRAY_SIZE(yama_hooks), "yama");
> > + security_add_hook_array(&hook_array);
> > yama_init_sysctl();
> > return 0;
> > }
> >
>
--
Kees Cook
^ permalink raw reply
* [PATCH AUTOSEL 5.4 108/134] apparmor: fix unsigned len comparison with less than zero
From: Sasha Levin @ 2019-12-11 15:11 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Colin Ian King, John Johansen, Sasha Levin, linux-security-module
In-Reply-To: <20191211151150.19073-1-sashal@kernel.org>
From: Colin Ian King <colin.king@canonical.com>
[ Upstream commit 00e0590dbaec6f1bcaa36a85467d7e3497ced522 ]
The sanity check in macro update_for_len checks to see if len
is less than zero, however, len is a size_t so it can never be
less than zero, so this sanity check is a no-op. Fix this by
making len a ssize_t so the comparison will work and add ulen
that is a size_t copy of len so that the min() macro won't
throw warnings about comparing different types.
Addresses-Coverity: ("Macro compares unsigned to 0")
Fixes: f1bd904175e8 ("apparmor: add the base fns() for domain labels")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
Signed-off-by: John Johansen <john.johansen@canonical.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
security/apparmor/label.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/security/apparmor/label.c b/security/apparmor/label.c
index 59f1cc2557a73..470693239e64f 100644
--- a/security/apparmor/label.c
+++ b/security/apparmor/label.c
@@ -1458,11 +1458,13 @@ static inline bool use_label_hname(struct aa_ns *ns, struct aa_label *label,
/* helper macro for snprint routines */
#define update_for_len(total, len, size, str) \
do { \
+ size_t ulen = len; \
+ \
AA_BUG(len < 0); \
- total += len; \
- len = min(len, size); \
- size -= len; \
- str += len; \
+ total += ulen; \
+ ulen = min(ulen, size); \
+ size -= ulen; \
+ str += ulen; \
} while (0)
/**
@@ -1597,7 +1599,7 @@ int aa_label_snxprint(char *str, size_t size, struct aa_ns *ns,
struct aa_ns *prev_ns = NULL;
struct label_it i;
int count = 0, total = 0;
- size_t len;
+ ssize_t len;
AA_BUG(!str && size != 0);
AA_BUG(!label);
--
2.20.1
^ permalink raw reply related
* Re: Looks like issue in handling active_nodes count in 4.19 kernel .
From: Stephen Smalley @ 2019-12-11 15:53 UTC (permalink / raw)
To: rsiddoji, selinux; +Cc: paul, linux-security-module
In-Reply-To: <0101016ef59a2152-41e65aac-8784-4401-b20d-45b2852872d4-000000@us-west-2.amazonses.com>
On 12/11/19 10:35 AM, rsiddoji@codeaurora.org wrote:
> Thanks for tacking the patch fwd . On the question :
>
> Actually issue started when we were seeing most of the time "avc_reclaim_node" in the stack .
> Which on debugging further avc_cache.active_nodes was already in 7K+ nodes and as the logic is
>
> As below .
> if (atomic_inc_return(&avc->avc_cache.active_nodes) > avc->avc_cache_threshold)
> avc_reclaim_node(avc);
>
> So if the active_nodes count is > 512 (if not configured) we will be always be calling avc_reclaim_node() and eventually for each node insert we will be calling avc_reclaim_node and might be expansive then using
> cache and advantage of cache might be null and void due to this overhead?
Was this on a system with the default avc_cache_threshold value or was
it set higher by the distro/user?
If it was still 512 or any value significantly less than 7K, then the
bug is that it ever reached 7K in the first place. The first bug should
only trigger under severe memory pressure. The other potential reason
for growing numbers of active nodes would be cache thrashing leading to
avc_reclaim_node() being unable to take the lock on any buckets and
therefore unable to release nodes.
Possibly you need a larger cache threshold set on this system. It can
be set via /sys/fs/selinux/avc/cache_threshold.
Allowing AVC_CACHE_RECLAIM to also be set via selinuxfs or computed
relative to avc_cache_threshold would make sense as a further improvement.
>
> Thanks ,
> Ravi
>
> -----Original Message-----
> From: selinux-owner@vger.kernel.org <selinux-owner@vger.kernel.org> On Behalf Of Stephen Smalley
> Sent: Wednesday, December 11, 2019 8:18 PM
> To: rsiddoji@codeaurora.org; selinux@vger.kernel.org
> Cc: paul@paul-moore.com; linux-security-module@vger.kernel.org
> Subject: Re: Looks like issue in handling active_nodes count in 4.19 kernel .
>
> On 12/11/19 9:37 AM, Stephen Smalley wrote:
>> On 12/9/19 1:30 PM, rsiddoji@codeaurora.org wrote:
>>> Thanks for quick response , yes it will be helpful if you can raise
>>> the change .
>>> On the second issue in avc_alloc_node we are trying to check the
>>> slot status as active_nodes > 512 ( default ) Where checking
>>> the occupancy should be corrected as active_nodes
>>>> 80% of slots occupied or 16*512 or
>>> May be we need to use a different logic .
>>
>> Are you seeing an actual problem with this in practice, and if so,
>> what exactly is it that you are seeing and do you have a reproducer?
>
> BTW, on Linux distributions, there is an avcstat(8) utility that can be used to monitor the AVC statistics, or you can directly read the stats from the kernel via /sys/fs/selinux/avc/cache_stats
>
>>
>>>
>>>> /*@ static struct avc_node *avc_alloc_node(struct selinux_avc *avc)
>>>> */
>>>>
>>>> if (atomic_inc_return(&avc->avc_cache.active_nodes) >
>>>> avc->avc_cache_threshold) // default threshold is
>>>> 512
>>>> avc_reclaim_node(avc);
>>>>
>>>
>>> Regards,
>>> Ravi
>>>
>>> -----Original Message-----
>>> From: selinux-owner@vger.kernel.org <selinux-owner@vger.kernel.org>
>>> On Behalf Of Stephen Smalley
>>> Sent: Monday, December 9, 2019 11:35 PM
>>> To: rsiddoji@codeaurora.org; selinux@vger.kernel.org
>>> Cc: paul@paul-moore.com; linux-security-module@vger.kernel.org
>>> Subject: Re: Looks like issue in handling active_nodes count in 4.19
>>> kernel .
>>>
>>> On 12/9/19 10:55 AM, rsiddoji@codeaurora.org wrote:
>>>> Hi team ,
>>>> Looks like we have issue in handling the "active_nodes" count in
>>>> the Selinux - avc.c file.
>>>> Where avc_cache.active_nodes increase more than slot array and
>>>> code frequency calling of avc_reclaim_node() from avc_alloc_node()
>>>> ;
>>>>
>>>> Where following are the 2 instance which seem to possible culprits
>>>> which are seen on 4.19 kernel . Can you comment if my understand is
>>>> wrong.
>>>>
>>>>
>>>> #1. if we see the active_nodes count is incremented in
>>>> avc_alloc_node
>>>> (avc) which is called in avc_insert() Where if the code take
>>>> failure path on avc_xperms_populate the code will not decrement
>>>> this counter .
>>>>
>>>>
>>>> static struct avc_node *avc_insert(struct selinux_avc *avc,
>>>> u32 ssid, u32 tsid, u16 tclass,
>>>> struct av_decision *avd, ....
>>>> node = avc_alloc_node(avc); //incremented here ....
>>>> rc = avc_xperms_populate(node, xp_node); //
>>>> possibilities of this getting failure is there .
>>>> if (rc) {
>>>> kmem_cache_free(avc_node_cachep, node); // but on
>>>> failure we are not decrementing active_nodes ?
>>>> return NULL;
>>>> }
>>>
>>> I think you are correct; we should perhaps be calling avc_node_kill()
>>> here as we do in an earlier error path?
>>>
>>>>
>>>> #2. where it looks like the logic on comparing the active_nodes
>>>> against avc_cache_threshold seems wired as the count of active
>>>> nodes is always going to be
>>>> more than 512 will may land in simply removing /calling
>>>> avc_reclaim_node frequently much before the slots are full maybe we
>>>> are not using cache at best ?
>>>> we should be comparing with some high watermark ? or my
>>>> understanding wrong ?
>>>> /*@ static struct avc_node *avc_alloc_node(struct selinux_avc *avc)
>>>> */
>>>>
>>>> if (atomic_inc_return(&avc->avc_cache.active_nodes) >
>>>> avc->avc_cache_threshold) // default threshold is
>>>> 512
>>>> avc_reclaim_node(avc);
>>>>
>>>
>>> Not entirely sure what you are asking here. avc_reclaim_node()
>>> should reclaim multiple nodes up to AVC_CACHE_RECLAIM. Possibly that
>>> should be configurable via selinuxfs too, and/or calculated from
>>> avc_cache_threshold in some way?
>>>
>>> Were you interested in creating a patch to fix the first issue above
>>> or looking to us to do so?
>>>
>>>
>>>
>>
>
>
^ permalink raw reply
* Re: [PATCH v1 0/3] Introduce CAP_SYS_PERFMON capability for secure Perf users groups
From: Peter Zijlstra @ 2019-12-11 15:24 UTC (permalink / raw)
To: Alexey Budankov
Cc: Casey Schaufler, Arnaldo Carvalho de Melo, Ingo Molnar, Jiri Olsa,
Andi Kleen, elena.reshetova, Alexander Shishkin, Jann Horn,
Kees Cook, Stephane Eranian, Namhyung Kim, linux-security-module,
selinux, linux-kernel
In-Reply-To: <ab206ef5-466e-7bce-3e5f-53da110bddb2@linux.intel.com>
On Wed, Dec 11, 2019 at 01:52:15PM +0300, Alexey Budankov wrote:
> Undoubtedly, SELinux is the powerful, mature, whole level of functionality that
> could provide benefits not only for perf_events subsystem. However perf_events
> is built around capabilities to provide access control to its functionality,
> thus perf_events would require considerable rework prior it could be controlled
> thru SELinux.
You mean this:
da97e18458fb ("perf_event: Add support for LSM and SELinux checks")
?
> Then the adoption could also require changes to the installed
> infrastructure just for the sake of adopting alternative access control mechanism.
This is still very much true.
^ permalink raw reply
* [PATCH AUTOSEL 4.19 64/79] apparmor: fix unsigned len comparison with less than zero
From: Sasha Levin @ 2019-12-11 15:26 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Colin Ian King, John Johansen, Sasha Levin, linux-security-module
In-Reply-To: <20191211152643.23056-1-sashal@kernel.org>
From: Colin Ian King <colin.king@canonical.com>
[ Upstream commit 00e0590dbaec6f1bcaa36a85467d7e3497ced522 ]
The sanity check in macro update_for_len checks to see if len
is less than zero, however, len is a size_t so it can never be
less than zero, so this sanity check is a no-op. Fix this by
making len a ssize_t so the comparison will work and add ulen
that is a size_t copy of len so that the min() macro won't
throw warnings about comparing different types.
Addresses-Coverity: ("Macro compares unsigned to 0")
Fixes: f1bd904175e8 ("apparmor: add the base fns() for domain labels")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
Signed-off-by: John Johansen <john.johansen@canonical.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
security/apparmor/label.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/security/apparmor/label.c b/security/apparmor/label.c
index ba11bdf9043aa..2469549842d24 100644
--- a/security/apparmor/label.c
+++ b/security/apparmor/label.c
@@ -1462,11 +1462,13 @@ static inline bool use_label_hname(struct aa_ns *ns, struct aa_label *label,
/* helper macro for snprint routines */
#define update_for_len(total, len, size, str) \
do { \
+ size_t ulen = len; \
+ \
AA_BUG(len < 0); \
- total += len; \
- len = min(len, size); \
- size -= len; \
- str += len; \
+ total += ulen; \
+ ulen = min(ulen, size); \
+ size -= ulen; \
+ str += ulen; \
} while (0)
/**
@@ -1601,7 +1603,7 @@ int aa_label_snxprint(char *str, size_t size, struct aa_ns *ns,
struct aa_ns *prev_ns = NULL;
struct label_it i;
int count = 0, total = 0;
- size_t len;
+ ssize_t len;
AA_BUG(!str && size != 0);
AA_BUG(!label);
--
2.20.1
^ permalink raw reply related
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