* Re: [RFC PATCH] security: add an interface to lookup the lockdown reason
From: Paul Moore @ 2019-12-10 15:58 UTC (permalink / raw)
To: Stephen Smalley; +Cc: selinux, linux-security-module, linux-next, jamorris
In-Reply-To: <a11bfefc-c010-36ca-2303-35dcd4e9aa41@tycho.nsa.gov>
On Tue, Dec 10, 2019 at 10:45 AM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> On 12/10/19 10:04 AM, Paul Moore wrote:
> > On Tue, Dec 10, 2019 at 9:59 AM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> >> On 12/9/19 9:28 PM, Paul Moore wrote:
> >>> With CONFIG_AUDIT enabled but CONFIG_SECURITY disabled we run into
> >>> a problem where the lockdown reason table is missing. This patch
> >>> attempts to fix this by hiding the table behind a lookup function.
> >>
> >> Shouldn't lsm_audit.c be conditional on both CONFIG_AUDIT and
> >> CONFIG_SECURITY? When/why would we want it built without
> >> CONFIG_SECURITY enabled?
> >
> > My first thought of a fix was just that, but I remembered that the
> > capabilities code is built regardless of the CONFIG_SECURITY setting
> > and I thought there might be some value in allowing for lsm_audit to
> > be used in commoncap (although in full disclosure commoncap doesn't
> > currently make use of lsm_audit).
>
> Seems contrary to normal practice, i.e. if/when commoncap grows a
> dependency, it can be changed then.
Okay, want to submit a tested patch? I really would like to get this
fixed before today's linux-next run.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [RFC PATCH] selinux: ensure we cleanup the internal AVC counters on error in avc_insert()
From: Stephen Smalley @ 2019-12-10 16:12 UTC (permalink / raw)
To: Paul Moore; +Cc: selinux, rsiddoji, linux-security-module
In-Reply-To: <CAHC9VhSO0Jaqyxw_5AtPTTQTqS+Q9CWhBQQ7822hvUS8MWLy6A@mail.gmail.com>
On 12/10/19 10:54 AM, Paul Moore wrote:
> On Tue, Dec 10, 2019 at 8:44 AM Stephen Smalley <sds@tycho.nsa.gov> wrote:
>> On 12/9/19 8:53 PM, Paul Moore wrote:
>>> In AVC insert we don't call avc_node_kill() when avc_xperms_populate()
>>> fails, resulting in the avc->avc_cache.active_nodes counter having a
>>> false value.
>>
>> incorrect value?
>>
>> This patch corrects this problem and does some cleanup
>>> in avc_insert() while we are there.
>>
>> submitting-patches.rst recommends describing in imperative mood and
>> avoiding the words "patch" in what will eventually just be a commit log,
>> ala "Correct this problem and perform some cleanup..."
>
> Well, you've made me feel better about my nit-picky comments on patches ;)
>
> Are you okay with the following?
>
> selinux: ensure we cleanup the internal AVC counters on error in avc_insert()
>
> Fix avc_insert() to call avc_node_kill() if we've already allocated
> an AVC node and the code fails to insert the node in the cache.
Sure, or just "Fix the AVC to correctly decrement the count of AVC nodes
if it encounters an allocation failure on an extended permissions node."
>
>> Should probably add a:
>>
>> Fixes: fa1aa143ac4a ("selinux: extended permissions for ioctls")
>>
>> Might be easier to back port if you split the cleanup from the fix, but
>> your call of course.
>
> I waffled on that last night when I wrote up the patch, and more
> generally if this should go to -stable or -next (despite what is
> claimed, adding a "Fixes:" tag means it gets picked up by -stable more
> often than not in my experience). At its worst, not fixing this bug
> means we could end up effectively shrinking the AVC cache if xperms
> are used *and* we happen to fail a memory allocation while adding a
> new entry to the AVC; we don't cause an incorrect node to be cached,
> we don't crash the system, we don't leak memory. My thinking is that
> this isn't a major concern, and not worth the risk to -stable, but if
> anyone has any data that shows otherwise, please let me know.
>
> I'll go ahead and add the "Fixes:" tag (technically this is the
> *right* thing to do), but I'm going to stick with -next and leave the
> cleanup as-is just to raise the bar a bit for the -stable backports
> which I'm sure are going to happen.
>
^ permalink raw reply
* Re: [RFC PATCH] security: add an interface to lookup the lockdown reason
From: Stephen Smalley @ 2019-12-10 16:20 UTC (permalink / raw)
To: Paul Moore; +Cc: selinux, linux-security-module, linux-next, jamorris
In-Reply-To: <CAHC9VhRjs-pMWD-2ZTcF42eR3ugW7Bn7uYhmp4cQFneOtcqUkg@mail.gmail.com>
On 12/10/19 10:58 AM, Paul Moore wrote:
> On Tue, Dec 10, 2019 at 10:45 AM Stephen Smalley <sds@tycho.nsa.gov> wrote:
>> On 12/10/19 10:04 AM, Paul Moore wrote:
>>> On Tue, Dec 10, 2019 at 9:59 AM Stephen Smalley <sds@tycho.nsa.gov> wrote:
>>>> On 12/9/19 9:28 PM, Paul Moore wrote:
>>>>> With CONFIG_AUDIT enabled but CONFIG_SECURITY disabled we run into
>>>>> a problem where the lockdown reason table is missing. This patch
>>>>> attempts to fix this by hiding the table behind a lookup function.
>>>>
>>>> Shouldn't lsm_audit.c be conditional on both CONFIG_AUDIT and
>>>> CONFIG_SECURITY? When/why would we want it built without
>>>> CONFIG_SECURITY enabled?
>>>
>>> My first thought of a fix was just that, but I remembered that the
>>> capabilities code is built regardless of the CONFIG_SECURITY setting
>>> and I thought there might be some value in allowing for lsm_audit to
>>> be used in commoncap (although in full disclosure commoncap doesn't
>>> currently make use of lsm_audit).
>>
>> Seems contrary to normal practice, i.e. if/when commoncap grows a
>> dependency, it can be changed then.
>
> Okay, want to submit a tested patch? I really would like to get this
> fixed before today's linux-next run.
In looking at it, I'm wondering if we could just change the Makefile to
use obj-$(CONFIG_SECURITY) instead of obj-$(CONFIG_AUDIT) for
lsm_audit.c. I think it might build just fine without CONFIG_AUDIT
since audit.h provides static inlines that boil away to nothing for
audit_log*() in that case. Offhand I don't see any support/examples of
specifying two different config options for an obj list in a Makefile.
The other option would be to introduce its own CONFIG_LSM_AUDIT option
and select that option automatically for the modules that use it, which
would currently be selinux, apparmor, and smack. Then if you aren't
using any of those modules you can omit it from your kernel.
^ permalink raw reply
* Re: [PATCH] selinux: reorder hooks to make runtime disable less broken
From: Casey Schaufler @ 2019-12-10 16:23 UTC (permalink / raw)
To: Ondrej Mosnacek; +Cc: Stephen Smalley, SElinux list, Paul Moore, LSM
In-Reply-To: <CAFqZXNsZTRveUYBdsXC2iM2MU+nWPz0xL9eLRFwFYMnti-Ww-g@mail.gmail.com>
On 12/10/2019 3:27 AM, Ondrej Mosnacek wrote:
> On Mon, Dec 9, 2019 at 6:20 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 12/9/2019 5:58 AM, Stephen Smalley wrote:
>>> On 12/9/19 8:21 AM, Stephen Smalley wrote:
>>>> On 12/9/19 2:57 AM, Ondrej Mosnacek wrote:
>>>>> Commit b1d9e6b0646d ("LSM: Switch to lists of hooks") switched the LSM
>>>>> infrastructure to use per-hook lists, which meant that removing the
>>>>> hooks for a given module was no longer atomic. Even though the commit
>>>>> clearly documents that modules implementing runtime revmoval of hooks
>>>>> (only SELinux attempts this madness) need to take special precautions to
>>>>> avoid race conditions, SELinux has never addressed this.
>>>>>
>>>>> By inserting an artificial delay between the loop iterations of
>>>>> security_delete_hooks() (I used 100 ms), booting to a state where
>>>>> SELinux is enabled, but policy is not yet loaded, and running these
>>>>> commands:
>>>>>
>>>>> while true; do ping -c 1 <some IP>; done &
>>>>> echo -n 1 >/sys/fs/selinux/disable
>>>>> kill %1
>>>>> wait
>>>>>
>>>>> ...I was able to trigger NULL pointer dereferences in various places. I
>>>>> also have a report of someone getting panics on a stock RHEL-8 kernel
>>>>> after setting SELINUX=disabled in /etc/selinux/config and rebooting
>>>>> (without adding "selinux=0" to kernel command-line).
>>>>>
>>>>> Reordering the SELinux hooks such that those that allocate structures
>>>>> are removed last seems to prevent these panics. It is very much possible
>>>>> that this doesn't make the runtime disable completely race-free, but at
>>>>> least it makes the operation much less fragile.
>>>>>
>>>>> An alternative (and safer) solution would be to add NULL checks to each
>>>>> hook, but doing this just to support the runtime disable hack doesn't
>>>>> seem to be worth the effort...
>>>> Personally, I would prefer to just get rid of runtime disable altogether; it also precludes making the hooks read-only after initialization. IMHO, selinux=0 is the proper way to disable SELinux if necessary. I believe there is an open bugzilla on Fedora related to this issue, since runtime disable was originally introduced for Fedora.
>>> Also, if we have to retain this support, it seems like this ought to be fixed in the LSM framework especially since it was a change there that broke the SELinux implementation.
>> Agreed, mostly. Deleting an LSM is fundamentally something the infrastructure
>> should handle *if* we allow it. Should we decide at some point to allow loadable
>> modules, as Tetsuo has advocated from time to time, we would need a general
>> solution. We don't have a general solution now because only SELinux wants it.
>> The previous implementation was under #ifdef for SELinux. At the time I understood
>> that there was no interest in investing in it. The implementation passed tests
>> at the time.
>>
>> I propose that until such time as someone decides to seriously investigate
>> loadable security modules* the sole user of the deletion mechanism is
>> welcome to invest whatever they like in their special case, and I will be
>> happy to lend whatever assistance I can.
> On my way to lunch I came up with another relatively simple solution
> that should address this problem at the infrastructure level. Let me
> try to write it up into a patch, hopefully it will work...
I await your proposal with keen interest.
>
>> ---
>> * I do not plan to propose an implementation of loadable modules.
>> I leave that as an exercise for the next generation.
>>
>>
^ permalink raw reply
* Re: [RFC PATCH] security: add an interface to lookup the lockdown reason
From: Paul Moore @ 2019-12-10 16:50 UTC (permalink / raw)
To: Stephen Smalley; +Cc: selinux, linux-security-module, linux-next, jamorris
In-Reply-To: <85a3c4ce-0636-30e7-21bf-dfcd4be5cd9c@tycho.nsa.gov>
On Tue, Dec 10, 2019 at 11:20 AM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> On 12/10/19 10:58 AM, Paul Moore wrote:
> > On Tue, Dec 10, 2019 at 10:45 AM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> >> On 12/10/19 10:04 AM, Paul Moore wrote:
> >>> On Tue, Dec 10, 2019 at 9:59 AM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> >>>> On 12/9/19 9:28 PM, Paul Moore wrote:
> >>>>> With CONFIG_AUDIT enabled but CONFIG_SECURITY disabled we run into
> >>>>> a problem where the lockdown reason table is missing. This patch
> >>>>> attempts to fix this by hiding the table behind a lookup function.
> >>>>
> >>>> Shouldn't lsm_audit.c be conditional on both CONFIG_AUDIT and
> >>>> CONFIG_SECURITY? When/why would we want it built without
> >>>> CONFIG_SECURITY enabled?
> >>>
> >>> My first thought of a fix was just that, but I remembered that the
> >>> capabilities code is built regardless of the CONFIG_SECURITY setting
> >>> and I thought there might be some value in allowing for lsm_audit to
> >>> be used in commoncap (although in full disclosure commoncap doesn't
> >>> currently make use of lsm_audit).
> >>
> >> Seems contrary to normal practice, i.e. if/when commoncap grows a
> >> dependency, it can be changed then.
> >
> > Okay, want to submit a tested patch? I really would like to get this
> > fixed before today's linux-next run.
>
> In looking at it, I'm wondering if we could just change the Makefile to
> use obj-$(CONFIG_SECURITY) instead of obj-$(CONFIG_AUDIT) for
> lsm_audit.c. I think it might build just fine without CONFIG_AUDIT
> since audit.h provides static inlines that boil away to nothing for
> audit_log*() in that case. Offhand I don't see any support/examples of
> specifying two different config options for an obj list in a Makefile.
That should work too I think.
> The other option would be to introduce its own CONFIG_LSM_AUDIT option
> and select that option automatically for the modules that use it, which
> would currently be selinux, apparmor, and smack. Then if you aren't
> using any of those modules you can omit it from your kernel.
This seems to be both the right thing to do, as well as overly complicated :)
I'm not sure I have a strong preference at this point, other than
getting it fixed.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* [PATCH] security: only build lsm_audit if CONFIG_SECURITY=y
From: Stephen Smalley @ 2019-12-10 16:55 UTC (permalink / raw)
To: paul; +Cc: selinux, linux-security-module, linux-next, jamorris,
Stephen Smalley
The lsm_audit code is only required when CONFIG_SECURITY is enabled.
It does not have a build dependency on CONFIG_AUDIT since audit.h
provides trivial static inlines for audit_log*() when CONFIG_AUDIT
is disabled. Hence, the Makefile should only add lsm_audit to the
obj lists based on CONFIG_SECURITY, not CONFIG_AUDIT.
Fixes: 59438b46471a ("security,lockdown,selinux: implement SELinux lockdown")
Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
---
security/Makefile | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/security/Makefile b/security/Makefile
index be1dd9d2cb2f..746438499029 100644
--- a/security/Makefile
+++ b/security/Makefile
@@ -22,7 +22,7 @@ obj-$(CONFIG_SECURITY) += security.o
obj-$(CONFIG_SECURITYFS) += inode.o
obj-$(CONFIG_SECURITY_SELINUX) += selinux/
obj-$(CONFIG_SECURITY_SMACK) += smack/
-obj-$(CONFIG_AUDIT) += lsm_audit.o
+obj-$(CONFIG_SECURITY) += lsm_audit.o
obj-$(CONFIG_SECURITY_TOMOYO) += tomoyo/
obj-$(CONFIG_SECURITY_APPARMOR) += apparmor/
obj-$(CONFIG_SECURITY_YAMA) += yama/
--
2.23.0
^ permalink raw reply related
* Re: [PATCH] security: only build lsm_audit if CONFIG_SECURITY=y
From: Paul Moore @ 2019-12-10 19:15 UTC (permalink / raw)
To: Stephen Smalley; +Cc: selinux, linux-security-module, linux-next, jamorris
In-Reply-To: <20191210165541.85245-1-sds@tycho.nsa.gov>
On Tue, Dec 10, 2019 at 11:55 AM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> The lsm_audit code is only required when CONFIG_SECURITY is enabled.
> It does not have a build dependency on CONFIG_AUDIT since audit.h
> provides trivial static inlines for audit_log*() when CONFIG_AUDIT
> is disabled. Hence, the Makefile should only add lsm_audit to the
> obj lists based on CONFIG_SECURITY, not CONFIG_AUDIT.
>
> Fixes: 59438b46471a ("security,lockdown,selinux: implement SELinux lockdown")
> Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
> ---
> security/Makefile | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Merged into selinux/next in order to fix the linux-next build
breakage. James, if you would prefer a different fix, let us know.
> diff --git a/security/Makefile b/security/Makefile
> index be1dd9d2cb2f..746438499029 100644
> --- a/security/Makefile
> +++ b/security/Makefile
> @@ -22,7 +22,7 @@ obj-$(CONFIG_SECURITY) += security.o
> obj-$(CONFIG_SECURITYFS) += inode.o
> obj-$(CONFIG_SECURITY_SELINUX) += selinux/
> obj-$(CONFIG_SECURITY_SMACK) += smack/
> -obj-$(CONFIG_AUDIT) += lsm_audit.o
> +obj-$(CONFIG_SECURITY) += lsm_audit.o
> obj-$(CONFIG_SECURITY_TOMOYO) += tomoyo/
> obj-$(CONFIG_SECURITY_APPARMOR) += apparmor/
> obj-$(CONFIG_SECURITY_YAMA) += yama/
> --
> 2.23.0
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [RFC PATCH] selinux: ensure we cleanup the internal AVC counters on error in avc_insert()
From: Paul Moore @ 2019-12-10 19:19 UTC (permalink / raw)
To: Stephen Smalley; +Cc: selinux, rsiddoji, linux-security-module
In-Reply-To: <2abbcb79-4384-cfb0-1feb-c3a2e042a2ed@tycho.nsa.gov>
On Tue, Dec 10, 2019 at 11:12 AM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> On 12/10/19 10:54 AM, Paul Moore wrote:
> > On Tue, Dec 10, 2019 at 8:44 AM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> >> On 12/9/19 8:53 PM, Paul Moore wrote:
> >>> In AVC insert we don't call avc_node_kill() when avc_xperms_populate()
> >>> fails, resulting in the avc->avc_cache.active_nodes counter having a
> >>> false value.
> >>
> >> incorrect value?
> >>
> >> This patch corrects this problem and does some cleanup
> >>> in avc_insert() while we are there.
> >>
> >> submitting-patches.rst recommends describing in imperative mood and
> >> avoiding the words "patch" in what will eventually just be a commit log,
> >> ala "Correct this problem and perform some cleanup..."
> >
> > Well, you've made me feel better about my nit-picky comments on patches ;)
> >
> > Are you okay with the following?
> >
> > selinux: ensure we cleanup the internal AVC counters on error in avc_insert()
> >
> > Fix avc_insert() to call avc_node_kill() if we've already allocated
> > an AVC node and the code fails to insert the node in the cache.
>
> Sure, or just "Fix the AVC to correctly decrement the count of AVC nodes
> if it encounters an allocation failure on an extended permissions node."
>
> >> Should probably add a:
> >>
> >> Fixes: fa1aa143ac4a ("selinux: extended permissions for ioctls")
> >>
> >> Might be easier to back port if you split the cleanup from the fix, but
> >> your call of course.
> >
> > I waffled on that last night when I wrote up the patch, and more
> > generally if this should go to -stable or -next (despite what is
> > claimed, adding a "Fixes:" tag means it gets picked up by -stable more
> > often than not in my experience). At its worst, not fixing this bug
> > means we could end up effectively shrinking the AVC cache if xperms
> > are used *and* we happen to fail a memory allocation while adding a
> > new entry to the AVC; we don't cause an incorrect node to be cached,
> > we don't crash the system, we don't leak memory. My thinking is that
> > this isn't a major concern, and not worth the risk to -stable, but if
> > anyone has any data that shows otherwise, please let me know.
> >
> > I'll go ahead and add the "Fixes:" tag (technically this is the
> > *right* thing to do), but I'm going to stick with -next and leave the
> > cleanup as-is just to raise the bar a bit for the -stable backports
> > which I'm sure are going to happen.
Merged into selinux/next.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH linux-kselftest/test v2] apparmor: add AppArmor KUnit tests for policy unpack
From: Kees Cook @ 2019-12-10 19:48 UTC (permalink / raw)
To: Brendan Higgins
Cc: shuah, John Johansen, jmorris, serge, Alan Maguire, Iurii Zaikin,
David Gow, Luis Chamberlain, Theodore Ts'o,
Linux Kernel Mailing List, linux-security-module,
KUnit Development, open list:KERNEL SELFTEST FRAMEWORK,
Mike Salvatore
In-Reply-To: <CAFd5g462jFnbPxA2Nvc_3W064kZ8t5oHNE4M_3yt84+NuoiHGQ@mail.gmail.com>
On Mon, Nov 18, 2019 at 04:34:53PM -0800, Brendan Higgins wrote:
> On Thu, Nov 7, 2019 at 3:33 PM Brendan Higgins
> <brendanhiggins@google.com> wrote:
> >
> > On Wed, Nov 06, 2019 at 09:18:27AM -0800, Kees Cook wrote:
> > > On Tue, Nov 05, 2019 at 04:43:29PM -0800, Brendan Higgins wrote:
> > > > From: Mike Salvatore <mike.salvatore@canonical.com>
> > > >
> > > > Add KUnit tests to test AppArmor unpacking of userspace policies.
> > > > AppArmor uses a serialized binary format for loading policies. To find
> > > > policy format documentation see
> > > > Documentation/admin-guide/LSM/apparmor.rst.
> > > >
> > > > In order to write the tests against the policy unpacking code, some
> > > > static functions needed to be exposed for testing purposes. One of the
> > > > goals of this patch is to establish a pattern for which testing these
> > > > kinds of functions should be done in the future.
> > > >
> > > > Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> > > > Signed-off-by: Mike Salvatore <mike.salvatore@canonical.com>
> > > > ---
> > > > security/apparmor/Kconfig | 16 +
> > > > security/apparmor/policy_unpack.c | 4 +
> > > > security/apparmor/policy_unpack_test.c | 607 +++++++++++++++++++++++++
> > > > 3 files changed, 627 insertions(+)
> > > > create mode 100644 security/apparmor/policy_unpack_test.c
> > > >
> > > > diff --git a/security/apparmor/Kconfig b/security/apparmor/Kconfig
> > > > index d8b1a360a6368..78a33ccac2574 100644
> > > > --- a/security/apparmor/Kconfig
> > > > +++ b/security/apparmor/Kconfig
> > > > @@ -66,3 +66,19 @@ config SECURITY_APPARMOR_DEBUG_MESSAGES
> > > > Set the default value of the apparmor.debug kernel parameter.
> > > > When enabled, various debug messages will be logged to
> > > > the kernel message buffer.
> > > > +
> > > > +config SECURITY_APPARMOR_KUNIT_TEST
> > > > + bool "Build KUnit tests for policy_unpack.c"
> > > > + depends on KUNIT && SECURITY_APPARMOR
> > > > + help
> > > > + This builds the AppArmor KUnit tests.
> > > > +
> > > > + KUnit tests run during boot and output the results to the debug log
> > > > + in TAP format (http://testanything.org/). Only useful for kernel devs
> > > > + running KUnit test harness and are not for inclusion into a
> > > > + production build.
> > > > +
> > > > + For more information on KUnit and unit tests in general please refer
> > > > + to the KUnit documentation in Documentation/dev-tools/kunit/.
> > > > +
> > > > + If unsure, say N.
> > > > diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c
> > > > index 8cfc9493eefc7..37c1dd3178fc0 100644
> > > > --- a/security/apparmor/policy_unpack.c
> > > > +++ b/security/apparmor/policy_unpack.c
> > > > @@ -1120,3 +1120,7 @@ int aa_unpack(struct aa_loaddata *udata, struct list_head *lh,
> > > >
> > > > return error;
> > > > }
> > > > +
> > > > +#ifdef CONFIG_SECURITY_APPARMOR_KUNIT_TEST
> > > > +#include "policy_unpack_test.c"
> > > > +#endif /* CONFIG_SECURITY_APPARMOR_KUNIT_TEST */
> > >
> > > To make this even LESS intrusive, the ifdefs could live in ..._test.c.
> >
> > Less intrusive, yes, but I think I actually like the ifdef here; it
> > makes it clear from the source that the test is only a part of the build
> > when configured to do so. Nevertheless, I will change it if anyone feels
> > strongly about it.
> >
> > > Also, while I *think* the kernel build system will correctly track this
> > > dependency, can you double-check that changes to ..._test.c correctly
> > > trigger a recompile of policy_unpack.c?
> >
> > Yep, just verified, first I ran the tests and everything passed. Then I
> > applied the following diff:
> >
> > diff --git a/security/apparmor/policy_unpack_test.c b/security/apparmor/policy_unpack_test.c
> > index 533137f45361c..e1b0670dbdc27 100644
> > --- a/security/apparmor/policy_unpack_test.c
> > +++ b/security/apparmor/policy_unpack_test.c
> > @@ -161,7 +161,7 @@ static void policy_unpack_test_unpack_array_with_name(struct kunit *test)
> >
> > array_size = unpack_array(puf->e, name);
> >
> > - KUNIT_EXPECT_EQ(test, array_size, (u16)TEST_ARRAY_SIZE);
> > + KUNIT_EXPECT_EQ(test, array_size + 1, (u16)TEST_ARRAY_SIZE);
> > KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
> > puf->e->start + TEST_ARRAY_BUF_OFFSET + sizeof(u16) + 1);
> > }
> >
> > and reran the tests (to trigger an incremental build) and the test
> > failed as expected indicating that the dependency is properly tracked.
>
> Hey Kees,
>
> Since it looks like you already took a pretty close look at this,
> would you mind giving me a review?
Yes! Thanks for checking on those items. :)
Reviewed-by: Kees Cook <keescook@chromium.org>
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v6 00/27] nfs: Mount API conversion
From: Schumaker, Anna @ 2019-12-10 21:16 UTC (permalink / raw)
To: smayhew@redhat.com, trond.myklebust@hammerspace.com
Cc: linux-security-module@vger.kernel.org, linux-nfs@vger.kernel.org,
linux-fsdevel@vger.kernel.org, viro@zeniv.linux.org.uk,
linux-kernel@vger.kernel.org, dhowells@redhat.com
In-Reply-To: <20191210123115.1655-1-smayhew@redhat.com>
Hi Scott,
On Tue, 2019-12-10 at 07:30 -0500, Scott Mayhew wrote:
> Hi Anna, Trond,
>
> Here's a set of patches that converts NFS to use the mount API. Note that
> there are a lot of preliminary patches, some from David and some from Al.
> The final patch (the one that does the actual conversion) from the David's
> initial posting has been split into 5 separate patches, and the entire set
> has been rebased on top of v5.5-rc1.
Thanks for the updated patches! Everything looks okay to me, but I've only
tested with the legacy mount command. I'm curious if you've tested it using the
new system?
Thanks,
Anna
>
> Changes since v5:
> - fixed possible derefence of error pointer in nfs4_validate_fspath()
> reported by Dan Carpenter
> - rebased on top of v5.5-rc1
> Changes since v4:
> - further split the original "NFS: Add fs_context support" patch (new
> patch is about 25% smaller than the v4 patch)
> - fixed NFSv4 referral mounts (broken in the original patch)
> - fixed leak of nfs_fattr when fs_context is freed
> Changes since v3:
> - changed license and copyright text in fs/nfs/fs_context.c
> Changes since v2:
> - fixed the conversion of the nconnect= option
> - added '#if IS_ENABLED(CONFIG_NFS_V4)' around nfs4_parse_monolithic()
> to avoid unused-function warning when compiling with v4 disabled
> Chagnes since v1:
> - split up patch 23 into 4 separate patches
>
> -Scott
>
> Al Viro (15):
> saner calling conventions for nfs_fs_mount_common()
> nfs: stash server into struct nfs_mount_info
> nfs: lift setting mount_info from nfs4_remote{,_referral}_mount
> nfs: fold nfs4_remote_fs_type and nfs4_remote_referral_fs_type
> nfs: don't bother setting/restoring export_path around
> do_nfs_root_mount()
> nfs4: fold nfs_do_root_mount/nfs_follow_remote_path
> nfs: lift setting mount_info from nfs_xdev_mount()
> nfs: stash nfs_subversion reference into nfs_mount_info
> nfs: don't bother passing nfs_subversion to ->try_mount() and
> nfs_fs_mount_common()
> nfs: merge xdev and remote file_system_type
> nfs: unexport nfs_fs_mount_common()
> nfs: don't pass nfs_subversion to ->create_server()
> nfs: get rid of mount_info ->fill_super()
> nfs_clone_sb_security(): simplify the check for server bogosity
> nfs: get rid of ->set_security()
>
> David Howells (8):
> NFS: Move mount parameterisation bits into their own file
> NFS: Constify mount argument match tables
> NFS: Rename struct nfs_parsed_mount_data to struct nfs_fs_context
> NFS: Split nfs_parse_mount_options()
> NFS: Deindent nfs_fs_context_parse_option()
> NFS: Add a small buffer in nfs_fs_context to avoid string dup
> NFS: Do some tidying of the parsing code
> NFS: Add fs_context support.
>
> Scott Mayhew (4):
> NFS: rename nfs_fs_context pointer arg in a few functions
> NFS: Convert mount option parsing to use functionality from
> fs_parser.h
> NFS: Additional refactoring for fs_context conversion
> NFS: Attach supplementary error information to fs_context.
>
> fs/nfs/Makefile | 2 +-
> fs/nfs/client.c | 80 +-
> fs/nfs/fs_context.c | 1424 +++++++++++++++++++++++++
> fs/nfs/fscache.c | 2 +-
> fs/nfs/getroot.c | 73 +-
> fs/nfs/internal.h | 132 +--
> fs/nfs/namespace.c | 146 ++-
> fs/nfs/nfs3_fs.h | 2 +-
> fs/nfs/nfs3client.c | 6 +-
> fs/nfs/nfs3proc.c | 2 +-
> fs/nfs/nfs4_fs.h | 9 +-
> fs/nfs/nfs4client.c | 99 +-
> fs/nfs/nfs4file.c | 1 +
> fs/nfs/nfs4namespace.c | 292 +++---
> fs/nfs/nfs4proc.c | 2 +-
> fs/nfs/nfs4super.c | 257 ++---
> fs/nfs/proc.c | 2 +-
> fs/nfs/super.c | 2217 +++++----------------------------------
> include/linux/nfs_xdr.h | 9 +-
> 19 files changed, 2287 insertions(+), 2470 deletions(-)
> create mode 100644 fs/nfs/fs_context.c
>
^ permalink raw reply
* Re: [PATCH v2] tomoyo: Don't check open/getattr permission on sockets.
From: Stephen Rothwell @ 2019-12-10 23:02 UTC (permalink / raw)
To: Tetsuo Handa
Cc: James Morris, Andrew Morton, linux-security-module,
Linus Torvalds
In-Reply-To: <457927e7-2cec-3933-3e5c-67ebd29d8a52@i-love.sakura.ne.jp>
[-- Attachment #1: Type: text/plain, Size: 3762 bytes --]
Hi Tetsuo,
On Tue, 10 Dec 2019 19:21:08 +0900 Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote:
>
> On 2019/12/10 6:37, James Morris wrote:
> > On Wed, 4 Dec 2019, Tetsuo Handa wrote:
> >
> >>
> >> I decided to drop tomoyo_get_socket_name(). Will you pick up the following commit?
> >>
> >> commit c39593ab0500fcd6db290b311c120349927ddc04
> >> Author: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> >> Date: Mon Nov 25 10:46:51 2019 +0900
> >>
> >> tomoyo: Don't use nifty names on sockets.
> >>
> >
> >>From where?
> >
> > Please send a patch.
> >
>
> Patch is at https://scm.osdn.net/gitroot/tomoyo/tomoyo-test1 and was tested on linux-next.git .
> But if you pick up c39593ab0500, what do I need to do (in order to avoid trying to apply the same
> patch) ? Could you explain me (using command line) how I can send only c39593ab0500 to linux.git ?
> https://osdn.net/projects/tomoyo/scm/git/tomoyo-test1/commits has only master branch.
>
> c39593ab0500 (HEAD -> master, origin/master) tomoyo: Don't use nifty names on sockets.
> cbf8353d474c Merge branch 'master' of https://scm.osdn.net/gitroot/tomoyo/tomoyo-test1
> fd46afeac605 Revert "tomoyo: Don't check open/getattr permission on sockets."
> 19768fdc4025 Revert "printk: Monitor change of console loglevel."
> 07fca3f339d7 printk: Monitor change of console loglevel.
> df8aec8cd8b2 tomoyo: Don't check open/getattr permission on sockets.
> 219d54332a09 (tag: v5.4, upstream/master) Linux 5.4
You should start by cleaning up your tree:
remove
fd46afeac605 Revert "tomoyo: Don't check open/getattr permission on sockets."
19768fdc4025 Revert "printk: Monitor change of console loglevel."
07fca3f339d7 printk: Monitor change of console loglevel.
df8aec8cd8b2 tomoyo: Don't check open/getattr permission on sockets.
since they end up cancelling each other out
cbf8353d474c Merge branch 'master' of https://scm.osdn.net/gitroot/tomoyo/tomoyo-test1
only introduces these commits:
79c8ca578dbf Revert "printk: Monitor change of console loglevel."
23641a048089 printk: Monitor change of console loglevel.
a5f9bda81cb4 tomoyo: Don't check open/getattr permission on sockets.
and the first 2 above cancel each other out.
so you are left with these:
c39593ab0500 tomoyo: Don't use nifty names on sockets.
a5f9bda81cb4 tomoyo: Don't check open/getattr permission on sockets.
you should rebase these onto v5.5-rc1.
If you want James to just take the first of these, then the easiest way
is probably to just send him a patch that you generate using "git
format-patch" and then remove it from your tree.
Since there are no other changes to the only file affected by that
commit since v5.4, you could just do this:
$ git format-patch -o <some file> -1 c39593ab0500
and then <some file> to James using your email client.
Having done that, you should just do this (and forget the cleanups
above):
$ git checkout master
$ git remote update upstream
$ git reset --hard upstream/master
$ git cherry-pick a5f9bda81cb4
$ git push -f origin master
After that you will have a nice clean tree (based on Linus' tree) to
continue development on that just contains the one patch "tomoyo: Don't
check open/getattr permission on sockets."
If, however, you intend to only send patche via James tree, then you
should be using his tree
(git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git
branch next-testing) as your upstream tree, not Linus' tree. Then you
can ask him to merge your tree before the merge window opens during
each cycle. You may want to rebase your tree on top of James tree
after he applies your patch from above.
--
Cheers,
Stephen Rothwell
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* [PATCH 2/8] afs: Fix SELinux setting security label on /afs
From: David Howells @ 2019-12-11 10:27 UTC (permalink / raw)
To: linux-afs; +Cc: selinux, linux-security-module, dhowells
In-Reply-To: <157606006065.21869.6615813521356169209.stgit@warthog.procyon.org.uk>
Make the AFS dynamic root superblock R/W so that SELinux can set the
security label on it. Without this, upgrades to, say, the Fedora
filesystem-afs RPM fail if afs is mounted on it because the SELinux label
can't be (re-)applied.
It might be better to make it possible to bypass the R/O check for LSM
label application through setxattr.
Fixes: 4d673da14533 ("afs: Support the AFS dynamic root")
Signed-off-by: David Howells <dhowells@redhat.com>
cc: selinux@vger.kernel.org
cc: linux-security-module@vger.kernel.org
---
fs/afs/super.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/fs/afs/super.c b/fs/afs/super.c
index 488641b1a418..d9a6036b70b9 100644
--- a/fs/afs/super.c
+++ b/fs/afs/super.c
@@ -448,7 +448,6 @@ static int afs_fill_super(struct super_block *sb, struct afs_fs_context *ctx)
/* allocate the root inode and dentry */
if (as->dyn_root) {
inode = afs_iget_pseudo_dir(sb, true);
- sb->s_flags |= SB_RDONLY;
} else {
sprintf(sb->s_id, "%llu", as->volume->vid);
afs_activate_volume(as->volume);
^ permalink raw reply related
* Re: [PATCH v1 0/3] Introduce CAP_SYS_PERFMON capability for secure Perf users groups
From: Alexey Budankov @ 2019-12-11 10:52 UTC (permalink / raw)
To: Casey Schaufler, 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
In-Reply-To: <a81248c5-971a-9d3f-6df4-e6335384fe7f@schaufler-ca.com>
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.
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 v2] tomoyo: Don't check open/getattr permission on sockets.
From: Tetsuo Handa @ 2019-12-11 11:19 UTC (permalink / raw)
To: Stephen Rothwell
Cc: James Morris, Andrew Morton, linux-security-module,
Linus Torvalds
In-Reply-To: <20191211100215.2c0aec54@canb.auug.org.au>
Hello, Stephen Rothwell.
Thank you for the command line.
I was wondering why "git push -f" does not work. But I finally found
there is denyNonFastforwards option, and I was able to clean up.
$ git format-patch -1
0001-tomoyo-Don-t-use-nifty-names-on-sockets.patch
$ git reset --hard v5.5-rc1
$ git push -f origin master
$ git pull upstream master
$ git am 0001-tomoyo-Don-t-use-nifty-names-on-sockets.patch
$ git push origin master
On 2019/12/11 8:02, Stephen Rothwell wrote:
> Having done that, you should just do this (and forget the cleanups
> above):
>
> $ git checkout master
> $ git remote update upstream
> $ git reset --hard upstream/master
> $ git cherry-pick a5f9bda81cb4
> $ git push -f origin master
>
> After that you will have a nice clean tree (based on Linus' tree) to
> continue development on that just contains the one patch "tomoyo: Don't
> check open/getattr permission on sockets."
Now the history looks like below.
6f7c41374b62 (HEAD -> master, origin/master) tomoyo: Don't use nifty names on sockets.
6794862a16ef (upstream/master) Merge tag 'for-5.5-rc1-kconfig-tag' of git://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux
184b8f7f91ca Merge tag 'printk-for-5.5-pr-warning-removal' of git://git.kernel.org/pub/scm/linux/kernel/git/pmladek/printk
316eaf170252 Merge tag 'thermal-5.5-rc2' of git://git.kernel.org/pub/scm/linux/kernel/git/thermal/linux
78f926f72e43 btrfs: add Kconfig dependency for BLAKE2B
e42617b825f8 (tag: v5.5-rc1) Linux 5.5-rc1
>
> If, however, you intend to only send patche via James tree, then you
> should be using his tree
> (git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git
> branch next-testing) as your upstream tree, not Linus' tree. Then you
> can ask him to merge your tree before the merge window opens during
> each cycle. You may want to rebase your tree on top of James tree
> after he applies your patch from above.
>
I was previously using linux-security.git . But after experiencing confusion of
linux-security.git management, LSM users (including me) were suggested to have
their own git tree and were suggested to directly send patches to Linus.
And I am currently experiencing confusion of my tree management (because I have
never maintained a tree for "git push" purpose)...
Next step is how to create a pull request. If I recall correctly, I need a
GPG key for signing my request? I have a GPG key but I have never attended
key signing party.
^ permalink raw reply
* Re: [PATCH 0/2] Revert patches fixing probing of interrupts
From: Jarkko Sakkinen @ 2019-12-11 11:22 UTC (permalink / raw)
To: Stefan Berger, Stefan Berger, linux-integrity, linux-kernel,
stable, linux-security-module
In-Reply-To: <20191209215535.pw6ewyetskaet2o6@cantor>
On Mon, Dec 09, 2019 at 02:55:35PM -0700, Jerry Snitselaar wrote:
> On Mon Dec 09 19, Jarkko Sakkinen wrote:
> > On Mon, Dec 02, 2019 at 11:55:20AM -0700, Jerry Snitselaar wrote:
> > > On Sun Dec 01 19, Stefan Berger wrote:
> > > > On 11/29/19 5:37 PM, Jarkko Sakkinen wrote:
> > > > > On Tue, Nov 26, 2019 at 08:17:51AM -0500, Stefan Berger wrote:
> > > > > > From: Stefan Berger <stefanb@linux.ibm.com>
> > > > > >
> > > > > > Revert the patches that were fixing the probing of interrupts due
> > > > > > to reports of interrupt stroms on some systems
> > > > > Can you explain how reverting is going to fix the issue?
> > > >
> > > >
> > > > The reverts fix 'the interrupt storm issue' that they are causing on
> > > > some systems but don't fix the issue with the interrupt mode not being
> > > > used. I was hoping Jerry would get access to a system faster but this
> > > > didn't seem to be the case. So sending these patches seemed the better
> > > > solution than leaving 5.4.x with the problem but going back to when it
> > > > worked 'better.'
> > > >
> > >
> > > I finally heard back from IT support, and unfortunately they don't
> > > have any T490s systems to give out on temp loan. So I can only send
> > > patched kernels to the end user that had the problem.
> >
> > At least it is a fact that tpm_chip_stop() is called too early and that
> > is destined to cause issues.
> >
> > Should I bake a patch or do you have already something?
> >
> > /Jarkko
> >
>
> This is what I'm currently building:
With a quick skim looks what I had in mind.
/Jarkko
^ permalink raw reply
* [PATCH] apparmor: fix bind mounts aborting with -ENOMEM
From: Patrick Steinhardt @ 2019-12-11 11:44 UTC (permalink / raw)
To: linux-security-module
Cc: Patrick Steinhardt, John Johansen, James Morris, Serge E . Hallyn,
Sebastian Andrzej Siewior
With commit df323337e507 (apparmor: Use a memory pool instead per-CPU
caches, 2019-05-03), AppArmor code was converted to use memory pools. In
that conversion, a bug snuck into the code that polices bind mounts that
causes all bind mounts to fail with -ENOMEM, as we erroneously error out
if `aa_get_buffer` returns a pointer instead of erroring out when it
does _not_ return a valid pointer.
Fix the issue by correctly checking for valid pointers returned by
`aa_get_buffer` to fix bind mounts with AppArmor.
Fixes: df323337e507 (apparmor: Use a memory pool instead per-CPU caches)
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
I've fixed the issue on top of v5.5-rc1, where I in fact found
the issue.
security/apparmor/mount.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/security/apparmor/mount.c b/security/apparmor/mount.c
index 4ed6688f9d40..e0828ee7a345 100644
--- a/security/apparmor/mount.c
+++ b/security/apparmor/mount.c
@@ -442,7 +442,7 @@ int aa_bind_mount(struct aa_label *label, const struct path *path,
buffer = aa_get_buffer(false);
old_buffer = aa_get_buffer(false);
error = -ENOMEM;
- if (!buffer || old_buffer)
+ if (!buffer || !old_buffer)
goto out;
error = fn_for_each_confined(label, profile,
--
2.24.1
^ permalink raw reply related
* Re: [PATCH v6 00/27] nfs: Mount API conversion
From: Scott Mayhew @ 2019-12-11 13:21 UTC (permalink / raw)
To: Schumaker, Anna
Cc: trond.myklebust@hammerspace.com,
linux-security-module@vger.kernel.org, linux-nfs@vger.kernel.org,
linux-fsdevel@vger.kernel.org, viro@zeniv.linux.org.uk,
linux-kernel@vger.kernel.org, dhowells@redhat.com
In-Reply-To: <498258bf630d4c2667920f21341a2a6e82a3788d.camel@netapp.com>
On Tue, 10 Dec 2019, Schumaker, Anna wrote:
> Hi Scott,
>
> On Tue, 2019-12-10 at 07:30 -0500, Scott Mayhew wrote:
> > Hi Anna, Trond,
> >
> > Here's a set of patches that converts NFS to use the mount API. Note that
> > there are a lot of preliminary patches, some from David and some from Al.
> > The final patch (the one that does the actual conversion) from the David's
> > initial posting has been split into 5 separate patches, and the entire set
> > has been rebased on top of v5.5-rc1.
>
> Thanks for the updated patches! Everything looks okay to me, but I've only
> tested with the legacy mount command. I'm curious if you've tested it using the
> new system?
My testing was also with the legacy mount command. I can work on
testing with the new syscalls now.
-Scott
>
> Thanks,
> Anna
>
> >
> > Changes since v5:
> > - fixed possible derefence of error pointer in nfs4_validate_fspath()
> > reported by Dan Carpenter
> > - rebased on top of v5.5-rc1
> > Changes since v4:
> > - further split the original "NFS: Add fs_context support" patch (new
> > patch is about 25% smaller than the v4 patch)
> > - fixed NFSv4 referral mounts (broken in the original patch)
> > - fixed leak of nfs_fattr when fs_context is freed
> > Changes since v3:
> > - changed license and copyright text in fs/nfs/fs_context.c
> > Changes since v2:
> > - fixed the conversion of the nconnect= option
> > - added '#if IS_ENABLED(CONFIG_NFS_V4)' around nfs4_parse_monolithic()
> > to avoid unused-function warning when compiling with v4 disabled
> > Chagnes since v1:
> > - split up patch 23 into 4 separate patches
> >
> > -Scott
> >
> > Al Viro (15):
> > saner calling conventions for nfs_fs_mount_common()
> > nfs: stash server into struct nfs_mount_info
> > nfs: lift setting mount_info from nfs4_remote{,_referral}_mount
> > nfs: fold nfs4_remote_fs_type and nfs4_remote_referral_fs_type
> > nfs: don't bother setting/restoring export_path around
> > do_nfs_root_mount()
> > nfs4: fold nfs_do_root_mount/nfs_follow_remote_path
> > nfs: lift setting mount_info from nfs_xdev_mount()
> > nfs: stash nfs_subversion reference into nfs_mount_info
> > nfs: don't bother passing nfs_subversion to ->try_mount() and
> > nfs_fs_mount_common()
> > nfs: merge xdev and remote file_system_type
> > nfs: unexport nfs_fs_mount_common()
> > nfs: don't pass nfs_subversion to ->create_server()
> > nfs: get rid of mount_info ->fill_super()
> > nfs_clone_sb_security(): simplify the check for server bogosity
> > nfs: get rid of ->set_security()
> >
> > David Howells (8):
> > NFS: Move mount parameterisation bits into their own file
> > NFS: Constify mount argument match tables
> > NFS: Rename struct nfs_parsed_mount_data to struct nfs_fs_context
> > NFS: Split nfs_parse_mount_options()
> > NFS: Deindent nfs_fs_context_parse_option()
> > NFS: Add a small buffer in nfs_fs_context to avoid string dup
> > NFS: Do some tidying of the parsing code
> > NFS: Add fs_context support.
> >
> > Scott Mayhew (4):
> > NFS: rename nfs_fs_context pointer arg in a few functions
> > NFS: Convert mount option parsing to use functionality from
> > fs_parser.h
> > NFS: Additional refactoring for fs_context conversion
> > NFS: Attach supplementary error information to fs_context.
> >
> > fs/nfs/Makefile | 2 +-
> > fs/nfs/client.c | 80 +-
> > fs/nfs/fs_context.c | 1424 +++++++++++++++++++++++++
> > fs/nfs/fscache.c | 2 +-
> > fs/nfs/getroot.c | 73 +-
> > fs/nfs/internal.h | 132 +--
> > fs/nfs/namespace.c | 146 ++-
> > fs/nfs/nfs3_fs.h | 2 +-
> > fs/nfs/nfs3client.c | 6 +-
> > fs/nfs/nfs3proc.c | 2 +-
> > fs/nfs/nfs4_fs.h | 9 +-
> > fs/nfs/nfs4client.c | 99 +-
> > fs/nfs/nfs4file.c | 1 +
> > fs/nfs/nfs4namespace.c | 292 +++---
> > fs/nfs/nfs4proc.c | 2 +-
> > fs/nfs/nfs4super.c | 257 ++---
> > fs/nfs/proc.c | 2 +-
> > fs/nfs/super.c | 2217 +++++----------------------------------
> > include/linux/nfs_xdr.h | 9 +-
> > 19 files changed, 2287 insertions(+), 2470 deletions(-)
> > create mode 100644 fs/nfs/fs_context.c
> >
^ permalink raw reply
* [PATCH] LSM: allow an LSM to disable all hooks at once
From: Ondrej Mosnacek @ 2019-12-11 14:08 UTC (permalink / raw)
To: linux-security-module, James Morris, Serge E. Hallyn
Cc: Casey Schaufler, selinux, Paul Moore, Stephen Smalley,
John Johansen, Kees Cook, Micah Morton, Tetsuo Handa
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.
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;
}
--
2.23.0
^ permalink raw reply related
* Re: [PATCH] LSM: allow an LSM to disable all hooks at once
From: Ondrej Mosnacek @ 2019-12-11 14:13 UTC (permalink / raw)
To: Linux Security Module list, James Morris, Serge E. Hallyn
Cc: Casey Schaufler, SElinux list, Paul Moore, Stephen Smalley,
John Johansen, Kees Cook, Micah Morton, Tetsuo Handa
In-Reply-To: <20191211140833.939845-1-omosnace@redhat.com>
On Wed, Dec 11, 2019 at 3:08 PM Ondrej Mosnacek <omosnace@redhat.com> 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.
>
> 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>
I missed that there is still a hlist_add_tail_rcu() call left, so I'll
have to add this back in the next revision in case of positive
feedback for this patch.
--
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: Stephen Smalley @ 2019-12-11 14:29 UTC (permalink / raw)
To: Ondrej Mosnacek, linux-security-module, James Morris,
Serge E. Hallyn
Cc: Casey Schaufler, selinux, Paul Moore, John Johansen, Kees Cook,
Micah Morton, Tetsuo Handa
In-Reply-To: <20191211140833.939845-1-omosnace@redhat.com>
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.
>
> 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: Looks like issue in handling active_nodes count in 4.19 kernel .
From: Stephen Smalley @ 2019-12-11 14:37 UTC (permalink / raw)
To: rsiddoji, selinux; +Cc: paul, linux-security-module
In-Reply-To: <0101016eebed2b2e-db98eae1-b92b-450b-934e-c8e92c5370b3-000000@us-west-2.amazonses.com>
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?
>
>> /*@ 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: Looks like issue in handling active_nodes count in 4.19 kernel .
From: Stephen Smalley @ 2019-12-11 14:47 UTC (permalink / raw)
To: rsiddoji, selinux; +Cc: paul, linux-security-module
In-Reply-To: <7b047966-33c0-de62-b10f-047819890337@tycho.nsa.gov>
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
* [PATCH AUTOSEL 4.14 46/58] apparmor: fix unsigned len comparison with less than zero
From: Sasha Levin @ 2019-12-11 15:28 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Colin Ian King, John Johansen, Sasha Levin, linux-security-module
In-Reply-To: <20191211152831.23507-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 c5b99b954580c..ea63710442ae5 100644
--- a/security/apparmor/label.c
+++ b/security/apparmor/label.c
@@ -1463,11 +1463,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)
/**
@@ -1602,7 +1604,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: rsiddoji @ 2019-12-11 15:35 UTC (permalink / raw)
To: 'Stephen Smalley', selinux; +Cc: paul, linux-security-module
In-Reply-To: <d6081414-613f-fdb8-8dcd-9ebf6a3baa27@tycho.nsa.gov>
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?
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
* [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