* Re: [PATCH 1/2] LSM: switch to blocking policy update notifiers
From: Casey Schaufler @ 2019-06-05 17:05 UTC (permalink / raw)
To: Janne Karhunen
Cc: Stephen Smalley, Mimi Zohar, Paul Moore, linux-integrity,
linux-security-module, casey
In-Reply-To: <CAE=NcrahPmzmB-xJwxzXqaPGtJY+ijbxV4wXz7K=y-ocw4Cmwg@mail.gmail.com>
On 6/5/2019 9:51 AM, Janne Karhunen wrote:
> On Wed, Jun 5, 2019 at 6:23 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
>>> -int call_lsm_notifier(enum lsm_event event, void *data);
>>> -int register_lsm_notifier(struct notifier_block *nb);
>>> -int unregister_lsm_notifier(struct notifier_block *nb);
>>> +int call_blocking_lsm_notifier(enum lsm_event event, void *data);
>>> +int register_blocking_lsm_notifier(struct notifier_block *nb);
>>> +int unregister_blocking_lsm_notifier(struct notifier_block *nb);
>> Why is it important to change the names of these hooks?
>> It's not like you had call_atomic_lsm_notifier() before.
>> It seems like a lot of unnecessary code churn.
> Paul was thinking there will eventually be two sets of notifiers
> (atomic and blocking) and this creates the clear separation.
One hook with an added "bool blocking" argument, if
that's the only difference?
> That's
> probably true, but it does indeed create a pretty big change that it
> is not really needed yet. I'm fine either way.
>
>
> --
> Janne
^ permalink raw reply
* Re: [RFC][PATCH 0/8] Mount, FS, Block and Keyrings notifications [ver #2]
From: Casey Schaufler @ 2019-06-05 17:01 UTC (permalink / raw)
To: Andy Lutomirski
Cc: David Howells, Al Viro, raven, Linux FS Devel, Linux API,
linux-block, keyrings, LSM List, LKML, casey
In-Reply-To: <CALCETrVSBwHEm-1pgBXxth07PZ0XF6FD+7E25=WbiS7jxUe83A@mail.gmail.com>
On 6/5/2019 9:04 AM, Andy Lutomirski wrote:
> On Wed, Jun 5, 2019 at 7:51 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 6/5/2019 1:41 AM, David Howells wrote:
>>> Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>
>>>> I will try to explain the problem once again. If process A
>>>> sends a signal (writes information) to process B the kernel
>>>> checks that either process A has the same UID as process B
>>>> or that process A has privilege to override that policy.
>>>> Process B is passive in this access control decision, while
>>>> process A is active. In the event delivery case, process A
>>>> does something (e.g. modifies a keyring) that generates an
>>>> event, which is then sent to process B's event buffer.
>>> I think this might be the core sticking point here. It looks like two
>>> different situations:
>>>
>>> (1) A explicitly sends event to B (eg. signalling, sendmsg, etc.)
>>>
>>> (2) A implicitly and unknowingly sends event to B as a side effect of some
>>> other action (eg. B has a watch for the event A did).
>>>
>>> The LSM treats them as the same: that is B must have MAC authorisation to send
>>> a message to A.
>> YES!
>>
>> Threat is about what you can do, not what you intend to do.
>>
>> And it would be really great if you put some thought into what
>> a rational model would be for UID based controls, too.
>>
>>> But there are problems with not sending the event:
>>>
>>> (1) B's internal state is then corrupt (or, at least, unknowingly invalid).
>> Then B is a badly written program.
> Either I'm misunderstanding you or I strongly disagree.
A program needs to be aware of the conditions under
which it gets event, *including the possibility that
it may not get an event that it's not allowed*. Do you
regularly write programs that go into corrupt states
if an open() fails? Or where read() returns less than
the amount of data you ask for?
> If B has
> authority to detect a certain action, and A has authority to perform
> that action, then refusing to notify B because B is somehow missing
> some special authorization to be notified by A is nuts.
You are hand-waving the notion of authority. You are assuming
that if A can read X and B can read X that A can write B.
> This is just
> introducing incorrectness into the design in support of a
> not-actually-helpful security idea.
Where is the incorrectness? Are you seriously saying that
you expect all events to be generated exactly as you think
they should? Have you ever even used systemd?
> If I can read /proc/self/mounts, I can detect changes to my mount
> namespace.
Then read /proc/self/mounts!
Can't you poll on an fd open on /proc/self/mounts?
> Giving me a faster and nicer way to do this is fine, AS
> LONG AS IT ACTUALLY WORKS. "Works" means it needs to detect all
> changes.
So long as "WORKS" includes maintaining the system security
policy, I agree. No, I don't. We already have too many bizarre
and unnatural mechanisms to address whimsical special cases.
If speed is such an issue you could look at making /proc better.
^ permalink raw reply
* Rational model for UID based controls
From: David Howells @ 2019-06-05 16:56 UTC (permalink / raw)
To: Casey Schaufler
Cc: dhowells, Andy Lutomirski, Al Viro, raven, Linux FS Devel,
Linux API, linux-block, keyrings, LSM List, LKML
In-Reply-To: <e4c19d1b-9827-5949-ecb8-6c3cb4648f58@schaufler-ca.com>
Casey Schaufler <casey@schaufler-ca.com> wrote:
> YES!
I'm trying to decide if that's fervour or irritation at this point ;-)
> And it would be really great if you put some thought into what
> a rational model would be for UID based controls, too.
I have put some thought into it, but I don't see a single rational model. It
depends very much on the situation.
In any case, that's what I was referring to when I said I might need to call
inode_permission(). But UIDs don't exist for all filesystems, for example,
and there are no UIDs on superblocks, mount objects or hardware events.
Now, I could see that you ignore UIDs on things like keys and
hardware-triggered events, but how does this interact with things like mount
watches that see directories that have UIDs?
Are you advocating making it such that process B can only see events triggered
by process A if they have the same UID, for example?
David
^ permalink raw reply
* Re: [PATCH 1/2] LSM: switch to blocking policy update notifiers
From: Janne Karhunen @ 2019-06-05 16:51 UTC (permalink / raw)
To: Casey Schaufler
Cc: Stephen Smalley, Mimi Zohar, Paul Moore, linux-integrity,
linux-security-module
In-Reply-To: <9121835b-d6ac-c9d5-349a-1ef7024c6192@schaufler-ca.com>
On Wed, Jun 5, 2019 at 6:23 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > -int call_lsm_notifier(enum lsm_event event, void *data);
> > -int register_lsm_notifier(struct notifier_block *nb);
> > -int unregister_lsm_notifier(struct notifier_block *nb);
> > +int call_blocking_lsm_notifier(enum lsm_event event, void *data);
> > +int register_blocking_lsm_notifier(struct notifier_block *nb);
> > +int unregister_blocking_lsm_notifier(struct notifier_block *nb);
>
> Why is it important to change the names of these hooks?
> It's not like you had call_atomic_lsm_notifier() before.
> It seems like a lot of unnecessary code churn.
Paul was thinking there will eventually be two sets of notifiers
(atomic and blocking) and this creates the clear separation. That's
probably true, but it does indeed create a pretty big change that it
is not really needed yet. I'm fine either way.
--
Janne
^ permalink raw reply
* Re: [RFC 1/1] Add dm verity root hash pkcs7 sig validation.
From: Sasha Levin @ 2019-06-05 16:42 UTC (permalink / raw)
To: Jaskaran Khurana
Cc: linux-security-module, linux-kernel, agk, snitzer, dm-devel,
jmorris
In-Reply-To: <20190520215422.23939-2-jaskarankhurana@linux.microsoft.com>
On Mon, May 20, 2019 at 02:54:22PM -0700, Jaskaran Khurana wrote:
>Adds in-kernel pkcs7 signature checking for the roothash of
>the dm-verity hash tree.
>
>The verification is to support cases where the roothash is not secured by
>Trusted Boot, UEFI Secureboot or similar technologies.
>One of the use cases for this is for dm-verity volumes mounted after boot,
>the root hash provided during the creation of the dm-verity volume has to
>be secure and thus in-kernel validation implemented here will be used
>before we trust the root hash and allow the block device to be created.
>
>The signature being provided for verification must verify the root hash and
>must be trusted by the builtin keyring for verification to succeed.
>
>Adds DM_VERITY_VERIFY_ROOTHASH_SIG: roothash verification
>against the roothash signature file *if* specified, if signature file is
>specified verification must succeed prior to creation of device mapper
>block device.
>
>Adds DM_VERITY_VERIFY_ROOTHASH_SIG_FORCE: roothash signature *must* be
>specified for all dm verity volumes and verification must succeed prior
>to creation of device mapper block device.
>
>Signed-off-by: Jaskaran Khurana <jaskarankhurana@linux.microsoft.com>
>---
> drivers/md/Kconfig | 23 ++++++
> drivers/md/Makefile | 2 +-
> drivers/md/dm-verity-target.c | 44 ++++++++--
> drivers/md/dm-verity-verify-sig.c | 129 ++++++++++++++++++++++++++++++
> drivers/md/dm-verity-verify-sig.h | 32 ++++++++
> 5 files changed, 222 insertions(+), 8 deletions(-)
> create mode 100644 drivers/md/dm-verity-verify-sig.c
> create mode 100644 drivers/md/dm-verity-verify-sig.h
>
>diff --git a/drivers/md/Kconfig b/drivers/md/Kconfig
>index db269a348b20..da4115753f25 100644
>--- a/drivers/md/Kconfig
>+++ b/drivers/md/Kconfig
>@@ -489,6 +489,29 @@ config DM_VERITY
>
> If unsure, say N.
>
>+config DM_VERITY_VERIFY_ROOTHASH_SIG
>+ def_bool n
>+ bool "Verity data device root hash signature verification support"
>+ depends on DM_VERITY
>+ select SYSTEM_DATA_VERIFICATION
>+ help
>+ The device mapper target created by DM-VERITY can be validated if the
>+ pre-generated tree of cryptographic checksums passed has a pkcs#7
>+ signature file that can validate the roothash of the tree.
>+
>+ If unsure, say N.
>+
>+config DM_VERITY_VERIFY_ROOTHASH_SIG_FORCE
>+ def_bool n
>+ bool "Forces all dm verity data device root hash should be signed"
>+ depends on DM_VERITY_VERIFY_ROOTHASH_SIG
>+ help
>+ The device mapper target created by DM-VERITY will succeed only if the
>+ pre-generated tree of cryptographic checksums passed also has a pkcs#7
>+ signature file that can validate the roothash of the tree.
>+
>+ If unsure, say N.
>+
> config DM_VERITY_FEC
> bool "Verity forward error correction support"
> depends on DM_VERITY
>diff --git a/drivers/md/Makefile b/drivers/md/Makefile
>index be7a6eb92abc..8a8c142bcfe1 100644
>--- a/drivers/md/Makefile
>+++ b/drivers/md/Makefile
>@@ -61,7 +61,7 @@ obj-$(CONFIG_DM_LOG_USERSPACE) += dm-log-userspace.o
> obj-$(CONFIG_DM_ZERO) += dm-zero.o
> obj-$(CONFIG_DM_RAID) += dm-raid.o
> obj-$(CONFIG_DM_THIN_PROVISIONING) += dm-thin-pool.o
>-obj-$(CONFIG_DM_VERITY) += dm-verity.o
>+obj-$(CONFIG_DM_VERITY) += dm-verity.o dm-verity-verify-sig.o
Can we avoid building dm-verity-verify-sig.o when
DM_VERITY_VERIFY_ROOTHASH_SIG is not set?
> obj-$(CONFIG_DM_CACHE) += dm-cache.o
> obj-$(CONFIG_DM_CACHE_SMQ) += dm-cache-smq.o
> obj-$(CONFIG_DM_ERA) += dm-era.o
>diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
>index f4c31ffaa88e..53aebfa8bc38 100644
>--- a/drivers/md/dm-verity-target.c
>+++ b/drivers/md/dm-verity-target.c
>@@ -16,7 +16,7 @@
>
> #include "dm-verity.h"
> #include "dm-verity-fec.h"
>-
>+#include "dm-verity-verify-sig.h"
> #include <linux/module.h>
> #include <linux/reboot.h>
>
>@@ -34,7 +34,11 @@
> #define DM_VERITY_OPT_IGN_ZEROES "ignore_zero_blocks"
> #define DM_VERITY_OPT_AT_MOST_ONCE "check_at_most_once"
>
>-#define DM_VERITY_OPTS_MAX (2 + DM_VERITY_OPTS_FEC)
>+#define DM_VERITY_OPTS_MAX (2 + DM_VERITY_OPTS_FEC + \
>+ DM_VERITY_ROOT_HASH_VERIFICATION_OPTS)
It would be nice if DM_VERITY_OPTS_MAX would remain the same size as it
is now if DM_VERITY_VERIFY_ROOTHASH_SIG is not enabled.
>+#define DM_VERITY_MANDATORY_ARGS 10
This cleanup should be in a separate patch.
>+
>
> static unsigned dm_verity_prefetch_cluster = DM_VERITY_DEFAULT_PREFETCH_SIZE;
>
>@@ -855,7 +859,8 @@ static int verity_alloc_zero_digest(struct dm_verity *v)
> return r;
> }
>
>-static int verity_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v)
>+static int verity_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v,
>+ struct dm_verity_sig_opts *verify_args)
> {
> int r;
> unsigned argc;
>@@ -904,6 +909,15 @@ static int verity_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v)
> if (r)
> return r;
> continue;
>+ } else if (verity_verify_is_sig_opt_arg(arg_name)) {
>+ r = verity_verify_sig_parse_opt_args(as, v,
>+ verify_args,
>+ &argc, arg_name);
Hm, I don't see empty verity_verify_is_sig_opt_arg() or
verity_verify_sig_parse_opt_args() for when this config option is
disabled.
>+ if (r) {
>+ ti->error = "Could not parse the sig args";
>+ return r;
>+ }
>+ continue;
> }
>
> ti->error = "Unrecognized verity feature request";
>@@ -930,6 +944,7 @@ static int verity_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v)
> static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
> {
> struct dm_verity *v;
>+ struct dm_verity_sig_opts verify_args = {0};
> struct dm_arg_set as;
> unsigned int num;
> unsigned long long num_ll;
>@@ -937,6 +952,7 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
> int i;
> sector_t hash_position;
> char dummy;
>+ char *root_hash_digest_to_validate = NULL;
>
> v = kzalloc(sizeof(struct dm_verity), GFP_KERNEL);
> if (!v) {
>@@ -956,7 +972,7 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
> goto bad;
> }
>
>- if (argc < 10) {
>+ if (argc < DM_VERITY_MANDATORY_ARGS) {
> ti->error = "Not enough arguments";
> r = -EINVAL;
> goto bad;
>@@ -1070,6 +1086,7 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
> r = -EINVAL;
> goto bad;
> }
>+ root_hash_digest_to_validate = argv[8];
Can we avoid magic numbers?
>
> if (strcmp(argv[9], "-")) {
> v->salt_size = strlen(argv[9]) / 2;
>@@ -1087,19 +1104,28 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
> }
> }
>
>- argv += 10;
>- argc -= 10;
>+ argv += DM_VERITY_MANDATORY_ARGS;
>+ argc -= DM_VERITY_MANDATORY_ARGS;
>
> /* Optional parameters */
> if (argc) {
> as.argc = argc;
> as.argv = argv;
>
>- r = verity_parse_opt_args(&as, v);
>+ r = verity_parse_opt_args(&as, v, &verify_args);
> if (r < 0)
> goto bad;
> }
>
>+ /* Root hash signature is a optional parameter*/
an
>+ r = verity_verify_root_hash(root_hash_digest_to_validate,
>+ strlen(root_hash_digest_to_validate),
>+ verify_args.sig,
>+ verify_args.sig_size);
>+ if (r < 0) {
>+ ti->error = "Root hash verification failed";
>+ goto bad;
>+ }
> v->hash_per_block_bits =
> __fls((1 << v->hash_dev_block_bits) / v->digest_size);
>
>@@ -1165,9 +1191,13 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
> ti->per_io_data_size = roundup(ti->per_io_data_size,
> __alignof__(struct dm_verity_io));
>
>+ verity_verify_sig_opts_cleanup(&verify_args);
>+
> return 0;
>
> bad:
>+
>+ verity_verify_sig_opts_cleanup(&verify_args);
> verity_dtr(ti);
>
> return r;
>diff --git a/drivers/md/dm-verity-verify-sig.c b/drivers/md/dm-verity-verify-sig.c
>new file mode 100644
>index 000000000000..491c84eb58ef
>--- /dev/null
>+++ b/drivers/md/dm-verity-verify-sig.c
>@@ -0,0 +1,129 @@
>+// SPDX-License-Identifier: GPL-2.0
>+/*
>+ * Copyright (C) 2019 Microsoft Corporation.
>+ *
>+ * Author: Jaskaran Singh Khurana <jaskarankhurana@linux.microsoft.com>
>+ *
>+ * This file is released under the GPLv2.
We don't need to state this, that's why we have SPDX.
>+ */
>+#include <linux/device-mapper.h>
>+#include <linux/verification.h>
>+#include "dm-verity.h"
>+#include "dm-verity-verify-sig.h"
>+
>+#define DM_VERITY_VERIFY_ERR(s) DM_VERITY_ROOT_HASH_VERIFICATION " " s
>+
>+bool verity_verify_is_sig_opt_arg(const char *arg_name)
>+{
>+ return (!strcasecmp(arg_name,
>+ DM_VERITY_ROOT_HASH_VERIFICATION_OPT_SIG));
>+}
>+EXPORT_SYMBOL_GPL(verity_verify_is_sig_opt_arg);
For whom are all these symbols exported for?
>+
>+int verity_verify_sig_parse_opt_args(struct dm_arg_set *as,
>+ struct dm_verity *v,
>+ struct dm_verity_sig_opts *sig_opts,
>+ unsigned int *argc,
>+ const char *arg_name)
>+{
>+ const char *sig_size;
>+ const char *sig_buf;
>+ char dummy;
>+ struct dm_target *ti = v->ti;
>+ int r = 0;
>+
>+ if (*argc < DM_VERITY_ROOT_HASH_VERIFICATION_OPTS - 1) {
>+ ti->error = DM_VERITY_VERIFY_ERR("sig values not specified");
>+ return -EINVAL;
>+ }
>+
>+ sig_size = dm_shift_arg(as);
>+ (*argc)--;
>+
>+ if (strcasecmp(arg_name, DM_VERITY_ROOT_HASH_VERIFICATION_OPT_SIG) ||
>+ sscanf(sig_size, "%u%c",
>+ &sig_opts->sig_size, &dummy) != 1) {
>+ ti->error = DM_VERITY_VERIFY_ERR("invalid signature size");
>+ return -EINVAL;
>+ }
>+
>+ sig_buf = dm_shift_arg(as);
>+ (*argc)--;
>+
>+ if (strlen(sig_buf) != sig_opts->sig_size * 2) {
>+ ti->error = DM_VERITY_VERIFY_ERR("sig buffer, size: mismatch");
>+ return -EINVAL;
>+ }
>+
>+ sig_opts->sig = kmalloc(sig_opts->sig_size, GFP_KERNEL);
>+ if (!sig_opts->sig) {
>+ r = -ENOMEM;
>+ goto end;
Why not return directly here?
>+ }
>+
>+ r = hex2bin(sig_opts->sig, sig_buf, sig_opts->sig_size);
>+
>+ if (r < 0) {
>+ ti->error = DM_VERITY_VERIFY_ERR("invalid roothash sig buf");
>+ r = -EINVAL;
>+ goto end;
We'll get to 'end' anyway.
>+ }
>+
>+end:
>+ if (r < 0)
>+ verity_verify_sig_opts_cleanup(sig_opts);
>+ return r;
>+}
>+EXPORT_SYMBOL_GPL(verity_verify_sig_parse_opt_args);
>+
>+#ifdef CONFIG_DM_VERITY_VERIFY_ROOTHASH_SIG
>+/*
>+ * verify_verify_roothash - Verify the root hash of the verity hash device
>+ * using builtin trusted keys.
>+ *
>+ * @root_hash: For verity, the roothash/data to be verified.
>+ * @root_hash_len: Size of the roothash/data to be verified.
>+ * @sig_data: The trusted signature that verifies the roothash/data.
>+ * @sig_len: Size of the signature.
>+ *
>+ */
>+int verity_verify_root_hash(const void *root_hash, size_t root_hash_len,
>+ const void *sig_data, size_t sig_len)
>+{
>+ int r;
>+
>+ if (!root_hash || root_hash_len == 0)
>+ return -EINVAL;
>+
>+ if (!sig_data || sig_len == 0) {
>+ if (IS_ENABLED(CONFIG_DM_VERITY_VERIFY_ROOTHASH_SIG_FORCE))
>+ return -EINVAL;
I'm not sure -EINVAL is the right failure here.
>+ else
>+ return 0;
>+ }
>+
>+ r = verify_pkcs7_signature(root_hash, root_hash_len, sig_data, sig_len,
>+ NULL, VERIFYING_UNSPECIFIED_SIGNATURE, NULL,
>+ NULL);
>+ if (r < 0)
>+ goto end;
This statement makes no sense :)
>+
>+end:
>+ return r;
>+}
>+#else
>+int verity_verify_root_hash(const void *root_hash, size_t root_hash_len,
>+ const void *sig_data, size_t sig_len)
>+{
>+ return 0;
>+}
>+#endif
>+EXPORT_SYMBOL_GPL(verity_verify_root_hash);
>+void verity_verify_sig_opts_cleanup(struct dm_verity_sig_opts *sig_opts)
Why doesn't this live somewhere inside verity_dtr()?
--
Thanks,
Sasha
^ permalink raw reply
* Re: [RFC][PATCH 0/8] Mount, FS, Block and Keyrings notifications [ver #2]
From: Andy Lutomirski @ 2019-06-05 16:04 UTC (permalink / raw)
To: Casey Schaufler
Cc: David Howells, Andy Lutomirski, Al Viro, raven, Linux FS Devel,
Linux API, linux-block, keyrings, LSM List, LKML
In-Reply-To: <e4c19d1b-9827-5949-ecb8-6c3cb4648f58@schaufler-ca.com>
On Wed, Jun 5, 2019 at 7:51 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> On 6/5/2019 1:41 AM, David Howells wrote:
> > Casey Schaufler <casey@schaufler-ca.com> wrote:
> >
> >> I will try to explain the problem once again. If process A
> >> sends a signal (writes information) to process B the kernel
> >> checks that either process A has the same UID as process B
> >> or that process A has privilege to override that policy.
> >> Process B is passive in this access control decision, while
> >> process A is active. In the event delivery case, process A
> >> does something (e.g. modifies a keyring) that generates an
> >> event, which is then sent to process B's event buffer.
> > I think this might be the core sticking point here. It looks like two
> > different situations:
> >
> > (1) A explicitly sends event to B (eg. signalling, sendmsg, etc.)
> >
> > (2) A implicitly and unknowingly sends event to B as a side effect of some
> > other action (eg. B has a watch for the event A did).
> >
> > The LSM treats them as the same: that is B must have MAC authorisation to send
> > a message to A.
>
> YES!
>
> Threat is about what you can do, not what you intend to do.
>
> And it would be really great if you put some thought into what
> a rational model would be for UID based controls, too.
>
> > But there are problems with not sending the event:
> >
> > (1) B's internal state is then corrupt (or, at least, unknowingly invalid).
>
> Then B is a badly written program.
Either I'm misunderstanding you or I strongly disagree. If B has
authority to detect a certain action, and A has authority to perform
that action, then refusing to notify B because B is somehow missing
some special authorization to be notified by A is nuts. This is just
introducing incorrectness into the design in support of a
not-actually-helpful security idea.
If I can read /proc/self/mounts, I can detect changes to my mount
namespace. Giving me a faster and nicer way to do this is fine, AS
LONG AS IT ACTUALLY WORKS. "Works" means it needs to detect all
changes.
^ permalink raw reply
* Re: [PATCH 1/2] LSM: switch to blocking policy update notifiers
From: Casey Schaufler @ 2019-06-05 15:23 UTC (permalink / raw)
To: Janne Karhunen, sds, zohar, paul; +Cc: linux-integrity, linux-security-module
In-Reply-To: <20190605083606.4209-1-janne.karhunen@gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 5714 bytes --]
On 6/5/2019 1:36 AM, Janne Karhunen wrote:
> Atomic policy updaters are not very useful as they cannot
> usually perform the policy updates on their own. Since it
> seems that there is no strict need for the atomicity,
> switch to the blocking variant. While doing so, rename
> the functions accordingly.
>
> Signed-off-by: Janne Karhunen <janne.karhunen@gmail.com>
> ---
> drivers/infiniband/core/device.c | 6 +++---
> include/linux/security.h | 6 +++---
> security/security.c | 23 +++++++++++++----------
> security/selinux/hooks.c | 2 +-
> security/selinux/selinuxfs.c | 2 +-
> 5 files changed, 21 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
> index 78dc07c6ac4b..61c0c93a2e73 100644
> --- a/drivers/infiniband/core/device.c
> +++ b/drivers/infiniband/core/device.c
> @@ -2499,7 +2499,7 @@ static int __init ib_core_init(void)
> goto err_mad;
> }
>
> - ret = register_lsm_notifier(&ibdev_lsm_nb);
> + ret = register_blocking_lsm_notifier(&ibdev_lsm_nb);
> if (ret) {
> pr_warn("Couldn't register LSM notifier. ret %d\n", ret);
> goto err_sa;
> @@ -2518,7 +2518,7 @@ static int __init ib_core_init(void)
> return 0;
>
> err_compat:
> - unregister_lsm_notifier(&ibdev_lsm_nb);
> + unregister_blocking_lsm_notifier(&ibdev_lsm_nb);
> err_sa:
> ib_sa_cleanup();
> err_mad:
> @@ -2544,7 +2544,7 @@ static void __exit ib_core_cleanup(void)
> nldev_exit();
> rdma_nl_unregister(RDMA_NL_LS);
> unregister_pernet_device(&rdma_dev_net_ops);
> - unregister_lsm_notifier(&ibdev_lsm_nb);
> + unregister_blocking_lsm_notifier(&ibdev_lsm_nb);
> ib_sa_cleanup();
> ib_mad_cleanup();
> addr_cleanup();
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 659071c2e57c..fc655fbe44ad 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -189,9 +189,9 @@ static inline const char *kernel_load_data_id_str(enum kernel_load_data_id id)
>
> #ifdef CONFIG_SECURITY
>
> -int call_lsm_notifier(enum lsm_event event, void *data);
> -int register_lsm_notifier(struct notifier_block *nb);
> -int unregister_lsm_notifier(struct notifier_block *nb);
> +int call_blocking_lsm_notifier(enum lsm_event event, void *data);
> +int register_blocking_lsm_notifier(struct notifier_block *nb);
> +int unregister_blocking_lsm_notifier(struct notifier_block *nb);
Why is it important to change the names of these hooks?
It's not like you had call_atomic_lsm_notifier() before.
It seems like a lot of unnecessary code churn.
>
> /* prototypes */
> extern int security_init(void);
> diff --git a/security/security.c b/security/security.c
> index c01a88f65ad8..6bfc7636ddb7 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -39,7 +39,7 @@
> #define LSM_COUNT (__end_lsm_info - __start_lsm_info)
>
> struct security_hook_heads security_hook_heads __lsm_ro_after_init;
> -static ATOMIC_NOTIFIER_HEAD(lsm_notifier_chain);
> +static BLOCKING_NOTIFIER_HEAD(blocking_lsm_notifier_chain);
>
> static struct kmem_cache *lsm_file_cache;
> static struct kmem_cache *lsm_inode_cache;
> @@ -430,23 +430,26 @@ void __init security_add_hooks(struct security_hook_list *hooks, int count,
> panic("%s - Cannot get early memory.\n", __func__);
> }
>
> -int call_lsm_notifier(enum lsm_event event, void *data)
> +int call_blocking_lsm_notifier(enum lsm_event event, void *data)
> {
> - return atomic_notifier_call_chain(&lsm_notifier_chain, event, data);
> + return blocking_notifier_call_chain(&blocking_lsm_notifier_chain,
> + event, data);
> }
> -EXPORT_SYMBOL(call_lsm_notifier);
> +EXPORT_SYMBOL(call_blocking_lsm_notifier);
>
> -int register_lsm_notifier(struct notifier_block *nb)
> +int register_blocking_lsm_notifier(struct notifier_block *nb)
> {
> - return atomic_notifier_chain_register(&lsm_notifier_chain, nb);
> + return blocking_notifier_chain_register(&blocking_lsm_notifier_chain,
> + nb);
> }
> -EXPORT_SYMBOL(register_lsm_notifier);
> +EXPORT_SYMBOL(register_blocking_lsm_notifier);
>
> -int unregister_lsm_notifier(struct notifier_block *nb)
> +int unregister_blocking_lsm_notifier(struct notifier_block *nb)
> {
> - return atomic_notifier_chain_unregister(&lsm_notifier_chain, nb);
> + return blocking_notifier_chain_unregister(&blocking_lsm_notifier_chain,
> + nb);
> }
> -EXPORT_SYMBOL(unregister_lsm_notifier);
> +EXPORT_SYMBOL(unregister_blocking_lsm_notifier);
>
> /**
> * lsm_cred_alloc - allocate a composite cred blob
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index c61787b15f27..c1e37018c8eb 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -197,7 +197,7 @@ static int selinux_lsm_notifier_avc_callback(u32 event)
> {
> if (event == AVC_CALLBACK_RESET) {
> sel_ib_pkey_flush();
> - call_lsm_notifier(LSM_POLICY_CHANGE, NULL);
> + call_blocking_lsm_notifier(LSM_POLICY_CHANGE, NULL);
> }
>
> return 0;
> diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> index 145ee62f205a..1e2e3e4b5fdb 100644
> --- a/security/selinux/selinuxfs.c
> +++ b/security/selinux/selinuxfs.c
> @@ -180,7 +180,7 @@ static ssize_t sel_write_enforce(struct file *file, const char __user *buf,
> selnl_notify_setenforce(new_value);
> selinux_status_update_setenforce(state, new_value);
> if (!new_value)
> - call_lsm_notifier(LSM_POLICY_CHANGE, NULL);
> + call_blocking_lsm_notifier(LSM_POLICY_CHANGE, NULL);
> }
> length = count;
> out:
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [RFC PATCH 2/9] x86/sgx: Do not naturally align MAP_FIXED address
From: Jarkko Sakkinen @ 2019-06-05 15:17 UTC (permalink / raw)
To: Xing, Cedric
Cc: Andy Lutomirski, Christopherson, Sean J, Stephen Smalley,
James Morris, Serge E . Hallyn, LSM List, Paul Moore, Eric Paris,
selinux@vger.kernel.org, Jethro Beekman, Hansen, Dave,
Thomas Gleixner, Linus Torvalds, LKML, X86 ML,
linux-sgx@vger.kernel.org, Andrew Morton, nhorman@redhat.com,
npmccallum@redhat.com, Ayoun, Serge, Katz-zamir, Shay,
Huang, Haitao, Andy Shevchenko, Svahn, Kai, Borislav Petkov,
Josh Triplett, Huang, Kai, David Rientjes, Roberts, William C,
Tricca, Philip B
In-Reply-To: <960B34DE67B9E140824F1DCDEC400C0F654EDBDE@ORSMSX116.amr.corp.intel.com>
On Tue, Jun 04, 2019 at 10:10:22PM +0000, Xing, Cedric wrote:
> A bit off topic here. This mmap()/mprotect() discussion reminds me a
> question (guess for Jarkko): Now that vma->vm_file->private_data keeps
> a pointer to the enclave, why do we store it again in vma->vm_private?
> It isn't a big deal but non-NULL vm_private does prevent mprotect()
> from merging adjacent VMAs.
Same semantics as with a regular mmap i.e. you can close the file and
still use the mapping.
/Jarkko
^ permalink raw reply
* Re: [RFC PATCH 2/9] x86/sgx: Do not naturally align MAP_FIXED address
From: Jarkko Sakkinen @ 2019-06-05 15:15 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Sean Christopherson, Cedric Xing, Stephen Smalley, James Morris,
Serge E . Hallyn, LSM List, Paul Moore, Eric Paris, selinux,
Jethro Beekman, Dave Hansen, Thomas Gleixner, Linus Torvalds,
LKML, X86 ML, linux-sgx, Andrew Morton, nhorman, npmccallum,
Serge Ayoun, Shay Katz-zamir, Haitao Huang, Andy Shevchenko,
Kai Svahn, Borislav Petkov, Josh Triplett, Kai Huang,
David Rientjes, William Roberts, Philip Tricca
In-Reply-To: <CALCETrVe0jhAWAFmx+NFEjJcijSJv2LDVC7cUXi0w99kNKjh_g@mail.gmail.com>
On Tue, Jun 04, 2019 at 01:16:04PM -0700, Andy Lutomirski wrote:
> On Tue, Jun 4, 2019 at 4:50 AM Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com> wrote:
> >
> > On Fri, May 31, 2019 at 04:31:52PM -0700, Sean Christopherson wrote:
> > > SGX enclaves have an associated Enclave Linear Range (ELRANGE) that is
> > > tracked and enforced by the CPU using a base+mask approach, similar to
> > > how hardware range registers such as the variable MTRRs. As a result,
> > > the ELRANGE must be naturally sized and aligned.
> > >
> > > To reduce boilerplate code that would be needed in every userspace
> > > enclave loader, the SGX driver naturally aligns the mmap() address and
> > > also requires the range to be naturally sized. Unfortunately, SGX fails
> > > to grant a waiver to the MAP_FIXED case, e.g. incorrectly rejects mmap()
> > > if userspace is attempting to map a small slice of an existing enclave.
> > >
> > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> >
> > Why you want to allow mmap() to be called multiple times? mmap() could
> > be allowed only once with PROT_NONE and denied afterwards. Is this for
> > sending fd to another process that would map already existing enclave?
> >
> > I don't see any checks for whether the is enclave underneath. Also, I
> > think that in all cases mmap() callback should allow only PROT_NONE
> > as permissions for clarity even if it could called multiple times.
> >
>
> What's the advantage to only allowing PROT_NONE? The idea here is to
> allow a PROT_NONE map followed by some replacemets that overlay it for
> the individual segments. Admittedly, mprotect() can do the same
> thing, but disallowing mmap() seems at least a bit surprising.
I was merely wondering if it is specifically for the application where a
client process would mmap(MAP_FIXED) an enclave created by a server
process.
/Jarkko
^ permalink raw reply
* Re: [RFC PATCH 7/9] x86/sgx: Enforce noexec filesystem restriction for enclaves
From: Jarkko Sakkinen @ 2019-06-05 15:10 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Sean Christopherson, Cedric Xing, Stephen Smalley, James Morris,
Serge E . Hallyn, LSM List, Paul Moore, Eric Paris, selinux,
Jethro Beekman, Dave Hansen, Thomas Gleixner, Linus Torvalds,
LKML, X86 ML, linux-sgx, Andrew Morton, nhorman, npmccallum,
Serge Ayoun, Shay Katz-zamir, Haitao Huang, Andy Shevchenko,
Kai Svahn, Borislav Petkov, Josh Triplett, Kai Huang,
David Rientjes, William Roberts, Philip Tricca
In-Reply-To: <CALCETrUqcQNbRvBe2UqQih8RHnuwn3KaC=xJU1cRsaEVsCQUgw@mail.gmail.com>
On Tue, Jun 04, 2019 at 01:25:10PM -0700, Andy Lutomirski wrote:
> On Tue, Jun 4, 2019 at 9:26 AM Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com> wrote:
> >
> > On Fri, May 31, 2019 at 04:31:57PM -0700, Sean Christopherson wrote:
> > > Do not allow an enclave page to be mapped with PROT_EXEC if the source
> > > page is backed by a file on a noexec file system.
> > >
> > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> >
> > Why don't you just check in sgx_encl_add_page() that whether the path
> > comes from noexec and deny if SECINFO contains X?
> >
>
> SECINFO seems almost entirely useless for this kind of thing because
> of SGX2. I'm thinking that SECINFO should be completely ignored for
> anything other than its required architectural purpose.
Not exactly sure why using it to pass the RWX bits to EADD ioctl would
cause anything to SGX2 support.
/Jarkko
^ permalink raw reply
* Re: [RFC PATCH 6/9] x86/sgx: Require userspace to provide allowed prots to ADD_PAGES
From: Jarkko Sakkinen @ 2019-06-05 15:06 UTC (permalink / raw)
To: Sean Christopherson
Cc: Andy Lutomirski, Cedric Xing, Stephen Smalley, James Morris,
Serge E . Hallyn, LSM List, Paul Moore, Eric Paris, selinux,
Jethro Beekman, Dave Hansen, Thomas Gleixner, Linus Torvalds,
LKML, X86 ML, linux-sgx, Andrew Morton, nhorman, npmccallum,
Serge Ayoun, Shay Katz-zamir, Haitao Huang, Andy Shevchenko,
Kai Svahn, Borislav Petkov, Josh Triplett, Kai Huang,
David Rientjes, William Roberts, Philip Tricca
In-Reply-To: <20190604164514.GB32350@linux.intel.com>
On Tue, Jun 04, 2019 at 09:45:14AM -0700, Sean Christopherson wrote:
> Heh, yeah, it's not duplicating LSM functionality. What I was trying to
> say is that this patch allows LSMs to implement policies that are
> equivalent to their existing functionality, e.g. paves the way to add
> security_enclave_load() as an equivalent to security_file_mprotect().
I would suggest describing explicitly in the commit message what you
want to do, which you said here e.g. "I do this because I want to add
LSM hooks". This also relevant information for the LKM discussion.
Lets see how the next version looks like now that you have some
feedback.
In the whole scope of the patch set, in order to make it more
readable, I'll give following suggestions on how it is organized:
1. Leave out anything that is not strictly necessary (cosmetic
fix, batch operation if possible). Better to focus one thing at
a time.
2. Try to organize it so that each function is fully defined in
the scope of one patch even if it would mean larger patches.
3. Do not add one call site helpers unless there is a good
reason to do so. A good reason would be something like needing
to extensive work in error rollback, which would make the
caller a mess.
/Jarkko
^ permalink raw reply
* Re: [RFC][PATCH 0/8] Mount, FS, Block and Keyrings notifications [ver #2]
From: Casey Schaufler @ 2019-06-05 14:50 UTC (permalink / raw)
To: David Howells
Cc: Andy Lutomirski, Al Viro, raven, Linux FS Devel, Linux API,
linux-block, keyrings, LSM List, LKML, casey
In-Reply-To: <20192.1559724094@warthog.procyon.org.uk>
[-- Attachment #1.1: Type: text/plain, Size: 4255 bytes --]
On 6/5/2019 1:41 AM, David Howells wrote:
> Casey Schaufler <casey@schaufler-ca.com> wrote:
>
>> I will try to explain the problem once again. If process A
>> sends a signal (writes information) to process B the kernel
>> checks that either process A has the same UID as process B
>> or that process A has privilege to override that policy.
>> Process B is passive in this access control decision, while
>> process A is active. In the event delivery case, process A
>> does something (e.g. modifies a keyring) that generates an
>> event, which is then sent to process B's event buffer.
> I think this might be the core sticking point here. It looks like two
> different situations:
>
> (1) A explicitly sends event to B (eg. signalling, sendmsg, etc.)
>
> (2) A implicitly and unknowingly sends event to B as a side effect of some
> other action (eg. B has a watch for the event A did).
>
> The LSM treats them as the same: that is B must have MAC authorisation to send
> a message to A.
YES!
Threat is about what you can do, not what you intend to do.
And it would be really great if you put some thought into what
a rational model would be for UID based controls, too.
> But there are problems with not sending the event:
>
> (1) B's internal state is then corrupt (or, at least, unknowingly invalid).
Then B is a badly written program.
> (2) B can potentially figure out that the event happened by other means.
Then why does it need the event mechanism in the first place?
> I've implemented four event sources so far:
>
> (1) Keys/keyrings. You can only get events on a key you have View permission
> on and the other process has to have write access to it, so I think this
> is good enough.
Sounds fine.
> (2) Block layer. Currently this will only get you hardware error events,
> which is probably safe. I'm not sure you can manipulate those without
> permission to directly access the device files.
There's an argument to be made that this should require CAP_SYS_ADMIN,
or that an LSM like SELinux might include hardware error events in
policy, but generally I agree that system generated events like this
are both harmless and pointless for the general public to watch.
> (3) Superblock. This is trickier since it can see events that can be
> manufactured (R/W <-> R/O remounting, EDQUOT) as well as events that
> can't without hardware control (EIO, network link loss, RF kill).
The events generated by processes (the 1st set) need controls
like keys. The events generated by the system (the 2nd set) may
need controls like the block layer.
> (4) Mount topology. This is the trickiest since it allows you to see events
> beyond the point at which you placed your watch (in essence, you place a
> subtree watch).
Like keys.
> The question is what permission checking should I do? Ideally, I'd
> emulate a pathwalk between the watchpoint and the eventing object to see
> if the owner of the watchpoint could reach it.
That will depend, as I've been saying, on what causes
the event to be generated. If it's from a process, the
question is "can the active process, the one that generated
the event, write to the passive, watching process?"
If it's the system on a hardware event, you may want the watcher
to have CAP_SYS_ADMIN.
> I'd need to do a reverse walk, calling inode_permission(MAY_NOT_BLOCK)
> for each directory between the eventing object and the watchpoint to see
> if one rejects it - but some filesystems have a permission check that
> can't be called in this state.
This is for setting the watch, right?
> It would also be necessary to do this separately for each watchpoint in
> the parental chain.
>
> Further, each permissions check would generate an audit event and could
> generate FAN_ACCESS and/or FAN_ACCESS_PERM fanotify events - which could
> be a problem if fanotify is also trying to post those events to the same
> watch queue.
If you required that the watching process open(dir) what
you want to watch you'd get this for free. Or did I miss
something obvious?
> David
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [RFC PATCH 2/9] x86/sgx: Do not naturally align MAP_FIXED address
From: Sean Christopherson @ 2019-06-05 14:08 UTC (permalink / raw)
To: Xing, Cedric
Cc: Andy Lutomirski, Jarkko Sakkinen, Stephen Smalley, James Morris,
Serge E . Hallyn, LSM List, Paul Moore, Eric Paris,
selinux@vger.kernel.org, Jethro Beekman, Hansen, Dave,
Thomas Gleixner, Linus Torvalds, LKML, X86 ML,
linux-sgx@vger.kernel.org, Andrew Morton, nhorman@redhat.com,
npmccallum@redhat.com, Ayoun, Serge, Katz-zamir, Shay,
Huang, Haitao, Andy Shevchenko, Svahn, Kai, Borislav Petkov,
Josh Triplett, Huang, Kai, David Rientjes, Roberts, William C,
Tricca, Philip B
In-Reply-To: <960B34DE67B9E140824F1DCDEC400C0F654EDBDE@ORSMSX116.amr.corp.intel.com>
On Tue, Jun 04, 2019 at 03:10:22PM -0700, Xing, Cedric wrote:
> A bit off topic here. This mmap()/mprotect() discussion reminds me a question
> (guess for Jarkko): Now that vma->vm_file->private_data keeps a pointer to
> the enclave, why do we store it again in vma->vm_private? It isn't a big deal
> but non-NULL vm_private does prevent mprotect() from merging adjacent VMAs.
vma->vm_ops->close also prevents merging, and we need that to refcount the
enclave and mm.
We also rely on nullifying vma->vm_private_data in the unlikely event that
adding a new mm to the enclave fails on kzalloc().
^ permalink raw reply
* Re: [RFC][PATCH 0/8] Mount, FS, Block and Keyrings notifications [ver #2]
From: Stephen Smalley @ 2019-06-05 13:47 UTC (permalink / raw)
To: Andy Lutomirski, Stephen Smalley
Cc: David Howells, Al Viro, Casey Schaufler, raven, Linux FS Devel,
Linux API, linux-block, keyrings, LSM List, LKML, Paul Moore
In-Reply-To: <CALCETrU_5djawkwW-GRyHZXHwOUjaei1Cp7NEJaVFDm_bK6G3w@mail.gmail.com>
On 6/5/19 12:19 AM, Andy Lutomirski wrote:
> On Tue, Jun 4, 2019 at 6:18 PM Stephen Smalley
> <stephen.smalley@gmail.com> wrote:
>>
>> On Tue, Jun 4, 2019 at 4:58 PM Andy Lutomirski <luto@kernel.org> wrote:
>>>
>>> On Tue, Jun 4, 2019 at 1:39 PM David Howells <dhowells@redhat.com> wrote:
>>>>
>>>> Andy Lutomirski <luto@kernel.org> wrote:
>>>>
>>>>>> Here's a set of patches to add a general variable-length notification queue
>>>>>> concept and to add sources of events for:
>>>>>
>>>>> I asked before and didn't see a response, so I'll ask again. Why are you
>>>>> paying any attention at all to the creds that generate an event?
>>>>
>>>> Casey responded to you. It's one of his requirements.
>>>>
>>>
>>> It being a "requirement" doesn't make it okay.
>>>
>>>> However, the LSMs (or at least SELinux) ignore f_cred and use current_cred()
>>>> when checking permissions. See selinux_revalidate_file_permission() for
>>>> example - it uses current_cred() not file->f_cred to re-evaluate the perms,
>>>> and the fd might be shared between a number of processes with different creds.
>>>
>>> That's a bug. It's arguably a rather severe bug. If I ever get
>>> around to writing the patch I keep thinking of that will warn if we
>>> use creds from invalid contexts, it will warn.
>>
>>
>> No, not a bug. Working as designed. Initial validation on open, but revalidation upon read/write if something has changed since open (process SID differs from opener, inode SID has changed, policy has changed). Current subject SID should be used for the revalidation. It's a MAC vs DAC difference.
>>
>
> Can you explain how the design is valid, then? Consider nasty cases like this:
>
> $ sudo -u lotsofgarbage 2>/dev/whatever
(sorry for the previous html email; gmail or my inability to properly
use it strikes again!)
Here we have four (or more) opportunities to say no:
1) Upon selinux_inode_permission(), when checking write access to
/dev/whatever in the context of the shell process,
2) Upon selinux_file_open(), when checking and caching the open and
write access for shell to /dev/whatever in the file security struct,
3) Upon selinux_bprm_committing_creds() -> flush_unauthorized_files(),
when revalidating write access to /dev/whatever in the context of sudo,
4) Upon selinux_file_permission() ->
selinux_revalidate_file_permission(), when revalidating write access to
/dev/whatever in the context of sudo.
If any of those fail, then access is denied, so unless both the shell
and sudo are authorized to write to /dev/whatever, it is a no-go. NB
Only the shell context requires open permission here; the sudo context
only needs write.
> It is certainly the case that drivers, fs code, and other core code
> MUST NOT look at current_cred() in the context of syscalls like
> open(). Jann, I, and others have found quite a few rootable bugs of
> this sort. What makes MAC special here?
Do you mean syscalls like write(), not open()? I think your concern is
that they apply some check only during write() and not open() and
therefore are susceptible to confused deputy scenario above. In
contrast we are validating access at open, transfer/inherit, and use. If
we use file->f_cred instead of current_cred() in
selinux_revalidate_file_permission() and the current process SID differs
from that of the opener, we'll never apply a check for the actual
security context performing the write(), so information can flow in
violation of the MAC policy.
> I would believe there are cases where auditing write() callers makes
> some sense, but anyone reading those logs needs to understand that the
> creds are dubious at best.
^ permalink raw reply
* RE: [RFC PATCH 6/9] x86/sgx: Require userspace to provide allowed prots to ADD_PAGES
From: Ayoun, Serge @ 2019-06-05 11:10 UTC (permalink / raw)
To: Christopherson, Sean J
Cc: Andy Lutomirski, Xing, Cedric, Stephen Smalley, James Morris,
Serge E . Hallyn, LSM List, Paul Moore, Eric Paris,
selinux@vger.kernel.org, Jethro Beekman, Hansen, Dave,
Thomas Gleixner, Linus Torvalds, LKML, X86 ML,
linux-sgx@vger.kernel.org, Andrew Morton, nhorman@redhat.com,
npmccallum@redhat.com, Katz-zamir, Shay, Huang, Haitao,
Jarkko Sakkinen, Andy Shevchenko, Svahn, Kai, Borislav Petkov,
Josh Triplett, Huang, Kai, David Rientjes, Roberts, William C,
Tricca, Philip B
In-Reply-To: <20190531233159.30992-7-sean.j.christopherson@intel.com>
> From: Christopherson, Sean J
> Sent: Saturday, June 01, 2019 02:32
>
> /**
> * struct sgx_enclave_add_pages - parameter structure for the
> * %SGX_IOC_ENCLAVE_ADD_PAGES ioctl
> @@ -39,6 +44,7 @@ struct sgx_enclave_create {
> * @secinfo: address for the SECINFO data (common to all pages)
> * @nr_pages: number of pages (must be virtually contiguous)
> * @mrmask: bitmask for the measured 256 byte chunks (common to all
> pages)
> + * @flags: flags, e.g. SGX_ALLOW_{READ,WRITE,EXEC} (common to all
> pages)
> */
> struct sgx_enclave_add_pages {
> __u64 addr;
> @@ -46,7 +52,8 @@ struct sgx_enclave_add_pages {
> __u64 secinfo;
> __u32 nr_pages;
> __u16 mrmask;
> -} __attribute__((__packed__));
> + __u16 flags;
> +};
You are adding a flags member. The secinfo structure has already a flags member in it.
Why do you need both - they are both coming from user mode. What kind of scenario would
require having different values. Seems confusing.
---------------------------------------------------------------------
Intel Israel (74) Limited
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
^ permalink raw reply
* Re: [RFC][PATCH 0/8] Mount, FS, Block and Keyrings notifications [ver #2]
From: David Howells @ 2019-06-05 8:41 UTC (permalink / raw)
To: Casey Schaufler
Cc: dhowells, Andy Lutomirski, Al Viro, raven, Linux FS Devel,
Linux API, linux-block, keyrings, LSM List, LKML
In-Reply-To: <50c2ea19-6ae8-1f42-97ef-ba5c95e40475@schaufler-ca.com>
Casey Schaufler <casey@schaufler-ca.com> wrote:
> I will try to explain the problem once again. If process A
> sends a signal (writes information) to process B the kernel
> checks that either process A has the same UID as process B
> or that process A has privilege to override that policy.
> Process B is passive in this access control decision, while
> process A is active. In the event delivery case, process A
> does something (e.g. modifies a keyring) that generates an
> event, which is then sent to process B's event buffer.
I think this might be the core sticking point here. It looks like two
different situations:
(1) A explicitly sends event to B (eg. signalling, sendmsg, etc.)
(2) A implicitly and unknowingly sends event to B as a side effect of some
other action (eg. B has a watch for the event A did).
The LSM treats them as the same: that is B must have MAC authorisation to send
a message to A.
But there are problems with not sending the event:
(1) B's internal state is then corrupt (or, at least, unknowingly invalid).
(2) B can potentially figure out that the event happened by other means.
I've implemented four event sources so far:
(1) Keys/keyrings. You can only get events on a key you have View permission
on and the other process has to have write access to it, so I think this
is good enough.
(2) Block layer. Currently this will only get you hardware error events,
which is probably safe. I'm not sure you can manipulate those without
permission to directly access the device files.
(3) Superblock. This is trickier since it can see events that can be
manufactured (R/W <-> R/O remounting, EDQUOT) as well as events that
can't without hardware control (EIO, network link loss, RF kill).
(4) Mount topology. This is the trickiest since it allows you to see events
beyond the point at which you placed your watch (in essence, you place a
subtree watch).
The question is what permission checking should I do? Ideally, I'd
emulate a pathwalk between the watchpoint and the eventing object to see
if the owner of the watchpoint could reach it.
I'd need to do a reverse walk, calling inode_permission(MAY_NOT_BLOCK)
for each directory between the eventing object and the watchpoint to see
if one rejects it - but some filesystems have a permission check that
can't be called in this state.
It would also be necessary to do this separately for each watchpoint in
the parental chain.
Further, each permissions check would generate an audit event and could
generate FAN_ACCESS and/or FAN_ACCESS_PERM fanotify events - which could
be a problem if fanotify is also trying to post those events to the same
watch queue.
David
^ permalink raw reply
* [PATCH 2/2] ima: use the lsm policy update notifier
From: Janne Karhunen @ 2019-06-05 8:36 UTC (permalink / raw)
To: sds, zohar, paul; +Cc: linux-integrity, linux-security-module, Janne Karhunen
In-Reply-To: <20190605083606.4209-1-janne.karhunen@gmail.com>
Don't do lazy policy updates while running the rule matching,
run the updates as they happen.
Depends on commit 2d1d5cee66d1 ("LSM: switch to blocking policy update notifiers")
Signed-off-by: Janne Karhunen <janne.karhunen@gmail.com>
---
security/integrity/ima/ima.h | 2 +
security/integrity/ima/ima_main.c | 8 ++
security/integrity/ima/ima_policy.c | 124 +++++++++++++++++++++++-----
3 files changed, 114 insertions(+), 20 deletions(-)
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index d213e835c498..2203451862d4 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -154,6 +154,8 @@ unsigned long ima_get_binary_runtime_size(void);
int ima_init_template(void);
void ima_init_template_list(void);
int __init ima_init_digests(void);
+int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event,
+ void *lsm_data);
/*
* used to protect h_table and sha_table
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index f16353b5097e..9e3ea8a3f2db 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -43,6 +43,10 @@ int ima_appraise;
int ima_hash_algo = HASH_ALGO_SHA1;
static int hash_setup_done;
+static struct notifier_block ima_lsm_policy_notifier = {
+ .notifier_call = ima_lsm_policy_change,
+};
+
static int __init hash_setup(char *str)
{
struct ima_template_desc *template_desc = ima_template_desc_current();
@@ -621,6 +625,10 @@ static int __init init_ima(void)
error = ima_init();
}
+ error = register_blocking_lsm_notifier(&ima_lsm_policy_notifier);
+ if (error)
+ pr_warn("Couldn't register LSM notifier, error %d\n", error);
+
if (!error)
ima_update_policy_flag();
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 1cc822a59054..7129dc4cd396 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -249,31 +249,121 @@ static int __init default_appraise_policy_setup(char *str)
}
__setup("ima_appraise_tcb", default_appraise_policy_setup);
+static void ima_lsm_free_rule(struct ima_rule_entry *entry)
+{
+ int i;
+
+ for (i = 0; i < MAX_LSM_RULES; i++) {
+ kfree(entry->lsm[i].rule);
+ kfree(entry->lsm[i].args_p);
+ }
+ kfree(entry->fsname);
+ kfree(entry);
+}
+
+
+static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry)
+{
+ struct ima_rule_entry *nentry;
+ int i, result;
+
+ nentry = kmalloc(sizeof(*nentry), GFP_KERNEL);
+ if (!nentry)
+ return NULL;
+
+ memcpy(nentry, entry, sizeof(*nentry));
+ nentry->fsname = NULL;
+ for (i = 0; i < MAX_LSM_RULES; i++) {
+ nentry->lsm[i].rule = NULL;
+ nentry->lsm[i].args_p = NULL;
+ }
+
+ if (entry->fsname) {
+ nentry->fsname = kstrdup(entry->fsname, GFP_KERNEL);
+ if (!nentry->fsname)
+ goto out_err;
+ }
+ for (i = 0; i < MAX_LSM_RULES; i++) {
+ if (!entry->lsm[i].rule)
+ continue;
+
+ nentry->lsm[i].type = entry->lsm[i].type;
+ nentry->lsm[i].args_p = kstrdup(entry->lsm[i].args_p,
+ GFP_KERNEL);
+ if (!nentry->lsm[i].args_p)
+ goto out_err;
+
+ result = security_filter_rule_init(nentry->lsm[i].type,
+ Audit_equal,
+ nentry->lsm[i].args_p,
+ &nentry->lsm[i].rule);
+ if (result == -EINVAL)
+ pr_warn("ima: rule for LSM \'%d\' is invalid\n",
+ entry->lsm[i].type);
+
+ }
+ return nentry;
+
+out_err:
+ ima_lsm_free_rule(nentry);
+ return NULL;
+}
+
+static int ima_lsm_update_rule(struct ima_rule_entry *entry)
+{
+ struct ima_rule_entry *nentry;
+
+ nentry = ima_lsm_copy_rule(entry);
+ if (!nentry)
+ return -ENOMEM;
+
+ list_replace_rcu(&entry->list, &nentry->list);
+ synchronize_rcu();
+ ima_lsm_free_rule(entry);
+
+ return 0;
+}
+
/*
* The LSM policy can be reloaded, leaving the IMA LSM based rules referring
* to the old, stale LSM policy. Update the IMA LSM based rules to reflect
- * the reloaded LSM policy. We assume the rules still exist; and BUG_ON() if
- * they don't.
+ * the reloaded LSM policy.
*/
static void ima_lsm_update_rules(void)
{
- struct ima_rule_entry *entry;
- int result;
- int i;
+ struct ima_rule_entry *entry, *e;
+ int i, result, needs_update;
- list_for_each_entry(entry, &ima_policy_rules, list) {
+ list_for_each_entry_safe(entry, e, &ima_policy_rules, list) {
+ needs_update = 0;
for (i = 0; i < MAX_LSM_RULES; i++) {
- if (!entry->lsm[i].rule)
- continue;
- result = security_filter_rule_init(entry->lsm[i].type,
- Audit_equal,
- entry->lsm[i].args_p,
- &entry->lsm[i].rule);
- BUG_ON(!entry->lsm[i].rule);
+ if (entry->lsm[i].rule) {
+ needs_update = 1;
+ break;
+ }
+ }
+ if (!needs_update)
+ continue;
+
+ result = ima_lsm_update_rule(entry);
+ if (result) {
+ pr_err("ima: lsm rule update error %d\n",
+ result);
+ return;
}
}
}
+int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event,
+ void *lsm_data)
+{
+ if (event != LSM_POLICY_CHANGE)
+ return NOTIFY_DONE;
+
+ ima_lsm_update_rules();
+ return NOTIFY_OK;
+}
+
/**
* ima_match_rules - determine whether an inode matches the measure rule.
* @rule: a pointer to a rule
@@ -327,11 +417,10 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
for (i = 0; i < MAX_LSM_RULES; i++) {
int rc = 0;
u32 osid;
- int retried = 0;
if (!rule->lsm[i].rule)
continue;
-retry:
+
switch (i) {
case LSM_OBJ_USER:
case LSM_OBJ_ROLE:
@@ -352,11 +441,6 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
default:
break;
}
- if ((rc < 0) && (!retried)) {
- retried = 1;
- ima_lsm_update_rules();
- goto retry;
- }
if (!rc)
return false;
}
--
2.17.1
^ permalink raw reply related
* [PATCH 1/2] LSM: switch to blocking policy update notifiers
From: Janne Karhunen @ 2019-06-05 8:36 UTC (permalink / raw)
To: sds, zohar, paul; +Cc: linux-integrity, linux-security-module, Janne Karhunen
Atomic policy updaters are not very useful as they cannot
usually perform the policy updates on their own. Since it
seems that there is no strict need for the atomicity,
switch to the blocking variant. While doing so, rename
the functions accordingly.
Signed-off-by: Janne Karhunen <janne.karhunen@gmail.com>
---
drivers/infiniband/core/device.c | 6 +++---
include/linux/security.h | 6 +++---
security/security.c | 23 +++++++++++++----------
security/selinux/hooks.c | 2 +-
security/selinux/selinuxfs.c | 2 +-
5 files changed, 21 insertions(+), 18 deletions(-)
diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
index 78dc07c6ac4b..61c0c93a2e73 100644
--- a/drivers/infiniband/core/device.c
+++ b/drivers/infiniband/core/device.c
@@ -2499,7 +2499,7 @@ static int __init ib_core_init(void)
goto err_mad;
}
- ret = register_lsm_notifier(&ibdev_lsm_nb);
+ ret = register_blocking_lsm_notifier(&ibdev_lsm_nb);
if (ret) {
pr_warn("Couldn't register LSM notifier. ret %d\n", ret);
goto err_sa;
@@ -2518,7 +2518,7 @@ static int __init ib_core_init(void)
return 0;
err_compat:
- unregister_lsm_notifier(&ibdev_lsm_nb);
+ unregister_blocking_lsm_notifier(&ibdev_lsm_nb);
err_sa:
ib_sa_cleanup();
err_mad:
@@ -2544,7 +2544,7 @@ static void __exit ib_core_cleanup(void)
nldev_exit();
rdma_nl_unregister(RDMA_NL_LS);
unregister_pernet_device(&rdma_dev_net_ops);
- unregister_lsm_notifier(&ibdev_lsm_nb);
+ unregister_blocking_lsm_notifier(&ibdev_lsm_nb);
ib_sa_cleanup();
ib_mad_cleanup();
addr_cleanup();
diff --git a/include/linux/security.h b/include/linux/security.h
index 659071c2e57c..fc655fbe44ad 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -189,9 +189,9 @@ static inline const char *kernel_load_data_id_str(enum kernel_load_data_id id)
#ifdef CONFIG_SECURITY
-int call_lsm_notifier(enum lsm_event event, void *data);
-int register_lsm_notifier(struct notifier_block *nb);
-int unregister_lsm_notifier(struct notifier_block *nb);
+int call_blocking_lsm_notifier(enum lsm_event event, void *data);
+int register_blocking_lsm_notifier(struct notifier_block *nb);
+int unregister_blocking_lsm_notifier(struct notifier_block *nb);
/* prototypes */
extern int security_init(void);
diff --git a/security/security.c b/security/security.c
index c01a88f65ad8..6bfc7636ddb7 100644
--- a/security/security.c
+++ b/security/security.c
@@ -39,7 +39,7 @@
#define LSM_COUNT (__end_lsm_info - __start_lsm_info)
struct security_hook_heads security_hook_heads __lsm_ro_after_init;
-static ATOMIC_NOTIFIER_HEAD(lsm_notifier_chain);
+static BLOCKING_NOTIFIER_HEAD(blocking_lsm_notifier_chain);
static struct kmem_cache *lsm_file_cache;
static struct kmem_cache *lsm_inode_cache;
@@ -430,23 +430,26 @@ void __init security_add_hooks(struct security_hook_list *hooks, int count,
panic("%s - Cannot get early memory.\n", __func__);
}
-int call_lsm_notifier(enum lsm_event event, void *data)
+int call_blocking_lsm_notifier(enum lsm_event event, void *data)
{
- return atomic_notifier_call_chain(&lsm_notifier_chain, event, data);
+ return blocking_notifier_call_chain(&blocking_lsm_notifier_chain,
+ event, data);
}
-EXPORT_SYMBOL(call_lsm_notifier);
+EXPORT_SYMBOL(call_blocking_lsm_notifier);
-int register_lsm_notifier(struct notifier_block *nb)
+int register_blocking_lsm_notifier(struct notifier_block *nb)
{
- return atomic_notifier_chain_register(&lsm_notifier_chain, nb);
+ return blocking_notifier_chain_register(&blocking_lsm_notifier_chain,
+ nb);
}
-EXPORT_SYMBOL(register_lsm_notifier);
+EXPORT_SYMBOL(register_blocking_lsm_notifier);
-int unregister_lsm_notifier(struct notifier_block *nb)
+int unregister_blocking_lsm_notifier(struct notifier_block *nb)
{
- return atomic_notifier_chain_unregister(&lsm_notifier_chain, nb);
+ return blocking_notifier_chain_unregister(&blocking_lsm_notifier_chain,
+ nb);
}
-EXPORT_SYMBOL(unregister_lsm_notifier);
+EXPORT_SYMBOL(unregister_blocking_lsm_notifier);
/**
* lsm_cred_alloc - allocate a composite cred blob
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index c61787b15f27..c1e37018c8eb 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -197,7 +197,7 @@ static int selinux_lsm_notifier_avc_callback(u32 event)
{
if (event == AVC_CALLBACK_RESET) {
sel_ib_pkey_flush();
- call_lsm_notifier(LSM_POLICY_CHANGE, NULL);
+ call_blocking_lsm_notifier(LSM_POLICY_CHANGE, NULL);
}
return 0;
diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
index 145ee62f205a..1e2e3e4b5fdb 100644
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c
@@ -180,7 +180,7 @@ static ssize_t sel_write_enforce(struct file *file, const char __user *buf,
selnl_notify_setenforce(new_value);
selinux_status_update_setenforce(state, new_value);
if (!new_value)
- call_lsm_notifier(LSM_POLICY_CHANGE, NULL);
+ call_blocking_lsm_notifier(LSM_POLICY_CHANGE, NULL);
}
length = count;
out:
--
2.17.1
^ permalink raw reply related
* Re: [PATCH 00/58] LSM: Module stacking for AppArmor
From: John Johansen @ 2019-06-05 5:03 UTC (permalink / raw)
To: James Morris
Cc: Stephen Smalley, Casey Schaufler, casey.schaufler,
linux-security-module, selinux, keescook, penguin-kernel, paul
In-Reply-To: <alpine.LRH.2.21.1906051305370.17052@namei.org>
On 6/4/19 8:08 PM, James Morris wrote:
> On Tue, 4 Jun 2019, John Johansen wrote:
>
>> system as a whole is still being protected by selinux. Similar requests
>> have been made for lxd doing system containers. lxd currently supports
>> nested apparmor, so on an ubuntu system you can run suse container,
>> where the ubuntu host is enforcing policy and the suse container is
>> loading and enforcing its policy as well. In this case the policy of the
>> container is bounded by the policy of the host. The goal is to be able
>> to the same with selinux and smack based systems, LSM stacking is of
>> course only part of what is required to make this work.
>
> Interesting. So you're stacking apparmor with itself, and one is the
> container instance? And you add another stacked apparmor for a 2nd
> container etc. ?
Yes, on Ubuntu & suse you can lauch lxd system containers with the
container having a system policy bounding the container, and the container
having its own apparmor policy namespace. So it loads and has its own
policy that is enforced.
This allows for us to run older versions of ubuntu (say 16.04) on an
18.04 host, and have the 16.04 policy behave just as if it was the host.
>
>> Ubuntu actually has a very small apparmor delta these days, and we are
>> working on eliminating it entirely. There are no patches in Ubuntu that
>> require new hooks. As for the delta wrt to the stacking work, Ubuntu has
>> pulled in a subset of this delta and has been shipping kernels with
>> stacking enabled for 4 releases now and apparmor development is done
>> with LSM stacking in mind.
>
> A subset of these patches from Casey?
>
Yes, we have been testing the and using Casey's patches. The set has
changed from release to release, and we don't usually take all of them.
As we are trying to find the right balance.
For 19.04 we did test the full stack and the decision was to hold off
and give it more testing. The big concern was around the secid changes
which needed more review and testing before we could commit to them.
Instead we cherry-picked the stacking patches from 5.2 and a subset of
the set under review, and reverted the upstream apparmor changes that
require secids.
This approach won't be an option for the 19.10 release and we will be
needing the full patchset. I should be able to provide some benchmark
and testing data soon.
^ permalink raw reply
* Re: [RFC][PATCH 0/8] Mount, FS, Block and Keyrings notifications [ver #2]
From: Andy Lutomirski @ 2019-06-05 4:19 UTC (permalink / raw)
To: Stephen Smalley
Cc: Andy Lutomirski, David Howells, Al Viro, Casey Schaufler, raven,
Linux FS Devel, Linux API, linux-block, keyrings, LSM List, LKML
In-Reply-To: <CAB9W1A0AgMYOwGx9c-TmAt=1O6Bjsr2P3Nhd=2+QV39dgw0CrA@mail.gmail.com>
On Tue, Jun 4, 2019 at 6:18 PM Stephen Smalley
<stephen.smalley@gmail.com> wrote:
>
> On Tue, Jun 4, 2019 at 4:58 PM Andy Lutomirski <luto@kernel.org> wrote:
>>
>> On Tue, Jun 4, 2019 at 1:39 PM David Howells <dhowells@redhat.com> wrote:
>> >
>> > Andy Lutomirski <luto@kernel.org> wrote:
>> >
>> > > > Here's a set of patches to add a general variable-length notification queue
>> > > > concept and to add sources of events for:
>> > >
>> > > I asked before and didn't see a response, so I'll ask again. Why are you
>> > > paying any attention at all to the creds that generate an event?
>> >
>> > Casey responded to you. It's one of his requirements.
>> >
>>
>> It being a "requirement" doesn't make it okay.
>>
>> > However, the LSMs (or at least SELinux) ignore f_cred and use current_cred()
>> > when checking permissions. See selinux_revalidate_file_permission() for
>> > example - it uses current_cred() not file->f_cred to re-evaluate the perms,
>> > and the fd might be shared between a number of processes with different creds.
>>
>> That's a bug. It's arguably a rather severe bug. If I ever get
>> around to writing the patch I keep thinking of that will warn if we
>> use creds from invalid contexts, it will warn.
>
>
> No, not a bug. Working as designed. Initial validation on open, but revalidation upon read/write if something has changed since open (process SID differs from opener, inode SID has changed, policy has changed). Current subject SID should be used for the revalidation. It's a MAC vs DAC difference.
>
Can you explain how the design is valid, then? Consider nasty cases like this:
$ sudo -u lotsofgarbage 2>/dev/whatever
It is certainly the case that drivers, fs code, and other core code
MUST NOT look at current_cred() in the context of syscalls like
open(). Jann, I, and others have found quite a few rootable bugs of
this sort. What makes MAC special here?
I would believe there are cases where auditing write() callers makes
some sense, but anyone reading those logs needs to understand that the
creds are dubious at best.
^ permalink raw reply
* Re: [PATCH 00/58] LSM: Module stacking for AppArmor
From: James Morris @ 2019-06-05 3:08 UTC (permalink / raw)
To: John Johansen
Cc: Stephen Smalley, Casey Schaufler, casey.schaufler,
linux-security-module, selinux, keescook, penguin-kernel, paul
In-Reply-To: <42fcfaf2-43a9-642e-2e19-282087c3cdb2@canonical.com>
On Tue, 4 Jun 2019, John Johansen wrote:
> system as a whole is still being protected by selinux. Similar requests
> have been made for lxd doing system containers. lxd currently supports
> nested apparmor, so on an ubuntu system you can run suse container,
> where the ubuntu host is enforcing policy and the suse container is
> loading and enforcing its policy as well. In this case the policy of the
> container is bounded by the policy of the host. The goal is to be able
> to the same with selinux and smack based systems, LSM stacking is of
> course only part of what is required to make this work.
Interesting. So you're stacking apparmor with itself, and one is the
container instance? And you add another stacked apparmor for a 2nd
container etc. ?
> Ubuntu actually has a very small apparmor delta these days, and we are
> working on eliminating it entirely. There are no patches in Ubuntu that
> require new hooks. As for the delta wrt to the stacking work, Ubuntu has
> pulled in a subset of this delta and has been shipping kernels with
> stacking enabled for 4 releases now and apparmor development is done
> with LSM stacking in mind.
A subset of these patches from Casey?
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: [PATCH v5 2/3] mm: init: report memory auto-initialization features at boot time
From: Kaiwan N Billimoria @ 2019-06-05 2:58 UTC (permalink / raw)
To: Alexander Potapenko
Cc: Kees Cook, Andrew Morton, Christoph Lameter, Dmitry Vyukov,
James Morris, Jann Horn, Kostya Serebryany, Laura Abbott,
Mark Rutland, Masahiro Yamada, Matthew Wilcox, Nick Desaulniers,
Randy Dunlap, Sandeep Patil, Serge E. Hallyn, Souptick Joarder,
Marco Elver, Kernel Hardening, Linux Memory Management List,
linux-security-module
In-Reply-To: <CAG_fn=UxfaFVZbtnO0VefKhi3iZUYn5ybe_Nvo0rCOxxA2nn-Q@mail.gmail.com>
On Tue, Jun 4, 2019 at 8:36 PM Alexander Potapenko <glider@google.com> wrote:
>
> On Tue, Jun 4, 2019 at 8:01 AM Kaiwan N Billimoria
> <kaiwan@kaiwantech.com> wrote:
> >
> > On Tue, Jun 4, 2019 at 8:44 AM Kees Cook <keescook@chromium.org> wrote:
> > >
> > > On Mon, Jun 03, 2019 at 11:24:49AM +0200, Alexander Potapenko wrote:
> > > > On Sat, Jun 1, 2019 at 3:18 AM Andrew Morton <akpm@linux-foundation.org> wrote:
> > > > >
> > > > > On Wed, 29 May 2019 14:38:11 +0200 Alexander Potapenko <glider@google.com> wrote:
> > > > >
> > > > > > Print the currently enabled stack and heap initialization modes.
> > > > > >
> > > > > > The possible options for stack are:
> > > > > > - "all" for CONFIG_INIT_STACK_ALL;
> > > > > > - "byref_all" for CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL;
> > > > > > - "byref" for CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF;
> > > > > > - "__user" for CONFIG_GCC_PLUGIN_STRUCTLEAK_USER;
> > > > > > - "off" otherwise.
> > > > > >
> > > > > > Depending on the values of init_on_alloc and init_on_free boottime
> > > > > > options we also report "heap alloc" and "heap free" as "on"/"off".
> > > > >
> > > > > Why?
> > > > >
> > > > > Please fully describe the benefit to users so that others can judge the
> > > > > desirability of the patch. And so they can review it effectively, etc.
> > > > I'm going to update the description with the following passage:
> > > >
> > > > Print the currently enabled stack and heap initialization modes.
> > > >
> > > > Stack initialization is enabled by a config flag, while heap
> > > > initialization is configured at boot time with defaults being set
> > > > in the config. It's more convenient for the user to have all information
> > > > about these hardening measures in one place.
> > > >
> > > > Does this make sense?
> > > > > Always!
> > > > >
> > > > > > In the init_on_free mode initializing pages at boot time may take some
> > > > > > time, so print a notice about that as well.
> > > > >
> > > > > How much time?
> > > > I've seen pauses up to 1 second, not actually sure they're worth a
> > > > separate line in the log.
> > > > Kees, how long were the delays in your case?
> > >
> > > I didn't measure it, but I think it was something like 0.5 second per GB.
> > > I noticed because normally boot flashes by. With init_on_free it pauses
> > > for no apparent reason, which is why I suggested the note. (I mean *I*
> > > knew why it was pausing, but it might surprise someone who sets
> > > init_on_free=1 without really thinking about what's about to happen at
> > > boot.)
> >
> > (Pardon the gmail client)
> > How about:
> > - if (want_init_on_free())
> > - pr_info("Clearing system memory may take some time...\n");
> > + if (want_init_on_free())
> > + pr_info("meminit: clearing system memory may take some
> > time...\n");
> Yes, adding a prefix may give the users better understanding of who's
> clearing the memory.
> We should stick to the same prefix as before though, i.e. "mem auto-init"
True, agreed.
--
Kaiwan
^ permalink raw reply
* Re: [PATCH 00/58] LSM: Module stacking for AppArmor
From: John Johansen @ 2019-06-05 1:50 UTC (permalink / raw)
To: Stephen Smalley, Casey Schaufler, casey.schaufler, jmorris,
linux-security-module, selinux
Cc: keescook, penguin-kernel, paul
In-Reply-To: <f71388e9-a4c5-8935-137b-8eb50be7f833@tycho.nsa.gov>
On 6/4/19 5:29 AM, Stephen Smalley wrote:
> On 6/2/19 12:50 PM, Casey Schaufler wrote:
>> This patchset provides the changes required for
>> the AppArmor security module to stack safely with any other.
>
> Please explain the motivation - why do we want to allow AppArmor to stack with other modules, who would use it, how would it be used, what does it provide that isn't already possible in the absence of it.
>
Its another step towards making stacking generic. The current stacking in 5.2 only allows for a subset of blobs and limits what can be done by new security modules. This is another step towards achieving generic stacking. Whether it makes sense to stacking a given set of security modules is a different discussion. I am fairly sure if landlock/sara get in upstream they will at some point want access to parts of the LSM that are currently limited to a major LSM.
On the apparmor front stacking with other modules has been asked for in the context of containers, both application and system. For example snapd would like to be able to stack apparmor on an selinux system so the snap container can enforce its apparmor set of policy while the system as a whole is still being protected by selinux. Similar requests have been made for lxd doing system containers. lxd currently supports nested apparmor, so on an ubuntu system you can run suse container, where the ubuntu host is enforcing policy and the suse container is loading and enforcing its policy as well. In this case the policy of the container is bounded by the policy of the host. The goal is to be able to the same with selinux and smack based systems, LSM stacking is of course only part of what is required to make this work.
Currently with the stacking patches we can boot a fedora system, and run an ubuntu container with apparmor enforcing its policy inside the container and selinux enforcing its policy on the host.
> Also, Ubuntu fully upstreamed all of their changes to AppArmor, would this still suffice to enable stacking of AppArmor or do they rely on hooks that are not handled here?
Ubuntu actually has a very small apparmor delta these days, and we are working on eliminating it entirely. There are no patches in Ubuntu that require new hooks. As for the delta wrt to the stacking work, Ubuntu has pulled in a subset of this delta and has been shipping kernels with stacking enabled for 4 releases now and apparmor development is done with LSM stacking in mind.
^ permalink raw reply
* Re: possible deadlock in get_user_pages_unlocked (2)
From: syzbot @ 2019-06-05 1:16 UTC (permalink / raw)
To: akpm, aneesh.kumar, dan.j.williams, ira.weiny, jack, jhubbard,
jmorris, keith.busch, linux-kernel, linux-mm,
linux-security-module, richard.weiyang, rppt, serge, sfr,
syzkaller-bugs, willy, zohar
In-Reply-To: <000000000000543e45058a3cf40b@google.com>
syzbot has bisected this bug to:
commit 69d61f577d147b396be0991b2ac6f65057f7d445
Author: Mimi Zohar <zohar@linux.ibm.com>
Date: Wed Apr 3 21:47:46 2019 +0000
ima: verify mprotect change is consistent with mmap policy
bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=1055a2f2a00000
start commit: 56b697c6 Add linux-next specific files for 20190604
git tree: linux-next
final crash: https://syzkaller.appspot.com/x/report.txt?x=1255a2f2a00000
console output: https://syzkaller.appspot.com/x/log.txt?x=1455a2f2a00000
kernel config: https://syzkaller.appspot.com/x/.config?x=4248d6bc70076f7d
dashboard link: https://syzkaller.appspot.com/bug?extid=e1374b2ec8f6a25ab2e5
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=165757eea00000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=10dd3e86a00000
Reported-by: syzbot+e1374b2ec8f6a25ab2e5@syzkaller.appspotmail.com
Fixes: 69d61f577d14 ("ima: verify mprotect change is consistent with mmap
policy")
For information about bisection process see: https://goo.gl/tpsmEJ#bisection
^ permalink raw reply
* RE: [RFC PATCH 2/9] x86/sgx: Do not naturally align MAP_FIXED address
From: Xing, Cedric @ 2019-06-04 22:10 UTC (permalink / raw)
To: Andy Lutomirski, Jarkko Sakkinen
Cc: Christopherson, Sean J, Stephen Smalley, James Morris,
Serge E . Hallyn, LSM List, Paul Moore, Eric Paris,
selinux@vger.kernel.org, Jethro Beekman, Hansen, Dave,
Thomas Gleixner, Linus Torvalds, LKML, X86 ML,
linux-sgx@vger.kernel.org, Andrew Morton, nhorman@redhat.com,
npmccallum@redhat.com, Ayoun, Serge, Katz-zamir, Shay,
Huang, Haitao, Andy Shevchenko, Svahn, Kai, Borislav Petkov,
Josh Triplett, Huang, Kai, David Rientjes, Roberts, William C,
Tricca, Philip B
In-Reply-To: <CALCETrVe0jhAWAFmx+NFEjJcijSJv2LDVC7cUXi0w99kNKjh_g@mail.gmail.com>
> From: linux-sgx-owner@vger.kernel.org [mailto:linux-sgx-
> owner@vger.kernel.org] On Behalf Of Andy Lutomirski
> Sent: Tuesday, June 04, 2019 1:16 PM
>
> On Tue, Jun 4, 2019 at 4:50 AM Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com> wrote:
> >
> > On Fri, May 31, 2019 at 04:31:52PM -0700, Sean Christopherson wrote:
> > > SGX enclaves have an associated Enclave Linear Range (ELRANGE) that
> > > is tracked and enforced by the CPU using a base+mask approach,
> > > similar to how hardware range registers such as the variable MTRRs.
> > > As a result, the ELRANGE must be naturally sized and aligned.
> > >
> > > To reduce boilerplate code that would be needed in every userspace
> > > enclave loader, the SGX driver naturally aligns the mmap() address
> > > and also requires the range to be naturally sized. Unfortunately,
> > > SGX fails to grant a waiver to the MAP_FIXED case, e.g. incorrectly
> > > rejects mmap() if userspace is attempting to map a small slice of an
> existing enclave.
> > >
> > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> >
> > Why you want to allow mmap() to be called multiple times? mmap() could
> > be allowed only once with PROT_NONE and denied afterwards. Is this for
> > sending fd to another process that would map already existing enclave?
> >
> > I don't see any checks for whether the is enclave underneath. Also, I
> > think that in all cases mmap() callback should allow only PROT_NONE as
> > permissions for clarity even if it could called multiple times.
> >
>
> What's the advantage to only allowing PROT_NONE? The idea here is to
> allow a PROT_NONE map followed by some replacemets that overlay it for
> the individual segments. Admittedly, mprotect() can do the same thing,
> but disallowing mmap() seems at least a bit surprising.
Disallowing mmap() is not only surprising but also unnecessary.
A bit off topic here. This mmap()/mprotect() discussion reminds me a question (guess for Jarkko): Now that vma->vm_file->private_data keeps a pointer to the enclave, why do we store it again in vma->vm_private? It isn't a big deal but non-NULL vm_private does prevent mprotect() from merging adjacent VMAs.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox