[1/9] integrity: Prepare for having "ima" and "evm" available in "integrity" LSM
Commit Message
Move "integrity" LSM to the end of the Kconfig list and prepare for
having ima and evm LSM initialization called from the top-level
"integrity" LSM.
Cc: Paul Moore <paul@paul-moore.com>
Cc: James Morris <jmorris@namei.org>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Cc: Mimi Zohar <zohar@linux.ibm.com>
Cc: Dmitry Kasatkin <dmitry.kasatkin@gmail.com>
Cc: "Mickaël Salaün" <mic@digikod.net>
Cc: linux-security-module@vger.kernel.org
Cc: linux-integrity@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
security/Kconfig | 10 +++++-----
security/integrity/evm/evm_main.c | 4 ++++
security/integrity/iint.c | 17 +++++++++++++----
security/integrity/ima/ima_main.c | 4 ++++
security/integrity/integrity.h | 6 ++++++
5 files changed, 32 insertions(+), 9 deletions(-)
Comments
On 14/10/2022 00:36, Kees Cook wrote:
> Move "integrity" LSM to the end of the Kconfig list and prepare for
> having ima and evm LSM initialization called from the top-level
> "integrity" LSM.
>
> Cc: Paul Moore <paul@paul-moore.com>
> Cc: James Morris <jmorris@namei.org>
> Cc: "Serge E. Hallyn" <serge@hallyn.com>
> Cc: Mimi Zohar <zohar@linux.ibm.com>
> Cc: Dmitry Kasatkin <dmitry.kasatkin@gmail.com>
> Cc: "Mickaël Salaün" <mic@digikod.net>
> Cc: linux-security-module@vger.kernel.org
> Cc: linux-integrity@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> security/Kconfig | 10 +++++-----
> security/integrity/evm/evm_main.c | 4 ++++
> security/integrity/iint.c | 17 +++++++++++++----
> security/integrity/ima/ima_main.c | 4 ++++
> security/integrity/integrity.h | 6 ++++++
> 5 files changed, 32 insertions(+), 9 deletions(-)
>
> diff --git a/security/Kconfig b/security/Kconfig
> index e6db09a779b7..d472e87a2fc4 100644
> --- a/security/Kconfig
> +++ b/security/Kconfig
> @@ -246,11 +246,11 @@ endchoice
>
> config LSM
> string "Ordered list of enabled LSMs"
> - default "landlock,lockdown,yama,loadpin,safesetid,integrity,smack,selinux,tomoyo,apparmor,bpf" if DEFAULT_SECURITY_SMACK
> - default "landlock,lockdown,yama,loadpin,safesetid,integrity,apparmor,selinux,smack,tomoyo,bpf" if DEFAULT_SECURITY_APPARMOR
> - default "landlock,lockdown,yama,loadpin,safesetid,integrity,tomoyo,bpf" if DEFAULT_SECURITY_TOMOYO
> - default "landlock,lockdown,yama,loadpin,safesetid,integrity,bpf" if DEFAULT_SECURITY_DAC
> - default "landlock,lockdown,yama,loadpin,safesetid,integrity,selinux,smack,tomoyo,apparmor,bpf"
> + default "landlock,lockdown,yama,loadpin,safesetid,smack,selinux,tomoyo,apparmor,bpf,integrity" if DEFAULT_SECURITY_SMACK
> + default "landlock,lockdown,yama,loadpin,safesetid,apparmor,selinux,smack,tomoyo,bpf,integrity" if DEFAULT_SECURITY_APPARMOR
> + default "landlock,lockdown,yama,loadpin,safesetid,tomoyo,bpf,integrity" if DEFAULT_SECURITY_TOMOYO
> + default "landlock,lockdown,yama,loadpin,safesetid,bpf,integrity" if DEFAULT_SECURITY_DAC
> + default "landlock,lockdown,yama,loadpin,safesetid,selinux,smack,tomoyo,apparmor,bpf,integrity"
This is not backward compatible, but can easily be fixed thanks to
DEFINE_LSM().order
Side node: I proposed an alternative to that but it was Nacked:
https://lore.kernel.org/all/20210222150608.808146-1-mic@digikod.net/
> help
> A comma-separated list of LSMs, in initialization order.
> Any LSMs left off this list will be ignored. This can be
> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> index 2e6fb6e2ffd2..1ef965089417 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -904,3 +904,7 @@ static int __init init_evm(void)
> }
>
> late_initcall(init_evm);
> +
> +void __init integrity_lsm_evm_init(void)
> +{
> +}
> diff --git a/security/integrity/iint.c b/security/integrity/iint.c
> index 8638976f7990..4f322324449d 100644
> --- a/security/integrity/iint.c
> +++ b/security/integrity/iint.c
> @@ -18,7 +18,6 @@
> #include <linux/file.h>
> #include <linux/uaccess.h>
> #include <linux/security.h>
> -#include <linux/lsm_hooks.h>
> #include "integrity.h"
>
> static struct rb_root integrity_iint_tree = RB_ROOT;
> @@ -172,19 +171,29 @@ static void init_once(void *foo)
> mutex_init(&iint->mutex);
> }
>
> -static int __init integrity_iintcache_init(void)
> +void __init integrity_add_lsm_hooks(struct security_hook_list *hooks,
> + int count)
> +{
> + security_add_hooks(hooks, count, "integrity");
> +}
> +
> +static int __init integrity_lsm_init(void)
> {
> iint_cache =
> kmem_cache_create("iint_cache", sizeof(struct integrity_iint_cache),
> 0, SLAB_PANIC, init_once);
> +
> + integrity_lsm_ima_init();
> + integrity_lsm_evm_init();
> +
> return 0;
> }
> +
> DEFINE_LSM(integrity) = {
> .name = "integrity",
> - .init = integrity_iintcache_init,
> + .init = integrity_lsm_init,
For backward compatibility, there should be an ".order =
LSM_ORDER_FIRST," here.
On Fri, Oct 14, 2022 at 04:40:01PM +0200, Mickaël Salaün wrote:
> This is not backward compatible
Why? Nothing will be running LSM hooks until init finishes, at which
point the integrity inode cache will be allocated. And ima and evm don't
start up until lateinit.
>, but can easily be fixed thanks to
> DEFINE_LSM().order
That forces the LSM to be enabled, which may not be desired?
> Side node: I proposed an alternative to that but it was Nacked:
> https://lore.kernel.org/all/20210222150608.808146-1-mic@digikod.net/
Yeah, for the reasons pointed out -- that can't work. The point is to
not have The Default LSM. I do think Casey's NAK was rather prickly,
though. ;)
On 14/10/2022 19:59, Kees Cook wrote:
> On Fri, Oct 14, 2022 at 04:40:01PM +0200, Mickaël Salaün wrote:
>> This is not backward compatible
>
> Why? Nothing will be running LSM hooks until init finishes, at which
> point the integrity inode cache will be allocated. And ima and evm don't
> start up until lateinit.
>
>> , but can easily be fixed thanks to
>> DEFINE_LSM().order
>
> That forces the LSM to be enabled, which may not be desired?
This is not backward compatible because currently IMA is enabled
independently of the "lsm=" cmdline, which means that for all installed
systems using IMA and also with a custom "lsm=" cmdline, updating the
kernel with this patch will (silently) disable IMA. Using ".order =
LSM_ORDER_FIRST," should keep this behavior.
BTW, I think we should set such order (but maybe rename it) for LSMs
that do nothing unless configured (e.g. Yama, Landlock).
>
>> Side node: I proposed an alternative to that but it was Nacked:
>> https://lore.kernel.org/all/20210222150608.808146-1-mic@digikod.net/
>
> Yeah, for the reasons pointed out -- that can't work. The point is to
> not have The Default LSM. I do think Casey's NAK was rather prickly,
> though. ;)
I don't agree, there is no "the default LSM", and this new behavior is
under an LSM_AUTO configuration option.
On Mon, Oct 17, 2022 at 11:26:44AM +0200, Mickaël Salaün wrote:
>
> On 14/10/2022 19:59, Kees Cook wrote:
> > On Fri, Oct 14, 2022 at 04:40:01PM +0200, Mickaël Salaün wrote:
> > > This is not backward compatible
> >
> > Why? Nothing will be running LSM hooks until init finishes, at which
> > point the integrity inode cache will be allocated. And ima and evm don't
> > start up until lateinit.
> >
> > > , but can easily be fixed thanks to
> > > DEFINE_LSM().order
> >
> > That forces the LSM to be enabled, which may not be desired?
>
> This is not backward compatible because currently IMA is enabled
> independently of the "lsm=" cmdline, which means that for all installed
> systems using IMA and also with a custom "lsm=" cmdline, updating the kernel
> with this patch will (silently) disable IMA. Using ".order =
> LSM_ORDER_FIRST," should keep this behavior.
>
> BTW, I think we should set such order (but maybe rename it) for LSMs that do
> nothing unless configured (e.g. Yama, Landlock).
Ah yeah, good point. the .enabled stuff will need to be correctly wired
up. Anyway, it's a good starting point for the conversion, so I'm hoping
it can be carried forward by someone who is not me. :) (Hint hint to the
integrity folks...)
> > > Side node: I proposed an alternative to that but it was Nacked:
> > > https://lore.kernel.org/all/20210222150608.808146-1-mic@digikod.net/
> >
> > Yeah, for the reasons pointed out -- that can't work. The point is to
> > not have The Default LSM. I do think Casey's NAK was rather prickly,
> > though. ;)
>
> I don't agree, there is no "the default LSM", and this new behavior is under
> an LSM_AUTO configuration option.
The "config it twice" aspect of the current situation is suboptimal,
yes. Let me go comment on the old thread...
On Thu, 2022-10-13 at 15:36 -0700, Kees Cook wrote:
> Move "integrity" LSM to the end of the Kconfig list and prepare for
> having ima and evm LSM initialization called from the top-level
> "integrity" LSM.
The securityfs integrity directory and the "iint_cache" are shared
IMA/EVM resources. Just because the "iint_cache" was on an LSM hook,
it should never have been treated as an LSM on its own. IMA maintains
and verifies file data integrity, while EVM maintains and verifies file
metadata integrity. IMA and EVM may both be configured and enabled, or
independently of each other. However, only if either IMA or EVM are
configured and enabled, should the iint_cache be created. There is
absolutely no need for an independent "integrity" LSM.
On Wed, Oct 19, 2022 at 10:34:08AM -0400, Mimi Zohar wrote:
> On Thu, 2022-10-13 at 15:36 -0700, Kees Cook wrote:
> > Move "integrity" LSM to the end of the Kconfig list and prepare for
> > having ima and evm LSM initialization called from the top-level
> > "integrity" LSM.
>
> The securityfs integrity directory and the "iint_cache" are shared
> IMA/EVM resources. Just because the "iint_cache" was on an LSM hook,
> it should never have been treated as an LSM on its own. IMA maintains
> and verifies file data integrity, while EVM maintains and verifies file
> metadata integrity. IMA and EVM may both be configured and enabled, or
> independently of each other. However, only if either IMA or EVM are
> configured and enabled, should the iint_cache be created. There is
> absolutely no need for an independent "integrity" LSM.
The purpose of this patch was to tie ima and evm into integrity, since
the iint_cache is used by both. It's been true since 4.20 that using
ima and evm requires that the LSM named "integrity" has been initialized.
Since ima and evm have separate indicators for "am I active?" (much like
apparmor, etc), it seemed sensible to make ima and evm part of the LSM
named "integrity". Other solutions are totally fine!
I do note, however, this patch needs to be tweaked for the case where
CONFIG_IMA or CONFIG_EVM are not set.
On Mon, Oct 17, 2022 at 11:26:44AM +0200, Mickaël Salaün wrote:
>
> On 14/10/2022 19:59, Kees Cook wrote:
> > On Fri, Oct 14, 2022 at 04:40:01PM +0200, Mickaël Salaün wrote:
> > > This is not backward compatible
> >
> > Why? Nothing will be running LSM hooks until init finishes, at which
> > point the integrity inode cache will be allocated. And ima and evm don't
> > start up until lateinit.
> >
> > > , but can easily be fixed thanks to
> > > DEFINE_LSM().order
> >
> > That forces the LSM to be enabled, which may not be desired?
>
> This is not backward compatible because currently IMA is enabled
> independently of the "lsm=" cmdline, which means that for all installed
> systems using IMA and also with a custom "lsm=" cmdline, updating the kernel
> with this patch will (silently) disable IMA. Using ".order =
> LSM_ORDER_FIRST," should keep this behavior.
This isn't true. If "integrity" is removed from the lsm= line today, IMA
will immediately panic:
process_measurement():
integrity_inode_get():
if (!iint_cache)
panic("%s: lsm=integrity required.\n", __func__);
and before v5.12 (where the panic was added), it would immediately NULL
deref. (And it took 3 years to even notice.)
On Wed, 2022-10-19 at 11:33 -0700, Kees Cook wrote:
> On Mon, Oct 17, 2022 at 11:26:44AM +0200, Mickaël Salaün wrote:
> >
> > On 14/10/2022 19:59, Kees Cook wrote:
> > > On Fri, Oct 14, 2022 at 04:40:01PM +0200, Mickaël Salaün wrote:
> > > > This is not backward compatible
> > >
> > > Why? Nothing will be running LSM hooks until init finishes, at which
> > > point the integrity inode cache will be allocated. And ima and evm don't
> > > start up until lateinit.
> > >
> > > > , but can easily be fixed thanks to
> > > > DEFINE_LSM().order
> > >
> > > That forces the LSM to be enabled, which may not be desired?
> >
> > This is not backward compatible because currently IMA is enabled
> > independently of the "lsm=" cmdline, which means that for all installed
> > systems using IMA and also with a custom "lsm=" cmdline, updating the kernel
> > with this patch will (silently) disable IMA. Using ".order =
> > LSM_ORDER_FIRST," should keep this behavior.
>
> This isn't true. If "integrity" is removed from the lsm= line today, IMA
> will immediately panic:
>
> process_measurement():
> integrity_inode_get():
> if (!iint_cache)
> panic("%s: lsm=integrity required.\n", __func__);
>
> and before v5.12 (where the panic was added), it would immediately NULL
> deref. (And it took 3 years to even notice.)
Most people were/are still using the "security=" boot command line
option, not "lsm=". This previously wasn't a problem with "security=",
but became a problem with "lsm=". I should have been aware of the
change from "security=" to "lsm=", but unfortunately wasn't. It took
me totally by surprise. All of sudden "integrity" went from being a
common IMA/EVM resource to an LSM. The correct solution would have
been to move it a different initcall. (It's not too late to fix it.)
On Wed, Oct 19, 2022 at 03:13:34PM -0400, Mimi Zohar wrote:
> Most people were/are still using the "security=" boot command line
> option, not "lsm=". This previously wasn't a problem with "security=",
> but became a problem with "lsm=".
How are people still using "security=" for IMA/EVM? It only interacts
with LSMs marked with LSM_FLAG_LEGACY_MAJOR.
@@ -246,11 +246,11 @@ endchoice
config LSM
string "Ordered list of enabled LSMs"
- default "landlock,lockdown,yama,loadpin,safesetid,integrity,smack,selinux,tomoyo,apparmor,bpf" if DEFAULT_SECURITY_SMACK
- default "landlock,lockdown,yama,loadpin,safesetid,integrity,apparmor,selinux,smack,tomoyo,bpf" if DEFAULT_SECURITY_APPARMOR
- default "landlock,lockdown,yama,loadpin,safesetid,integrity,tomoyo,bpf" if DEFAULT_SECURITY_TOMOYO
- default "landlock,lockdown,yama,loadpin,safesetid,integrity,bpf" if DEFAULT_SECURITY_DAC
- default "landlock,lockdown,yama,loadpin,safesetid,integrity,selinux,smack,tomoyo,apparmor,bpf"
+ default "landlock,lockdown,yama,loadpin,safesetid,smack,selinux,tomoyo,apparmor,bpf,integrity" if DEFAULT_SECURITY_SMACK
+ default "landlock,lockdown,yama,loadpin,safesetid,apparmor,selinux,smack,tomoyo,bpf,integrity" if DEFAULT_SECURITY_APPARMOR
+ default "landlock,lockdown,yama,loadpin,safesetid,tomoyo,bpf,integrity" if DEFAULT_SECURITY_TOMOYO
+ default "landlock,lockdown,yama,loadpin,safesetid,bpf,integrity" if DEFAULT_SECURITY_DAC
+ default "landlock,lockdown,yama,loadpin,safesetid,selinux,smack,tomoyo,apparmor,bpf,integrity"
help
A comma-separated list of LSMs, in initialization order.
Any LSMs left off this list will be ignored. This can be
@@ -904,3 +904,7 @@ static int __init init_evm(void)
}
late_initcall(init_evm);
+
+void __init integrity_lsm_evm_init(void)
+{
+}
@@ -18,7 +18,6 @@
#include <linux/file.h>
#include <linux/uaccess.h>
#include <linux/security.h>
-#include <linux/lsm_hooks.h>
#include "integrity.h"
static struct rb_root integrity_iint_tree = RB_ROOT;
@@ -172,19 +171,29 @@ static void init_once(void *foo)
mutex_init(&iint->mutex);
}
-static int __init integrity_iintcache_init(void)
+void __init integrity_add_lsm_hooks(struct security_hook_list *hooks,
+ int count)
+{
+ security_add_hooks(hooks, count, "integrity");
+}
+
+static int __init integrity_lsm_init(void)
{
iint_cache =
kmem_cache_create("iint_cache", sizeof(struct integrity_iint_cache),
0, SLAB_PANIC, init_once);
+
+ integrity_lsm_ima_init();
+ integrity_lsm_evm_init();
+
return 0;
}
+
DEFINE_LSM(integrity) = {
.name = "integrity",
- .init = integrity_iintcache_init,
+ .init = integrity_lsm_init,
};
-
/*
* integrity_kernel_read - read data from the file
*
@@ -1076,3 +1076,7 @@ static int __init init_ima(void)
}
late_initcall(init_ima); /* Start IMA after the TPM is available */
+
+void __init integrity_lsm_ima_init(void)
+{
+}
@@ -18,6 +18,7 @@
#include <crypto/hash.h>
#include <linux/key.h>
#include <linux/audit.h>
+#include <linux/lsm_hooks.h>
/* iint action cache flags */
#define IMA_MEASURE 0x00000001
@@ -191,6 +192,11 @@ extern struct dentry *integrity_dir;
struct modsig;
+void __init integrity_lsm_ima_init(void);
+void __init integrity_lsm_evm_init(void);
+void __init integrity_add_lsm_hooks(struct security_hook_list *hooks,
+ int count);
+
#ifdef CONFIG_INTEGRITY_SIGNATURE
int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,