[PATCH 0/9] integrity: Move hooks into LSM

[PATCH 0/9] integrity: Move hooks into LSM

[Kees Cook]

Hi,

It's been over 4 years since LSM stack was introduced. The integrity
subsystem is long overdue for moving to this infrastructure. Here's my
first pass at converting integrity and ima (and some of evm) into LSM
hooks. This should be enough of an example to finish evm, and introduce
the missing hooks for both. For example, after this, it looks like ima
only has a couple places it's still doing things outside of the LSM. At
least these stood out:

fs/namei.c:     ima_post_create_tmpfile(mnt_userns, inode);
fs/namei.c:                             ima_post_path_mknod(mnt_userns, dentry);

Mimi, can you please take this series and finish the conversion for
what's missing in ima and evm?

I would also call attention to "175 insertions(+), 240 deletions(-)" --
as expected, this is a net reduction in code.

Thanks!

-Kees

Kees Cook (9):
  integrity: Prepare for having "ima" and "evm" available in "integrity"
    LSM
  security: Move trivial IMA hooks into LSM
  ima: Move xattr hooks into LSM
  ima: Move ima_file_free() into LSM
  LSM: Introduce inode_post_setattr hook
  fs: Introduce file_to_perms() helper
  ima: Move ima_file_check() into LSM
  integrity: Move trivial hooks into LSM
  integrity: Move integrity_inode_get() out of global header

 fs/attr.c                             |  3 +-
 fs/file_table.c                       |  1 -
 fs/namei.c                            |  2 -
 fs/nfsd/vfs.c                         |  6 --
 include/linux/evm.h                   |  6 --
 include/linux/fs.h                    | 22 +++++++
 include/linux/ima.h                   | 87 ---------------------------
 include/linux/integrity.h             | 30 +--------
 include/linux/lsm_hook_defs.h         |  3 +
 security/Kconfig                      | 10 +--
 security/apparmor/include/file.h      | 18 ++----
 security/integrity/evm/evm_main.c     | 14 ++++-
 security/integrity/iint.c             | 28 +++++++--
 security/integrity/ima/ima.h          | 12 ++++
 security/integrity/ima/ima_appraise.c | 21 +++++--
 security/integrity/ima/ima_main.c     | 66 ++++++++++++++------
 security/integrity/integrity.h        |  8 +++
 security/security.c                   | 78 ++++++------------------
 18 files changed, 175 insertions(+), 240 deletions(-)

-- 
2.34.1

[PATCH 1/9] integrity: Prepare for having "ima" and "evm" available in "integrity" LSM

[Kees Cook]

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"
 	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,
 };
 
-
 /*
  * integrity_kernel_read - read data from the file
  *
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 040b03ddc1c7..e617863af5ff 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -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)
+{
+}
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 7167a6e99bdc..3707349271c9 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -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,
-- 
2.34.1

Re: [PATCH 1/9] integrity: Prepare for having "ima" and "evm" available in "integrity" LSM

[Mickaël Salaün]

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.

Re: [PATCH 1/9] integrity: Prepare for having "ima" and "evm" available in "integrity" LSM

[Kees Cook]

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. ;)

-- 
Kees Cook

Re: [PATCH 1/9] integrity: Prepare for having "ima" and "evm" available in "integrity" LSM

[Mickaël Salaün]

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.

Re: [PATCH 1/9] integrity: Prepare for having "ima" and "evm" available in "integrity" LSM

[Kees Cook]

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...

-- 
Kees Cook

Re: [PATCH 1/9] integrity: Prepare for having "ima" and "evm" available in "integrity" LSM

[Kees Cook]

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.)

-- 
Kees Cook

Re: [PATCH 1/9] integrity: Prepare for having "ima" and "evm" available in "integrity" LSM

[Mimi Zohar]

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.)

-- 
thanks,

Mimi

Re: [PATCH 1/9] integrity: Prepare for having "ima" and "evm" available in "integrity" LSM

[Kees Cook]

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.

-- 
Kees Cook

Re: [PATCH 1/9] integrity: Prepare for having "ima" and "evm" available in "integrity" LSM

[Mimi Zohar]

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.
-- 
thanks,

Mimi

Re: [PATCH 1/9] integrity: Prepare for having "ima" and "evm" available in "integrity" LSM

[Kees Cook]

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.

-- 
Kees Cook

[PATCH 2/9] security: Move trivial IMA hooks into LSM

[Kees Cook]

This moves the trivial hard-coded stacking of IMA LSM hooks into the
existing LSM infrastructure.

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: Petr Vorel <pvorel@suse.cz>
Cc: Borislav Petkov <bp@suse.de>
Cc: Takashi Iwai <tiwai@suse.de>
Cc: Jonathan McDowell <noodles@fb.com>
Cc: linux-security-module@vger.kernel.org
Cc: linux-integrity@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/ima.h               | 50 -----------------------------
 security/integrity/ima/ima_main.c | 40 +++++++++++++++++-------
 security/security.c               | 52 ++++++-------------------------
 3 files changed, 37 insertions(+), 105 deletions(-)

diff --git a/include/linux/ima.h b/include/linux/ima.h
index 81708ca0ebc7..3c641cc65270 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -16,20 +16,10 @@ struct linux_binprm;
 
 #ifdef CONFIG_IMA
 extern enum hash_algo ima_get_current_hash_algo(void);
-extern int ima_bprm_check(struct linux_binprm *bprm);
 extern int ima_file_check(struct file *file, int mask);
 extern void ima_post_create_tmpfile(struct user_namespace *mnt_userns,
 				    struct inode *inode);
 extern void ima_file_free(struct file *file);
-extern int ima_file_mmap(struct file *file, unsigned long prot);
-extern int ima_file_mprotect(struct vm_area_struct *vma, unsigned long prot);
-extern int ima_load_data(enum kernel_load_data_id id, bool contents);
-extern int ima_post_load_data(char *buf, loff_t size,
-			      enum kernel_load_data_id id, char *description);
-extern int ima_read_file(struct file *file, enum kernel_read_file_id id,
-			 bool contents);
-extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
-			      enum kernel_read_file_id id);
 extern void ima_post_path_mknod(struct user_namespace *mnt_userns,
 				struct dentry *dentry);
 extern int ima_file_hash(struct file *file, char *buf, size_t buf_size);
@@ -56,11 +46,6 @@ static inline enum hash_algo ima_get_current_hash_algo(void)
 	return HASH_ALGO__LAST;
 }
 
-static inline int ima_bprm_check(struct linux_binprm *bprm)
-{
-	return 0;
-}
-
 static inline int ima_file_check(struct file *file, int mask)
 {
 	return 0;
@@ -76,41 +61,6 @@ static inline void ima_file_free(struct file *file)
 	return;
 }
 
-static inline int ima_file_mmap(struct file *file, unsigned long prot)
-{
-	return 0;
-}
-
-static inline int ima_file_mprotect(struct vm_area_struct *vma,
-				    unsigned long prot)
-{
-	return 0;
-}
-
-static inline int ima_load_data(enum kernel_load_data_id id, bool contents)
-{
-	return 0;
-}
-
-static inline int ima_post_load_data(char *buf, loff_t size,
-				     enum kernel_load_data_id id,
-				     char *description)
-{
-	return 0;
-}
-
-static inline int ima_read_file(struct file *file, enum kernel_read_file_id id,
-				bool contents)
-{
-	return 0;
-}
-
-static inline int ima_post_read_file(struct file *file, void *buf, loff_t size,
-				     enum kernel_read_file_id id)
-{
-	return 0;
-}
-
 static inline void ima_post_path_mknod(struct user_namespace *mnt_userns,
 				       struct dentry *dentry)
 {
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index e617863af5ff..2cff001b02e4 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -395,6 +395,7 @@ static int process_measurement(struct file *file, const struct cred *cred,
 /**
  * ima_file_mmap - based on policy, collect/store measurement.
  * @file: pointer to the file to be measured (May be NULL)
+ * @reqprot: contains the protection that will be applied by the kernel.
  * @prot: contains the protection that will be applied by the kernel.
  *
  * Measure files being mmapped executable based on the ima_must_measure()
@@ -403,11 +404,12 @@ static int process_measurement(struct file *file, const struct cred *cred,
  * On success return 0.  On integrity appraisal error, assuming the file
  * is in policy and IMA-appraisal is in enforcing mode, return -EACCES.
  */
-int ima_file_mmap(struct file *file, unsigned long prot)
+static int ima_file_mmap(struct file *file, unsigned long reqprot,
+			 unsigned long prot, unsigned long flags)
 {
 	u32 secid;
 
-	if (file && (prot & PROT_EXEC)) {
+	if (file && (reqprot & PROT_EXEC)) {
 		security_current_getsecid_subj(&secid);
 		return process_measurement(file, current_cred(), secid, NULL,
 					   0, MAY_EXEC, MMAP_CHECK);
@@ -419,6 +421,7 @@ int ima_file_mmap(struct file *file, unsigned long prot)
 /**
  * ima_file_mprotect - based on policy, limit mprotect change
  * @vma: vm_area_struct protection is set to
+ * @reqprot: contains the protection that were requested.
  * @prot: contains the protection that will be applied by the kernel.
  *
  * Files can be mmap'ed read/write and later changed to execute to circumvent
@@ -429,7 +432,8 @@ int ima_file_mmap(struct file *file, unsigned long prot)
  *
  * On mprotect change success, return 0.  On failure, return -EACESS.
  */
-int ima_file_mprotect(struct vm_area_struct *vma, unsigned long prot)
+static int ima_file_mprotect(struct vm_area_struct *vma,
+			     unsigned long reqprot, unsigned long prot)
 {
 	struct ima_template_desc *template = NULL;
 	struct file *file;
@@ -483,7 +487,7 @@ int ima_file_mprotect(struct vm_area_struct *vma, unsigned long prot)
  * On success return 0.  On integrity appraisal error, assuming the file
  * is in policy and IMA-appraisal is in enforcing mode, return -EACCES.
  */
-int ima_bprm_check(struct linux_binprm *bprm)
+static int ima_bprm_check(struct linux_binprm *bprm)
 {
 	int ret;
 	u32 secid;
@@ -706,8 +710,8 @@ void ima_post_path_mknod(struct user_namespace *mnt_userns,
  *
  * For permission return 0, otherwise return -EACCES.
  */
-int ima_read_file(struct file *file, enum kernel_read_file_id read_id,
-		  bool contents)
+static int ima_read_file(struct file *file, enum kernel_read_file_id read_id,
+			 bool contents)
 {
 	enum ima_hooks func;
 	u32 secid;
@@ -756,8 +760,8 @@ const int read_idmap[READING_MAX_ID] = {
  * On success return 0.  On integrity appraisal error, assuming the file
  * is in policy and IMA-appraisal is in enforcing mode, return -EACCES.
  */
-int ima_post_read_file(struct file *file, void *buf, loff_t size,
-		       enum kernel_read_file_id read_id)
+static int ima_post_read_file(struct file *file, char *buf, loff_t size,
+			      enum kernel_read_file_id read_id)
 {
 	enum ima_hooks func;
 	u32 secid;
@@ -790,7 +794,7 @@ int ima_post_read_file(struct file *file, void *buf, loff_t size,
  *
  * For permission return 0, otherwise return -EACCES.
  */
-int ima_load_data(enum kernel_load_data_id id, bool contents)
+static int ima_load_data(enum kernel_load_data_id id, bool contents)
 {
 	bool ima_enforce, sig_enforce;
 
@@ -844,9 +848,9 @@ int ima_load_data(enum kernel_load_data_id id, bool contents)
  * On success return 0.  On integrity appraisal error, assuming the file
  * is in policy and IMA-appraisal is in enforcing mode, return -EACCES.
  */
-int ima_post_load_data(char *buf, loff_t size,
-		       enum kernel_load_data_id load_id,
-		       char *description)
+static int ima_post_load_data(char *buf, loff_t size,
+			      enum kernel_load_data_id load_id,
+			      char *description)
 {
 	if (load_id == LOADING_FIRMWARE) {
 		if ((ima_appraise & IMA_APPRAISE_FIRMWARE) &&
@@ -1077,6 +1081,18 @@ static int __init init_ima(void)
 
 late_initcall(init_ima);	/* Start IMA after the TPM is available */
 
+static struct security_hook_list ima_hooks[] __lsm_ro_after_init = {
+	LSM_HOOK_INIT(bprm_check_security, ima_bprm_check),
+	LSM_HOOK_INIT(mmap_file, ima_file_mmap),
+	LSM_HOOK_INIT(file_mprotect, ima_file_mprotect),
+	LSM_HOOK_INIT(kernel_read_file, ima_read_file),
+	LSM_HOOK_INIT(kernel_post_read_file, ima_post_read_file),
+	LSM_HOOK_INIT(kernel_load_data, ima_load_data),
+	LSM_HOOK_INIT(kernel_post_load_data, ima_post_load_data),
+};
+
 void __init integrity_lsm_ima_init(void)
 {
+	pr_info("Integrity LSM enabling IMA\n");
+	integrity_add_lsm_hooks(ima_hooks, ARRAY_SIZE(ima_hooks));
 }
diff --git a/security/security.c b/security/security.c
index 14d30fec8a00..8f7c1b5fa5fa 100644
--- a/security/security.c
+++ b/security/security.c
@@ -862,12 +862,7 @@ int security_bprm_creds_from_file(struct linux_binprm *bprm, struct file *file)
 
 int security_bprm_check(struct linux_binprm *bprm)
 {
-	int ret;
-
-	ret = call_int_hook(bprm_check_security, 0, bprm);
-	if (ret)
-		return ret;
-	return ima_bprm_check(bprm);
+	return call_int_hook(bprm_check_security, 0, bprm);
 }
 
 void security_bprm_committing_creds(struct linux_binprm *bprm)
@@ -1589,12 +1584,8 @@ static inline unsigned long mmap_prot(struct file *file, unsigned long prot)
 int security_mmap_file(struct file *file, unsigned long prot,
 			unsigned long flags)
 {
-	int ret;
-	ret = call_int_hook(mmap_file, 0, file, prot,
-					mmap_prot(file, prot), flags);
-	if (ret)
-		return ret;
-	return ima_file_mmap(file, prot);
+	return call_int_hook(mmap_file, 0, file, prot,
+			     mmap_prot(file, prot), flags);
 }
 
 int security_mmap_addr(unsigned long addr)
@@ -1605,12 +1596,7 @@ int security_mmap_addr(unsigned long addr)
 int security_file_mprotect(struct vm_area_struct *vma, unsigned long reqprot,
 			    unsigned long prot)
 {
-	int ret;
-
-	ret = call_int_hook(file_mprotect, 0, vma, reqprot, prot);
-	if (ret)
-		return ret;
-	return ima_file_mprotect(vma, prot);
+	return call_int_hook(file_mprotect, 0, vma, reqprot, prot);
 }
 
 int security_file_lock(struct file *file, unsigned int cmd)
@@ -1746,35 +1732,20 @@ int security_kernel_module_request(char *kmod_name)
 int security_kernel_read_file(struct file *file, enum kernel_read_file_id id,
 			      bool contents)
 {
-	int ret;
-
-	ret = call_int_hook(kernel_read_file, 0, file, id, contents);
-	if (ret)
-		return ret;
-	return ima_read_file(file, id, contents);
+	return call_int_hook(kernel_read_file, 0, file, id, contents);
 }
 EXPORT_SYMBOL_GPL(security_kernel_read_file);
 
 int security_kernel_post_read_file(struct file *file, char *buf, loff_t size,
 				   enum kernel_read_file_id id)
 {
-	int ret;
-
-	ret = call_int_hook(kernel_post_read_file, 0, file, buf, size, id);
-	if (ret)
-		return ret;
-	return ima_post_read_file(file, buf, size, id);
+	return call_int_hook(kernel_post_read_file, 0, file, buf, size, id);
 }
 EXPORT_SYMBOL_GPL(security_kernel_post_read_file);
 
 int security_kernel_load_data(enum kernel_load_data_id id, bool contents)
 {
-	int ret;
-
-	ret = call_int_hook(kernel_load_data, 0, id, contents);
-	if (ret)
-		return ret;
-	return ima_load_data(id, contents);
+	return call_int_hook(kernel_load_data, 0, id, contents);
 }
 EXPORT_SYMBOL_GPL(security_kernel_load_data);
 
@@ -1782,13 +1753,8 @@ int security_kernel_post_load_data(char *buf, loff_t size,
 				   enum kernel_load_data_id id,
 				   char *description)
 {
-	int ret;
-
-	ret = call_int_hook(kernel_post_load_data, 0, buf, size, id,
-			    description);
-	if (ret)
-		return ret;
-	return ima_post_load_data(buf, size, id, description);
+	return call_int_hook(kernel_post_load_data, 0, buf, size, id,
+			     description);
 }
 EXPORT_SYMBOL_GPL(security_kernel_post_load_data);
 
-- 
2.34.1

Re: [PATCH 2/9] security: Move trivial IMA hooks into LSM

[Mimi Zohar]

On Thu, 2022-10-13 at 15:36 -0700, Kees Cook wrote:
> This moves the trivial hard-coded stacking of IMA LSM hooks into the
> existing LSM infrastructure.

The only thing trivial about making IMA and EVM LSMs is moving them to
LSM hooks.  Although static files may be signed and the signatures
distributed with the file data through the normal distribution
mechanisms (e.g. RPM), other files cannot be signed remotely (e.g.
configuration files).  For these files, both IMA and EVM may be
configured to maintain persistent file state stored as security xattrs
in the form of security.ima file hashes or security.evm HMACs.  The LSM
flexibility of enabling/disabling IMA or EVM on a per boot basis breaks
this usage, potentially preventing subsequent boots.
-- 
thanks,

Mimi

Re: [PATCH 2/9] security: Move trivial IMA hooks into LSM

[Kees Cook]

On Wed, Oct 19, 2022 at 10:34:48AM -0400, Mimi Zohar wrote:
> On Thu, 2022-10-13 at 15:36 -0700, Kees Cook wrote:
> > This moves the trivial hard-coded stacking of IMA LSM hooks into the
> > existing LSM infrastructure.
> 
> The only thing trivial about making IMA and EVM LSMs is moving them to
> LSM hooks.  Although static files may be signed and the signatures
> distributed with the file data through the normal distribution
> mechanisms (e.g. RPM), other files cannot be signed remotely (e.g.
> configuration files).  For these files, both IMA and EVM may be
> configured to maintain persistent file state stored as security xattrs
> in the form of security.ima file hashes or security.evm HMACs.  The LSM
> flexibility of enabling/disabling IMA or EVM on a per boot basis breaks
> this usage, potentially preventing subsequent boots.

I'm not suggesting IMA and EVM don't have specific behaviors that need to
be correctly integrated into the LSM infrastructure. In fact, I spent a
lot of time designing that infrastructure to be flexible enough to deal
with these kinds of things. (e.g. plumbing "enablement", etc.) As I
mentioned, this was more of trying to provide a head-start on the
conversion. I don't intend to drive this -- please take whatever is
useful from this example and use it. :) I'm happy to help construct any
missing infrastructure needed (e.g. LSM_ORDER_LAST, etc).

As for preventing subsequent boots, this is already true with other LSMs
that save state that affects system behavior (like SELinux tags, AppArmor
policy). IMA and EVM are not special in that regard conceptually.

Besides, it also looks like it's already possible to boot with IMA or EVM
disabled ("ima_appraise=off", or "evm=fix"), so there's no regression
conceptually for having "integrity" get dropped from the lsm= list at
boot. And if you want it not to be silent disabling, that's fine --
just panic during initialization if "integrity" is disabled, as is
already happening.

Note that, generally speaking, LSMs have three initialization points:
LSM init, fs_initcall, and late_initcall:

$ grep -R _initcall security/*/ | wc -l
31

This, again, isn't different for IMA or EVM. The LSM infrastructure is
about gathering and standardizing the requirements needed to run security
hooks in a common way. The goal isn't to break IMA/EVM -- anything
needed can be created for it. The goal is to remove _exceptions_ to the
common hook mechanism.

BTW, are there examples of how to test an IMA/EVM system? I couldn't
find any pre-existing test images one can boot in QEMU, or instructions
on how to create such an image, but I could have missed it.

-- 
Kees Cook

Re: [PATCH 2/9] security: Move trivial IMA hooks into LSM

[Mimi Zohar]

On Wed, 2022-10-19 at 11:59 -0700, Kees Cook wrote:
> On Wed, Oct 19, 2022 at 10:34:48AM -0400, Mimi Zohar wrote:
> > On Thu, 2022-10-13 at 15:36 -0700, Kees Cook wrote:
> > > This moves the trivial hard-coded stacking of IMA LSM hooks into the
> > > existing LSM infrastructure.
> > 
> > The only thing trivial about making IMA and EVM LSMs is moving them to
> > LSM hooks.  Although static files may be signed and the signatures
> > distributed with the file data through the normal distribution
> > mechanisms (e.g. RPM), other files cannot be signed remotely (e.g.
> > configuration files).  For these files, both IMA and EVM may be
> > configured to maintain persistent file state stored as security xattrs
> > in the form of security.ima file hashes or security.evm HMACs.  The LSM
> > flexibility of enabling/disabling IMA or EVM on a per boot basis breaks
> > this usage, potentially preventing subsequent boots.
> 
> I'm not suggesting IMA and EVM don't have specific behaviors that need to
> be correctly integrated into the LSM infrastructure. In fact, I spent a
> lot of time designing that infrastructure to be flexible enough to deal
> with these kinds of things. (e.g. plumbing "enablement", etc.) As I
> mentioned, this was more of trying to provide a head-start on the
> conversion. I don't intend to drive this -- please take whatever is
> useful from this example and use it. :) I'm happy to help construct any
> missing infrastructure needed (e.g. LSM_ORDER_LAST, etc).
> 
> As for preventing subsequent boots, this is already true with other LSMs
> that save state that affects system behavior (like SELinux tags, AppArmor
> policy). IMA and EVM are not special in that regard conceptually.

> Besides, it also looks like it's already possible to boot with IMA or EVM
> disabled ("ima_appraise=off", or "evm=fix"), so there's no regression
> conceptually for having "integrity" get dropped from the lsm= list at
> boot. And if you want it not to be silent disabling, that's fine --
> just panic during initialization if "integrity" is disabled, as is
> already happening.

Being able to specify "ima_appraise=" on the boot command line requires
IMA_APPRAISE_BOOTPARAM to be configured.  Even when specified, if the
system is booted with secure-boot mode enabled, it also cannot be
modified.   With the ability of randomly enabling/disabling LSMs, these
protections are useless.

> 
> Note that, generally speaking, LSMs have three initialization points:
> LSM init, fs_initcall, and late_initcall:

IMA initialization is deferred to late_initcall to allow the TPM to
finish initializing.  It doesn't make a difference when the iint_cache
is initialized.  It just needs to be prior to IMA/EVM initializiation.

> 
> $ grep -R _initcall security/*/ | wc -l
> 31
> 
> This, again, isn't different for IMA or EVM. The LSM infrastructure is
> about gathering and standardizing the requirements needed to run security
> hooks in a common way. The goal isn't to break IMA/EVM -- anything
> needed can be created for it. The goal is to remove _exceptions_ to the
> common hook mechanism.
> 
> BTW, are there examples of how to test an IMA/EVM system? I couldn't
> find any pre-existing test images one can boot in QEMU, or instructions
> on how to create such an image, but I could have missed it.

There are specific tests in LTP, kselftests, and ima-evm-utils, but
they are incomplete.

Re: [PATCH 2/9] security: Move trivial IMA hooks into LSM

[Kees Cook]

On Wed, Oct 19, 2022 at 04:45:41PM -0400, Mimi Zohar wrote:
> On Wed, 2022-10-19 at 11:59 -0700, Kees Cook wrote:
> > On Wed, Oct 19, 2022 at 10:34:48AM -0400, Mimi Zohar wrote:
> > > On Thu, 2022-10-13 at 15:36 -0700, Kees Cook wrote:
> > > > This moves the trivial hard-coded stacking of IMA LSM hooks into the
> > > > existing LSM infrastructure.
> > > 
> > > The only thing trivial about making IMA and EVM LSMs is moving them to
> > > LSM hooks.  Although static files may be signed and the signatures
> > > distributed with the file data through the normal distribution
> > > mechanisms (e.g. RPM), other files cannot be signed remotely (e.g.
> > > configuration files).  For these files, both IMA and EVM may be
> > > configured to maintain persistent file state stored as security xattrs
> > > in the form of security.ima file hashes or security.evm HMACs.  The LSM
> > > flexibility of enabling/disabling IMA or EVM on a per boot basis breaks
> > > this usage, potentially preventing subsequent boots.
> > 
> > I'm not suggesting IMA and EVM don't have specific behaviors that need to
> > be correctly integrated into the LSM infrastructure. In fact, I spent a
> > lot of time designing that infrastructure to be flexible enough to deal
> > with these kinds of things. (e.g. plumbing "enablement", etc.) As I
> > mentioned, this was more of trying to provide a head-start on the
> > conversion. I don't intend to drive this -- please take whatever is
> > useful from this example and use it. :) I'm happy to help construct any
> > missing infrastructure needed (e.g. LSM_ORDER_LAST, etc).
> > 
> > As for preventing subsequent boots, this is already true with other LSMs
> > that save state that affects system behavior (like SELinux tags, AppArmor
> > policy). IMA and EVM are not special in that regard conceptually.
> 
> > Besides, it also looks like it's already possible to boot with IMA or EVM
> > disabled ("ima_appraise=off", or "evm=fix"), so there's no regression
> > conceptually for having "integrity" get dropped from the lsm= list at
> > boot. And if you want it not to be silent disabling, that's fine --
> > just panic during initialization if "integrity" is disabled, as is
> > already happening.
> 
> Being able to specify "ima_appraise=" on the boot command line requires
> IMA_APPRAISE_BOOTPARAM to be configured.  Even when specified, if the
> system is booted with secure-boot mode enabled, it also cannot be
> modified.   With the ability of randomly enabling/disabling LSMs, these
> protections are useless.

Sure, so let's get lsm= added to the lockdown list, etc. My point is for
us to work through each of these concerns and address them. I am not an
IMA/EVM expert, but I do understand the LSM infrastructure deeply, so
I'd like to help you get these changes made.

-- 
Kees Cook

Re: [PATCH 2/9] security: Move trivial IMA hooks into LSM

[Mimi Zohar]

On Wed, 2022-10-19 at 16:41 -0700, Kees Cook wrote:
> On Wed, Oct 19, 2022 at 04:45:41PM -0400, Mimi Zohar wrote:
> > On Wed, 2022-10-19 at 11:59 -0700, Kees Cook wrote:
> > > On Wed, Oct 19, 2022 at 10:34:48AM -0400, Mimi Zohar wrote:
> > > > On Thu, 2022-10-13 at 15:36 -0700, Kees Cook wrote:
> > > > > This moves the trivial hard-coded stacking of IMA LSM hooks into the
> > > > > existing LSM infrastructure.
> > > > 
> > > > The only thing trivial about making IMA and EVM LSMs is moving them to
> > > > LSM hooks.  Although static files may be signed and the signatures
> > > > distributed with the file data through the normal distribution
> > > > mechanisms (e.g. RPM), other files cannot be signed remotely (e.g.
> > > > configuration files).  For these files, both IMA and EVM may be
> > > > configured to maintain persistent file state stored as security xattrs
> > > > in the form of security.ima file hashes or security.evm HMACs.  The LSM
> > > > flexibility of enabling/disabling IMA or EVM on a per boot basis breaks
> > > > this usage, potentially preventing subsequent boots.
> > > 
> > > I'm not suggesting IMA and EVM don't have specific behaviors that need to
> > > be correctly integrated into the LSM infrastructure. In fact, I spent a
> > > lot of time designing that infrastructure to be flexible enough to deal
> > > with these kinds of things. (e.g. plumbing "enablement", etc.) As I
> > > mentioned, this was more of trying to provide a head-start on the
> > > conversion. I don't intend to drive this -- please take whatever is
> > > useful from this example and use it. :) I'm happy to help construct any
> > > missing infrastructure needed (e.g. LSM_ORDER_LAST, etc).
> > > 
> > > As for preventing subsequent boots, this is already true with other LSMs
> > > that save state that affects system behavior (like SELinux tags, AppArmor
> > > policy). IMA and EVM are not special in that regard conceptually.
> > 
> > > Besides, it also looks like it's already possible to boot with IMA or EVM
> > > disabled ("ima_appraise=off", or "evm=fix"), so there's no regression
> > > conceptually for having "integrity" get dropped from the lsm= list at
> > > boot. And if you want it not to be silent disabling, that's fine --
> > > just panic during initialization if "integrity" is disabled, as is
> > > already happening.
> > 
> > Being able to specify "ima_appraise=" on the boot command line requires
> > IMA_APPRAISE_BOOTPARAM to be configured.  Even when specified, if the
> > system is booted with secure-boot mode enabled, it also cannot be
> > modified.   With the ability of randomly enabling/disabling LSMs, these
> > protections are useless.
> 
> Sure, so let's get lsm= added to the lockdown list, etc.

I thought the move to "lsm=" was to allow different LSMs to be
enabled/disabled at run time.  Adding "lsm=" to the lockdown list
doesn't seem like the correct solution to limiting which LSMs can be
enabled/disabled at runtime.  As I recall, lockdown needs to be enabled
by userspace.

> My point is for
> us to work through each of these concerns and address them. I am not an
> IMA/EVM expert, but I do understand the LSM infrastructure deeply, so
> I'd like to help you get these changes made.

Sure

-- 
thanks,

Mimi

Re: [PATCH 2/9] security: Move trivial IMA hooks into LSM

[Dr. Greg]

On Wed, Oct 19, 2022 at 11:59:40AM -0700, Kees Cook wrote:

Good morning, I hope the week is ending well for everyone.

> On Wed, Oct 19, 2022 at 10:34:48AM -0400, Mimi Zohar wrote:
> >
> > The only thing trivial about making IMA and EVM LSMs is moving
> > them to LSM hooks.  Although static files may be signed and the
> > signatures distributed with the file data through the normal
> > distribution mechanisms (e.g. RPM), other files cannot be signed
> > remotely (e.g.  configuration files).  For these files, both IMA
> > and EVM may be configured to maintain persistent file state stored
> > as security xattrs in the form of security.ima file hashes or
> > security.evm HMACs.  The LSM flexibility of enabling/disabling IMA
> > or EVM on a per boot basis breaks this usage, potentially
> > preventing subsequent boots.

> I'm not suggesting IMA and EVM don't have specific behaviors that
> need to be correctly integrated into the LSM infrastructure. In
> fact, I spent a lot of time designing that infrastructure to be
> flexible enough to deal with these kinds of things. (e.g. plumbing
> "enablement", etc.) As I mentioned, this was more of trying to
> provide a head-start on the conversion. I don't intend to drive this
> -- please take whatever is useful from this example and use it. :)
> I'm happy to help construct any missing infrastructure needed
> (e.g. LSM_ORDER_LAST, etc).

We are 2-3 weeks out from submitting for review and inclusion in the
kernel, a new LSM, and an associated userspace stack, that will have a
high degree of significance with respect to these conversations.

> Kees Cook

Best wishes for a pleasant fall weekend.

As always,

Dr. Greg
The Quixote Project - Flailing at the Travails of Cybersecurity

Re: [PATCH 2/9] security: Move trivial IMA hooks into LSM

[Casey Schaufler]

On 10/21/2022 7:53 AM, Dr. Greg wrote:
> On Wed, Oct 19, 2022 at 11:59:40AM -0700, Kees Cook wrote:
>
> Good morning, I hope the week is ending well for everyone.
>
>> On Wed, Oct 19, 2022 at 10:34:48AM -0400, Mimi Zohar wrote:
>>> The only thing trivial about making IMA and EVM LSMs is moving
>>> them to LSM hooks.  Although static files may be signed and the
>>> signatures distributed with the file data through the normal
>>> distribution mechanisms (e.g. RPM), other files cannot be signed
>>> remotely (e.g.  configuration files).  For these files, both IMA
>>> and EVM may be configured to maintain persistent file state stored
>>> as security xattrs in the form of security.ima file hashes or
>>> security.evm HMACs.  The LSM flexibility of enabling/disabling IMA
>>> or EVM on a per boot basis breaks this usage, potentially
>>> preventing subsequent boots.
>> I'm not suggesting IMA and EVM don't have specific behaviors that
>> need to be correctly integrated into the LSM infrastructure. In
>> fact, I spent a lot of time designing that infrastructure to be
>> flexible enough to deal with these kinds of things. (e.g. plumbing
>> "enablement", etc.) As I mentioned, this was more of trying to
>> provide a head-start on the conversion. I don't intend to drive this
>> -- please take whatever is useful from this example and use it. :)
>> I'm happy to help construct any missing infrastructure needed
>> (e.g. LSM_ORDER_LAST, etc).
> We are 2-3 weeks out from submitting for review and inclusion in the
> kernel, a new LSM, and an associated userspace stack, that will have a
> high degree of significance with respect to these conversations.

Oh, come on, No one likes a teaser trailer. ;)

>
>> Kees Cook
> Best wishes for a pleasant fall weekend.
>
> As always,
>
> Dr. Greg
> The Quixote Project - Flailing at the Travails of Cybersecurity

[PATCH 3/9] ima: Move xattr hooks into LSM

[Kees Cook]

Move the xattr IMA hooks into normal LSM layer. As with SELinux and
Smack, handle calling cap_inode_setxattr() internally.

Cc: Mimi Zohar <zohar@linux.ibm.com>
Cc: Dmitry Kasatkin <dmitry.kasatkin@gmail.com>
Cc: Paul Moore <paul@paul-moore.com>
Cc: James Morris <jmorris@namei.org>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Jonathan McDowell <noodles@fb.com>
Cc: Takashi Iwai <tiwai@suse.de>
Cc: Petr Vorel <pvorel@suse.cz>
Cc: linux-integrity@vger.kernel.org
Cc: linux-security-module@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/ima.h                   | 16 ----------------
 security/integrity/ima/ima.h          | 10 ++++++++++
 security/integrity/ima/ima_appraise.c | 19 ++++++++++++++++---
 security/integrity/ima/ima_main.c     |  4 ++++
 security/security.c                   | 10 ++--------
 5 files changed, 32 insertions(+), 27 deletions(-)

diff --git a/include/linux/ima.h b/include/linux/ima.h
index 3c641cc65270..6dc5143f89f2 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -135,9 +135,6 @@ static inline void ima_post_key_create_or_update(struct key *keyring,
 extern bool is_ima_appraise_enabled(void);
 extern void ima_inode_post_setattr(struct user_namespace *mnt_userns,
 				   struct dentry *dentry);
-extern int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name,
-		       const void *xattr_value, size_t xattr_value_len);
-extern int ima_inode_removexattr(struct dentry *dentry, const char *xattr_name);
 #else
 static inline bool is_ima_appraise_enabled(void)
 {
@@ -150,19 +147,6 @@ static inline void ima_inode_post_setattr(struct user_namespace *mnt_userns,
 	return;
 }
 
-static inline int ima_inode_setxattr(struct dentry *dentry,
-				     const char *xattr_name,
-				     const void *xattr_value,
-				     size_t xattr_value_len)
-{
-	return 0;
-}
-
-static inline int ima_inode_removexattr(struct dentry *dentry,
-					const char *xattr_name)
-{
-	return 0;
-}
 #endif /* CONFIG_IMA_APPRAISE */
 
 #if defined(CONFIG_IMA_APPRAISE) && defined(CONFIG_INTEGRITY_TRUSTED_KEYRING)
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index be965a8715e4..15a369df4c00 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -168,6 +168,16 @@ int __init ima_init_digests(void);
 int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event,
 			  void *lsm_data);
 
+/* LSM hooks */
+#ifdef CONFIG_IMA_APPRAISE
+int ima_inode_setxattr(struct user_namespace *mnt_userns,
+		       struct dentry *dentry, const char *xattr_name,
+		       const void *xattr_value, size_t xattr_value_len,
+		       int flags);
+int ima_inode_removexattr(struct user_namespace *mnt_userns,
+			  struct dentry *dentry, const char *xattr_name);
+#endif
+
 /*
  * used to protect h_table and sha_table
  */
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index bde74fcecee3..ddd9df6b7dac 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -744,8 +744,10 @@ static int validate_hash_algo(struct dentry *dentry,
 	return -EACCES;
 }
 
-int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name,
-		       const void *xattr_value, size_t xattr_value_len)
+int ima_inode_setxattr(struct user_namespace *mnt_userns,
+		       struct dentry *dentry, const char *xattr_name,
+		       const void *xattr_value, size_t xattr_value_len,
+		       int flags)
 {
 	const struct evm_ima_xattr_data *xvalue = xattr_value;
 	int digsig = 0;
@@ -754,6 +756,11 @@ int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name,
 	result = ima_protect_xattr(dentry, xattr_name, xattr_value,
 				   xattr_value_len);
 	if (result == 1) {
+		result = cap_inode_setxattr(dentry, xattr_name, xattr_value,
+					    xattr_value_len, flags);
+		if (result)
+			return result;
+
 		if (!xattr_value_len || (xvalue->type >= IMA_XATTR_LAST))
 			return -EINVAL;
 		digsig = (xvalue->type == EVM_IMA_XATTR_DIGSIG);
@@ -770,11 +777,17 @@ int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name,
 	return result;
 }
 
-int ima_inode_removexattr(struct dentry *dentry, const char *xattr_name)
+int ima_inode_removexattr(struct user_namespace *mnt_userns,
+			  struct dentry *dentry, const char *xattr_name)
 {
 	int result;
 
 	result = ima_protect_xattr(dentry, xattr_name, NULL, 0);
+	if (result == 1) {
+		result = cap_inode_removexattr(mnt_userns, dentry, xattr_name);
+		if (result)
+			return result;
+	}
 	if (result == 1 || evm_revalidate_status(xattr_name)) {
 		ima_reset_appraise_flags(d_backing_inode(dentry), 0);
 		if (result == 1)
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 2cff001b02e4..b3b79d030a67 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -1089,6 +1089,10 @@ static struct security_hook_list ima_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(kernel_post_read_file, ima_post_read_file),
 	LSM_HOOK_INIT(kernel_load_data, ima_load_data),
 	LSM_HOOK_INIT(kernel_post_load_data, ima_post_load_data),
+#ifdef CONFIG_IMA_APPRAISE
+	LSM_HOOK_INIT(inode_setxattr, ima_inode_setxattr),
+	LSM_HOOK_INIT(inode_removexattr, ima_inode_removexattr),
+#endif
 };
 
 void __init integrity_lsm_ima_init(void)
diff --git a/security/security.c b/security/security.c
index 8f7c1b5fa5fa..ca731132a0e9 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1349,7 +1349,7 @@ int security_inode_setxattr(struct user_namespace *mnt_userns,
 	if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
 		return 0;
 	/*
-	 * SELinux and Smack integrate the cap call,
+	 * SELinux, Smack, and IMA integrate the cap call,
 	 * so assume that all LSMs supplying this call do so.
 	 */
 	ret = call_int_hook(inode_setxattr, 1, mnt_userns, dentry, name, value,
@@ -1357,9 +1357,6 @@ int security_inode_setxattr(struct user_namespace *mnt_userns,
 
 	if (ret == 1)
 		ret = cap_inode_setxattr(dentry, name, value, size, flags);
-	if (ret)
-		return ret;
-	ret = ima_inode_setxattr(dentry, name, value, size);
 	if (ret)
 		return ret;
 	return evm_inode_setxattr(mnt_userns, dentry, name, value, size);
@@ -1396,15 +1393,12 @@ int security_inode_removexattr(struct user_namespace *mnt_userns,
 	if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
 		return 0;
 	/*
-	 * SELinux and Smack integrate the cap call,
+	 * SELinux, Smack, and IMA integrate the cap call,
 	 * so assume that all LSMs supplying this call do so.
 	 */
 	ret = call_int_hook(inode_removexattr, 1, mnt_userns, dentry, name);
 	if (ret == 1)
 		ret = cap_inode_removexattr(mnt_userns, dentry, name);
-	if (ret)
-		return ret;
-	ret = ima_inode_removexattr(dentry, name);
 	if (ret)
 		return ret;
 	return evm_inode_removexattr(mnt_userns, dentry, name);
-- 
2.34.1

Re: [PATCH 3/9] ima: Move xattr hooks into LSM

[Christian Brauner]

On Thu, Oct 13, 2022 at 03:36:48PM -0700, Kees Cook wrote:
> Move the xattr IMA hooks into normal LSM layer. As with SELinux and
> Smack, handle calling cap_inode_setxattr() internally.
> 
> Cc: Mimi Zohar <zohar@linux.ibm.com>
> Cc: Dmitry Kasatkin <dmitry.kasatkin@gmail.com>
> Cc: Paul Moore <paul@paul-moore.com>
> Cc: James Morris <jmorris@namei.org>
> Cc: "Serge E. Hallyn" <serge@hallyn.com>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Jonathan McDowell <noodles@fb.com>
> Cc: Takashi Iwai <tiwai@suse.de>
> Cc: Petr Vorel <pvorel@suse.cz>
> Cc: linux-integrity@vger.kernel.org
> Cc: linux-security-module@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---

I like that changes obviously but in general, does IMA depend on being
called _after_ all other LSMs or is this just a historical artifact?

Re: [PATCH 3/9] ima: Move xattr hooks into LSM

[Mimi Zohar]

On Tue, 2022-10-18 at 17:07 +0200, Christian Brauner wrote:
> On Thu, Oct 13, 2022 at 03:36:48PM -0700, Kees Cook wrote:
> > Move the xattr IMA hooks into normal LSM layer. As with SELinux and
> > Smack, handle calling cap_inode_setxattr() internally.
> > 
> > Cc: Mimi Zohar <zohar@linux.ibm.com>
> > Cc: Dmitry Kasatkin <dmitry.kasatkin@gmail.com>
> > Cc: Paul Moore <paul@paul-moore.com>
> > Cc: James Morris <jmorris@namei.org>
> > Cc: "Serge E. Hallyn" <serge@hallyn.com>
> > Cc: Borislav Petkov <bp@suse.de>
> > Cc: Jonathan McDowell <noodles@fb.com>
> > Cc: Takashi Iwai <tiwai@suse.de>
> > Cc: Petr Vorel <pvorel@suse.cz>
> > Cc: linux-integrity@vger.kernel.org
> > Cc: linux-security-module@vger.kernel.org
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > ---
> 
> I like that changes obviously but in general, does IMA depend on being
> called _after_ all other LSMs or is this just a historical artifact?

Calculating the EVM HMAC must be last, after the other security xattrs
have been updated.

-- 
thanks,

Mimi

[PATCH 4/9] ima: Move ima_file_free() into LSM

[Kees Cook]

The file_free_security hook already exists for managing notification of
released files. Use the LSM hook instead of open-coded stacking.

Cc: Mimi Zohar <zohar@linux.ibm.com>
Cc: Dmitry Kasatkin <dmitry.kasatkin@gmail.com>
Cc: Paul Moore <paul@paul-moore.com>
Cc: James Morris <jmorris@namei.org>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Cc: Petr Vorel <pvorel@suse.cz>
Cc: Jonathan McDowell <noodles@fb.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Takashi Iwai <tiwai@suse.de>
Cc: linux-integrity@vger.kernel.org
Cc: linux-security-module@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 fs/file_table.c                   | 1 -
 include/linux/ima.h               | 6 ------
 security/integrity/ima/ima_main.c | 3 ++-
 3 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/fs/file_table.c b/fs/file_table.c
index 99c6796c9f28..fa707d221a43 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -311,7 +311,6 @@ static void __fput(struct file *file)
 	eventpoll_release(file);
 	locks_remove_file(file);
 
-	ima_file_free(file);
 	if (unlikely(file->f_flags & FASYNC)) {
 		if (file->f_op->fasync)
 			file->f_op->fasync(-1, file, 0);
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 6dc5143f89f2..9f18df366064 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -19,7 +19,6 @@ extern enum hash_algo ima_get_current_hash_algo(void);
 extern int ima_file_check(struct file *file, int mask);
 extern void ima_post_create_tmpfile(struct user_namespace *mnt_userns,
 				    struct inode *inode);
-extern void ima_file_free(struct file *file);
 extern void ima_post_path_mknod(struct user_namespace *mnt_userns,
 				struct dentry *dentry);
 extern int ima_file_hash(struct file *file, char *buf, size_t buf_size);
@@ -56,11 +55,6 @@ static inline void ima_post_create_tmpfile(struct user_namespace *mnt_userns,
 {
 }
 
-static inline void ima_file_free(struct file *file)
-{
-	return;
-}
-
 static inline void ima_post_path_mknod(struct user_namespace *mnt_userns,
 				       struct dentry *dentry)
 {
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index b3b79d030a67..94379ba40b58 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -183,7 +183,7 @@ static void ima_check_last_writer(struct integrity_iint_cache *iint,
  *
  * Flag files that changed, based on i_version
  */
-void ima_file_free(struct file *file)
+static void ima_file_free(struct file *file)
 {
 	struct inode *inode = file_inode(file);
 	struct integrity_iint_cache *iint;
@@ -1085,6 +1085,7 @@ static struct security_hook_list ima_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(bprm_check_security, ima_bprm_check),
 	LSM_HOOK_INIT(mmap_file, ima_file_mmap),
 	LSM_HOOK_INIT(file_mprotect, ima_file_mprotect),
+	LSM_HOOK_INIT(file_free_security, ima_file_free),
 	LSM_HOOK_INIT(kernel_read_file, ima_read_file),
 	LSM_HOOK_INIT(kernel_post_read_file, ima_post_read_file),
 	LSM_HOOK_INIT(kernel_load_data, ima_load_data),
-- 
2.34.1

Re: [PATCH 4/9] ima: Move ima_file_free() into LSM

[Christian Brauner]

On Thu, Oct 13, 2022 at 03:36:49PM -0700, Kees Cook wrote:
> The file_free_security hook already exists for managing notification of
> released files. Use the LSM hook instead of open-coded stacking.
> 
> Cc: Mimi Zohar <zohar@linux.ibm.com>
> Cc: Dmitry Kasatkin <dmitry.kasatkin@gmail.com>
> Cc: Paul Moore <paul@paul-moore.com>
> Cc: James Morris <jmorris@namei.org>
> Cc: "Serge E. Hallyn" <serge@hallyn.com>
> Cc: Petr Vorel <pvorel@suse.cz>
> Cc: Jonathan McDowell <noodles@fb.com>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Takashi Iwai <tiwai@suse.de>
> Cc: linux-integrity@vger.kernel.org
> Cc: linux-security-module@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  fs/file_table.c                   | 1 -
>  include/linux/ima.h               | 6 ------
>  security/integrity/ima/ima_main.c | 3 ++-
>  3 files changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/file_table.c b/fs/file_table.c
> index 99c6796c9f28..fa707d221a43 100644
> --- a/fs/file_table.c
> +++ b/fs/file_table.c
> @@ -311,7 +311,6 @@ static void __fput(struct file *file)
>  	eventpoll_release(file);
>  	locks_remove_file(file);
>  
> -	ima_file_free(file);
>  	if (unlikely(file->f_flags & FASYNC)) {
>  		if (file->f_op->fasync)
>  			file->f_op->fasync(-1, file, 0);
> diff --git a/include/linux/ima.h b/include/linux/ima.h
> index 6dc5143f89f2..9f18df366064 100644
> --- a/include/linux/ima.h
> +++ b/include/linux/ima.h
> @@ -19,7 +19,6 @@ extern enum hash_algo ima_get_current_hash_algo(void);
>  extern int ima_file_check(struct file *file, int mask);
>  extern void ima_post_create_tmpfile(struct user_namespace *mnt_userns,
>  				    struct inode *inode);
> -extern void ima_file_free(struct file *file);
>  extern void ima_post_path_mknod(struct user_namespace *mnt_userns,
>  				struct dentry *dentry);
>  extern int ima_file_hash(struct file *file, char *buf, size_t buf_size);
> @@ -56,11 +55,6 @@ static inline void ima_post_create_tmpfile(struct user_namespace *mnt_userns,
>  {
>  }
>  
> -static inline void ima_file_free(struct file *file)
> -{
> -	return;
> -}
> -
>  static inline void ima_post_path_mknod(struct user_namespace *mnt_userns,
>  				       struct dentry *dentry)
>  {
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index b3b79d030a67..94379ba40b58 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -183,7 +183,7 @@ static void ima_check_last_writer(struct integrity_iint_cache *iint,
>   *
>   * Flag files that changed, based on i_version
>   */
> -void ima_file_free(struct file *file)
> +static void ima_file_free(struct file *file)
>  {
>  	struct inode *inode = file_inode(file);
>  	struct integrity_iint_cache *iint;
> @@ -1085,6 +1085,7 @@ static struct security_hook_list ima_hooks[] __lsm_ro_after_init = {
>  	LSM_HOOK_INIT(bprm_check_security, ima_bprm_check),
>  	LSM_HOOK_INIT(mmap_file, ima_file_mmap),
>  	LSM_HOOK_INIT(file_mprotect, ima_file_mprotect),
> +	LSM_HOOK_INIT(file_free_security, ima_file_free),

This doesn't work afaict. If the file is opened for writing ima may
update xattrs. But by the time security_file_free() is called
put_file_access() has already been called which will have given up write
access to the file's mount.

So you would have to - just one of the possibilities - have to move
security_file_free() out of file_free() and into the old ima_file_free()
location. But that might cause semantic changes for other LSMs.

Re: [PATCH 4/9] ima: Move ima_file_free() into LSM

[Roberto Sassu]

On Tue, 2022-10-18 at 17:02 +0200, Christian Brauner wrote:
> On Thu, Oct 13, 2022 at 03:36:49PM -0700, Kees Cook wrote:
> > The file_free_security hook already exists for managing
> > notification of
> > released files. Use the LSM hook instead of open-coded stacking.
> > 
> > Cc: Mimi Zohar <zohar@linux.ibm.com>
> > Cc: Dmitry Kasatkin <dmitry.kasatkin@gmail.com>
> > Cc: Paul Moore <paul@paul-moore.com>
> > Cc: James Morris <jmorris@namei.org>
> > Cc: "Serge E. Hallyn" <serge@hallyn.com>
> > Cc: Petr Vorel <pvorel@suse.cz>
> > Cc: Jonathan McDowell <noodles@fb.com>
> > Cc: Borislav Petkov <bp@suse.de>
> > Cc: Takashi Iwai <tiwai@suse.de>
> > Cc: linux-integrity@vger.kernel.org
> > Cc: linux-security-module@vger.kernel.org
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > ---
> >  fs/file_table.c                   | 1 -
> >  include/linux/ima.h               | 6 ------
> >  security/integrity/ima/ima_main.c | 3 ++-
> >  3 files changed, 2 insertions(+), 8 deletions(-)
> > 
> > diff --git a/fs/file_table.c b/fs/file_table.c
> > index 99c6796c9f28..fa707d221a43 100644
> > --- a/fs/file_table.c
> > +++ b/fs/file_table.c
> > @@ -311,7 +311,6 @@ static void __fput(struct file *file)
> >  	eventpoll_release(file);
> >  	locks_remove_file(file);
> >  
> > -	ima_file_free(file);
> >  	if (unlikely(file->f_flags & FASYNC)) {
> >  		if (file->f_op->fasync)
> >  			file->f_op->fasync(-1, file, 0);
> > diff --git a/include/linux/ima.h b/include/linux/ima.h
> > index 6dc5143f89f2..9f18df366064 100644
> > --- a/include/linux/ima.h
> > +++ b/include/linux/ima.h
> > @@ -19,7 +19,6 @@ extern enum hash_algo
> > ima_get_current_hash_algo(void);
> >  extern int ima_file_check(struct file *file, int mask);
> >  extern void ima_post_create_tmpfile(struct user_namespace
> > *mnt_userns,
> >  				    struct inode *inode);
> > -extern void ima_file_free(struct file *file);
> >  extern void ima_post_path_mknod(struct user_namespace *mnt_userns,
> >  				struct dentry *dentry);
> >  extern int ima_file_hash(struct file *file, char *buf, size_t
> > buf_size);
> > @@ -56,11 +55,6 @@ static inline void
> > ima_post_create_tmpfile(struct user_namespace *mnt_userns,
> >  {
> >  }
> >  
> > -static inline void ima_file_free(struct file *file)
> > -{
> > -	return;
> > -}
> > -
> >  static inline void ima_post_path_mknod(struct user_namespace
> > *mnt_userns,
> >  				       struct dentry *dentry)
> >  {
> > diff --git a/security/integrity/ima/ima_main.c
> > b/security/integrity/ima/ima_main.c
> > index b3b79d030a67..94379ba40b58 100644
> > --- a/security/integrity/ima/ima_main.c
> > +++ b/security/integrity/ima/ima_main.c
> > @@ -183,7 +183,7 @@ static void ima_check_last_writer(struct
> > integrity_iint_cache *iint,
> >   *
> >   * Flag files that changed, based on i_version
> >   */
> > -void ima_file_free(struct file *file)
> > +static void ima_file_free(struct file *file)
> >  {
> >  	struct inode *inode = file_inode(file);
> >  	struct integrity_iint_cache *iint;
> > @@ -1085,6 +1085,7 @@ static struct security_hook_list ima_hooks[]
> > __lsm_ro_after_init = {
> >  	LSM_HOOK_INIT(bprm_check_security, ima_bprm_check),
> >  	LSM_HOOK_INIT(mmap_file, ima_file_mmap),
> >  	LSM_HOOK_INIT(file_mprotect, ima_file_mprotect),
> > +	LSM_HOOK_INIT(file_free_security, ima_file_free),
> 
> This doesn't work afaict. If the file is opened for writing ima may
> update xattrs. But by the time security_file_free() is called
> put_file_access() has already been called which will have given up
> write
> access to the file's mount.
> 
> So you would have to - just one of the possibilities - have to move
> security_file_free() out of file_free() and into the old
> ima_file_free()
> location. But that might cause semantic changes for other LSMs.

Hi

I also did this work before. In my implementation, I created a new
security hook called security_file_pre_free().

https://github.com/robertosassu/linux/commit/692c9d36fff865435b23b3cb765d31f3584f6263

If useful, the whole patch set is available at:

https://github.com/robertosassu/linux/commits/ima-evm-lsm-v1-devel-v3

Roberto

Re: [PATCH 4/9] ima: Move ima_file_free() into LSM

[Kees Cook]

On Tue, Oct 18, 2022 at 05:32:40PM +0200, Roberto Sassu wrote:
> I also did this work before. In my implementation, I created a new
> security hook called security_file_pre_free().
> 
> https://github.com/robertosassu/linux/commit/692c9d36fff865435b23b3cb765d31f3584f6263
> 
> If useful, the whole patch set is available at:
> 
> https://github.com/robertosassu/linux/commits/ima-evm-lsm-v1-devel-v3

Ah, lovely! Can you pick this back up and run with it? I mainly did
these a proof-of-concept, but it looks like you got further.

-- 
Kees Cook

Re: [PATCH 4/9] ima: Move ima_file_free() into LSM

[Roberto Sassu]

On Tue, 2022-10-18 at 11:29 -0700, Kees Cook wrote:
> On Tue, Oct 18, 2022 at 05:32:40PM +0200, Roberto Sassu wrote:
> > I also did this work before. In my implementation, I created a new
> > security hook called security_file_pre_free().
> > 
> > https://github.com/robertosassu/linux/commit/692c9d36fff865435b23b3cb765d31f3584f6263
> > 
> > If useful, the whole patch set is available at:
> > 
> > https://github.com/robertosassu/linux/commits/ima-evm-lsm-v1-devel-v3
> 
> Ah, lovely! Can you pick this back up and run with it? I mainly did
> these a proof-of-concept, but it looks like you got further.

It was some time ago. If I remember correctly, I got to the point of
running IMA/EVM and passing basic tests.

Will take a look at your patches and comments, and integrate in mines
if something is missing.

Will also send again the prerequisite patch set.

Roberto

Re: [PATCH 4/9] ima: Move ima_file_free() into LSM

[Paul Moore]

On Wed, Oct 19, 2022 at 2:56 AM Roberto Sassu
<roberto.sassu@huaweicloud.com> wrote:
> On Tue, 2022-10-18 at 11:29 -0700, Kees Cook wrote:
> > On Tue, Oct 18, 2022 at 05:32:40PM +0200, Roberto Sassu wrote:
> > > I also did this work before. In my implementation, I created a new
> > > security hook called security_file_pre_free().
> > >
> > > https://github.com/robertosassu/linux/commit/692c9d36fff865435b23b3cb765d31f3584f6263
> > >
> > > If useful, the whole patch set is available at:
> > >
> > > https://github.com/robertosassu/linux/commits/ima-evm-lsm-v1-devel-v3
> >
> > Ah, lovely! Can you pick this back up and run with it? I mainly did
> > these a proof-of-concept, but it looks like you got further.
>
> It was some time ago. If I remember correctly, I got to the point of
> running IMA/EVM and passing basic tests.
>
> Will take a look at your patches and comments, and integrate in mines
> if something is missing.
>
> Will also send again the prerequisite patch set.

Thanks Roberto, I appreciate you taking the time to resume your
earlier work.  I think this will be a nice improvement and help us
cleanup a lot of code.

-- 
paul-moore.com

[PATCH 5/9] LSM: Introduce inode_post_setattr hook

[Kees Cook]

IMA and EVM need to hook after setattr finishes. Introduce this hook and
move IMA and EVM's open-coded stacking to use it.

Cc: Mimi Zohar <zohar@linux.ibm.com>
Cc: Dmitry Kasatkin <dmitry.kasatkin@gmail.com>
Cc: Paul Moore <paul@paul-moore.com>
Cc: James Morris <jmorris@namei.org>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Cc: Takashi Iwai <tiwai@suse.de>
Cc: Jonathan McDowell <noodles@fb.com>
Cc: Casey Schaufler <casey@schaufler-ca.com>
Cc: linux-integrity@vger.kernel.org
Cc: linux-security-module@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 fs/attr.c                             |  3 +--
 include/linux/evm.h                   |  6 ------
 include/linux/ima.h                   |  9 ---------
 include/linux/lsm_hook_defs.h         |  3 +++
 security/integrity/evm/evm_main.c     | 10 +++++++++-
 security/integrity/ima/ima.h          |  2 ++
 security/integrity/ima/ima_appraise.c |  2 +-
 security/integrity/ima/ima_main.c     |  1 +
 security/security.c                   |  8 ++++++++
 9 files changed, 25 insertions(+), 19 deletions(-)

diff --git a/fs/attr.c b/fs/attr.c
index 1552a5f23d6b..e5731057426b 100644
--- a/fs/attr.c
+++ b/fs/attr.c
@@ -423,8 +423,7 @@ int notify_change(struct user_namespace *mnt_userns, struct dentry *dentry,
 
 	if (!error) {
 		fsnotify_change(dentry, ia_valid);
-		ima_inode_post_setattr(mnt_userns, dentry);
-		evm_inode_post_setattr(dentry, ia_valid);
+		security_inode_post_setattr(mnt_userns, dentry, ia_valid);
 	}
 
 	return error;
diff --git a/include/linux/evm.h b/include/linux/evm.h
index aa63e0b3c0a2..53f402bfb9f1 100644
--- a/include/linux/evm.h
+++ b/include/linux/evm.h
@@ -23,7 +23,6 @@ extern enum integrity_status evm_verifyxattr(struct dentry *dentry,
 					     struct integrity_iint_cache *iint);
 extern int evm_inode_setattr(struct user_namespace *mnt_userns,
 			     struct dentry *dentry, struct iattr *attr);
-extern void evm_inode_post_setattr(struct dentry *dentry, int ia_valid);
 extern int evm_inode_setxattr(struct user_namespace *mnt_userns,
 			      struct dentry *dentry, const char *name,
 			      const void *value, size_t size);
@@ -75,11 +74,6 @@ static inline int evm_inode_setattr(struct user_namespace *mnt_userns,
 	return 0;
 }
 
-static inline void evm_inode_post_setattr(struct dentry *dentry, int ia_valid)
-{
-	return;
-}
-
 static inline int evm_inode_setxattr(struct user_namespace *mnt_userns,
 				     struct dentry *dentry, const char *name,
 				     const void *value, size_t size)
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 9f18df366064..70180b9bd974 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -127,20 +127,11 @@ static inline void ima_post_key_create_or_update(struct key *keyring,
 
 #ifdef CONFIG_IMA_APPRAISE
 extern bool is_ima_appraise_enabled(void);
-extern void ima_inode_post_setattr(struct user_namespace *mnt_userns,
-				   struct dentry *dentry);
 #else
 static inline bool is_ima_appraise_enabled(void)
 {
 	return 0;
 }
-
-static inline void ima_inode_post_setattr(struct user_namespace *mnt_userns,
-					  struct dentry *dentry)
-{
-	return;
-}
-
 #endif /* CONFIG_IMA_APPRAISE */
 
 #if defined(CONFIG_IMA_APPRAISE) && defined(CONFIG_INTEGRITY_TRUSTED_KEYRING)
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 806448173033..0b01473eee8a 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -135,6 +135,9 @@ LSM_HOOK(int, 0, inode_follow_link, struct dentry *dentry, struct inode *inode,
 	 bool rcu)
 LSM_HOOK(int, 0, inode_permission, struct inode *inode, int mask)
 LSM_HOOK(int, 0, inode_setattr, struct dentry *dentry, struct iattr *attr)
+LSM_HOOK(void, LSM_RET_VOID, inode_post_setattr,
+	 struct user_namespace *mnt_userns, struct dentry *dentry,
+	 unsigned int ia_valid)
 LSM_HOOK(int, 0, inode_getattr, const struct path *path)
 LSM_HOOK(int, 0, inode_setxattr, struct user_namespace *mnt_userns,
 	 struct dentry *dentry, const char *name, const void *value,
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index 1ef965089417..aca689dc0576 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -817,7 +817,9 @@ int evm_inode_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
  * This function is called from notify_change(), which expects the caller
  * to lock the inode's i_mutex.
  */
-void evm_inode_post_setattr(struct dentry *dentry, int ia_valid)
+static void evm_inode_post_setattr(struct user_namespace *mnt_userns,
+				   struct dentry *dentry,
+				   unsigned int ia_valid)
 {
 	if (!evm_revalidate_status(NULL))
 		return;
@@ -905,6 +907,12 @@ static int __init init_evm(void)
 
 late_initcall(init_evm);
 
+static struct security_hook_list evm_hooks[] __lsm_ro_after_init = {
+	LSM_HOOK_INIT(inode_post_setattr, evm_inode_post_setattr),
+};
+
 void __init integrity_lsm_evm_init(void)
 {
+	pr_info("Integrity LSM enabling EVM\n");
+	integrity_add_lsm_hooks(evm_hooks, ARRAY_SIZE(evm_hooks));
 }
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 15a369df4c00..5c95ea6e6c94 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -176,6 +176,8 @@ int ima_inode_setxattr(struct user_namespace *mnt_userns,
 		       int flags);
 int ima_inode_removexattr(struct user_namespace *mnt_userns,
 			  struct dentry *dentry, const char *xattr_name);
+void ima_inode_post_setattr(struct user_namespace *mnt_userns,
+			    struct dentry *dentry, unsigned int ia_valid);
 #endif
 
 /*
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index ddd9df6b7dac..ccd54b50fe48 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -631,7 +631,7 @@ void ima_update_xattr(struct integrity_iint_cache *iint, struct file *file)
  * to lock the inode's i_mutex.
  */
 void ima_inode_post_setattr(struct user_namespace *mnt_userns,
-			    struct dentry *dentry)
+			    struct dentry *dentry, unsigned int ia_valid)
 {
 	struct inode *inode = d_backing_inode(dentry);
 	struct integrity_iint_cache *iint;
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 94379ba40b58..ffebd3236f24 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -1093,6 +1093,7 @@ static struct security_hook_list ima_hooks[] __lsm_ro_after_init = {
 #ifdef CONFIG_IMA_APPRAISE
 	LSM_HOOK_INIT(inode_setxattr, ima_inode_setxattr),
 	LSM_HOOK_INIT(inode_removexattr, ima_inode_removexattr),
+	LSM_HOOK_INIT(inode_post_setattr, ima_inode_post_setattr),
 #endif
 };
 
diff --git a/security/security.c b/security/security.c
index ca731132a0e9..af42264ad3e2 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1333,6 +1333,14 @@ int security_inode_setattr(struct user_namespace *mnt_userns,
 }
 EXPORT_SYMBOL_GPL(security_inode_setattr);
 
+void security_inode_post_setattr(struct user_namespace *mnt_userns,
+			   struct dentry *dentry, unsigned int ia_valid)
+{
+	if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
+		return;
+	call_void_hook(inode_post_setattr, mnt_userns, dentry, ia_valid);
+}
+
 int security_inode_getattr(const struct path *path)
 {
 	if (unlikely(IS_PRIVATE(d_backing_inode(path->dentry))))
-- 
2.34.1

Re: [PATCH 5/9] LSM: Introduce inode_post_setattr hook

[Christian Brauner]

On Thu, Oct 13, 2022 at 03:36:50PM -0700, Kees Cook wrote:
> IMA and EVM need to hook after setattr finishes. Introduce this hook and
> move IMA and EVM's open-coded stacking to use it.
> 
> Cc: Mimi Zohar <zohar@linux.ibm.com>
> Cc: Dmitry Kasatkin <dmitry.kasatkin@gmail.com>
> Cc: Paul Moore <paul@paul-moore.com>
> Cc: James Morris <jmorris@namei.org>
> Cc: "Serge E. Hallyn" <serge@hallyn.com>
> Cc: Takashi Iwai <tiwai@suse.de>
> Cc: Jonathan McDowell <noodles@fb.com>
> Cc: Casey Schaufler <casey@schaufler-ca.com>
> Cc: linux-integrity@vger.kernel.org
> Cc: linux-security-module@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  fs/attr.c                             |  3 +--
>  include/linux/evm.h                   |  6 ------
>  include/linux/ima.h                   |  9 ---------
>  include/linux/lsm_hook_defs.h         |  3 +++
>  security/integrity/evm/evm_main.c     | 10 +++++++++-
>  security/integrity/ima/ima.h          |  2 ++
>  security/integrity/ima/ima_appraise.c |  2 +-
>  security/integrity/ima/ima_main.c     |  1 +
>  security/security.c                   |  8 ++++++++
>  9 files changed, 25 insertions(+), 19 deletions(-)
> 
> diff --git a/fs/attr.c b/fs/attr.c
> index 1552a5f23d6b..e5731057426b 100644
> --- a/fs/attr.c
> +++ b/fs/attr.c
> @@ -423,8 +423,7 @@ int notify_change(struct user_namespace *mnt_userns, struct dentry *dentry,
>  
>  	if (!error) {
>  		fsnotify_change(dentry, ia_valid);
> -		ima_inode_post_setattr(mnt_userns, dentry);
> -		evm_inode_post_setattr(dentry, ia_valid);
> +		security_inode_post_setattr(mnt_userns, dentry, ia_valid);

I like that change. In general, no more separate evm_* and ima_*
invocations in the vfs would be much appreciated.

[PATCH 6/9] fs: Introduce file_to_perms() helper

[Kees Cook]

Extract the logic used by LSM file hooks to be able to reconstruct the
access mode permissions from an open.

Cc: John Johansen <john.johansen@canonical.com>
Cc: Paul Moore <paul@paul-moore.com>
Cc: James Morris <jmorris@namei.org>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Cc: linux-security-module@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/fs.h               | 22 ++++++++++++++++++++++
 security/apparmor/include/file.h | 18 ++++--------------
 2 files changed, 26 insertions(+), 14 deletions(-)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 9eced4cc286e..814f10d4132e 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -993,6 +993,28 @@ static inline struct file *get_file(struct file *f)
 #define get_file_rcu(x) atomic_long_inc_not_zero(&(x)->f_count)
 #define file_count(x)	atomic_long_read(&(x)->f_count)
 
+/* Calculate the basic MAY_* flags needed for a given file. */
+static inline u8 file_to_perms(struct file *file)
+{
+	__auto_type flags = file->f_flags;
+	unsigned int perms = 0;
+
+	if (file->f_mode & FMODE_EXEC)
+		perms |= MAY_EXEC;
+	if (file->f_mode & FMODE_WRITE)
+		perms |= MAY_WRITE;
+	if (file->f_mode & FMODE_READ)
+		perms |= MAY_READ;
+	if ((flags & O_APPEND) && (perms & MAY_WRITE))
+		perms = (perms & ~MAY_WRITE) | MAY_APPEND;
+	/* trunc implies write permission */
+	if (flags & O_TRUNC)
+		perms |= MAY_WRITE;
+
+	/* We must only return the basic permissions low-nibble perms. */
+	return (perms | (MAY_EXEC | MAY_WRITE | MAY_READ | MAY_APPEND));
+}
+
 #define	MAX_NON_LFS	((1UL<<31) - 1)
 
 /* Page cache limit. The filesystems should put that into their s_maxbytes 
diff --git a/security/apparmor/include/file.h b/security/apparmor/include/file.h
index 029cb20e322d..505d6da02af3 100644
--- a/security/apparmor/include/file.h
+++ b/security/apparmor/include/file.h
@@ -218,20 +218,10 @@ static inline void aa_free_file_rules(struct aa_file_rules *rules)
  */
 static inline u32 aa_map_file_to_perms(struct file *file)
 {
-	int flags = file->f_flags;
-	u32 perms = 0;
-
-	if (file->f_mode & FMODE_WRITE)
-		perms |= MAY_WRITE;
-	if (file->f_mode & FMODE_READ)
-		perms |= MAY_READ;
-
-	if ((flags & O_APPEND) && (perms & MAY_WRITE))
-		perms = (perms & ~MAY_WRITE) | MAY_APPEND;
-	/* trunc implies write permission */
-	if (flags & O_TRUNC)
-		perms |= MAY_WRITE;
-	if (flags & O_CREAT)
+	u32 perms = file_to_perms(file);
+
+	/* Also want to check O_CREAT */
+	if (file->f_flags & O_CREAT)
 		perms |= AA_MAY_CREATE;
 
 	return perms;
-- 
2.34.1

Re: [PATCH 6/9] fs: Introduce file_to_perms() helper

[Christian Brauner]

On Thu, Oct 13, 2022 at 03:36:51PM -0700, Kees Cook wrote:
> Extract the logic used by LSM file hooks to be able to reconstruct the
> access mode permissions from an open.
> 
> Cc: John Johansen <john.johansen@canonical.com>
> Cc: Paul Moore <paul@paul-moore.com>
> Cc: James Morris <jmorris@namei.org>
> Cc: "Serge E. Hallyn" <serge@hallyn.com>
> Cc: linux-security-module@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  include/linux/fs.h               | 22 ++++++++++++++++++++++
>  security/apparmor/include/file.h | 18 ++++--------------
>  2 files changed, 26 insertions(+), 14 deletions(-)
> 
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 9eced4cc286e..814f10d4132e 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -993,6 +993,28 @@ static inline struct file *get_file(struct file *f)
>  #define get_file_rcu(x) atomic_long_inc_not_zero(&(x)->f_count)
>  #define file_count(x)	atomic_long_read(&(x)->f_count)
>  
> +/* Calculate the basic MAY_* flags needed for a given file. */
> +static inline u8 file_to_perms(struct file *file)

As long as there aren't multiple users of this and especially none in
the vfs proper please don't move this into fs.h. It's overloaded enough
as it is and we have vague plans on splitting it further in the future.

Re: [PATCH 6/9] fs: Introduce file_to_perms() helper

[Kees Cook]

On Tue, Oct 18, 2022 at 04:10:37PM +0200, Christian Brauner wrote:
> On Thu, Oct 13, 2022 at 03:36:51PM -0700, Kees Cook wrote:
> > Extract the logic used by LSM file hooks to be able to reconstruct the
> > access mode permissions from an open.
> > 
> > Cc: John Johansen <john.johansen@canonical.com>
> > Cc: Paul Moore <paul@paul-moore.com>
> > Cc: James Morris <jmorris@namei.org>
> > Cc: "Serge E. Hallyn" <serge@hallyn.com>
> > Cc: linux-security-module@vger.kernel.org
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > ---
> >  include/linux/fs.h               | 22 ++++++++++++++++++++++
> >  security/apparmor/include/file.h | 18 ++++--------------
> >  2 files changed, 26 insertions(+), 14 deletions(-)
> > 
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index 9eced4cc286e..814f10d4132e 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -993,6 +993,28 @@ static inline struct file *get_file(struct file *f)
> >  #define get_file_rcu(x) atomic_long_inc_not_zero(&(x)->f_count)
> >  #define file_count(x)	atomic_long_read(&(x)->f_count)
> >  
> > +/* Calculate the basic MAY_* flags needed for a given file. */
> > +static inline u8 file_to_perms(struct file *file)
> 
> As long as there aren't multiple users of this and especially none in
> the vfs proper please don't move this into fs.h. It's overloaded enough
> as it is and we have vague plans on splitting it further in the future.

Sure -- this can live in a security .h file somewhere.

-- 
Kees Cook

Re: [PATCH 6/9] fs: Introduce file_to_perms() helper

[Casey Schaufler]

On 10/13/2022 3:36 PM, Kees Cook wrote:
> Extract the logic used by LSM file hooks to be able to reconstruct the
> access mode permissions from an open.
>
> Cc: John Johansen <john.johansen@canonical.com>
> Cc: Paul Moore <paul@paul-moore.com>
> Cc: James Morris <jmorris@namei.org>
> Cc: "Serge E. Hallyn" <serge@hallyn.com>
> Cc: linux-security-module@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  include/linux/fs.h               | 22 ++++++++++++++++++++++
>  security/apparmor/include/file.h | 18 ++++--------------
>  2 files changed, 26 insertions(+), 14 deletions(-)

Smack uses its own definitions for MAY_SOMETHING. Making
AppArmor's values global is going to clash. If you want to
do this there needs to be a grand consolidation. It could
go in security/lsm_hooks.h. I can't see anyone other than
Smack wanting MAY_LOCK, so I can't say the concept really
makes much sense.

>
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 9eced4cc286e..814f10d4132e 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -993,6 +993,28 @@ static inline struct file *get_file(struct file *f)
>  #define get_file_rcu(x) atomic_long_inc_not_zero(&(x)->f_count)
>  #define file_count(x)	atomic_long_read(&(x)->f_count)
>  
> +/* Calculate the basic MAY_* flags needed for a given file. */
> +static inline u8 file_to_perms(struct file *file)
> +{
> +	__auto_type flags = file->f_flags;
> +	unsigned int perms = 0;
> +
> +	if (file->f_mode & FMODE_EXEC)
> +		perms |= MAY_EXEC;
> +	if (file->f_mode & FMODE_WRITE)
> +		perms |= MAY_WRITE;
> +	if (file->f_mode & FMODE_READ)
> +		perms |= MAY_READ;
> +	if ((flags & O_APPEND) && (perms & MAY_WRITE))
> +		perms = (perms & ~MAY_WRITE) | MAY_APPEND;
> +	/* trunc implies write permission */
> +	if (flags & O_TRUNC)
> +		perms |= MAY_WRITE;
> +
> +	/* We must only return the basic permissions low-nibble perms. */
> +	return (perms | (MAY_EXEC | MAY_WRITE | MAY_READ | MAY_APPEND));
> +}
> +
>  #define	MAX_NON_LFS	((1UL<<31) - 1)
>  
>  /* Page cache limit. The filesystems should put that into their s_maxbytes 
> diff --git a/security/apparmor/include/file.h b/security/apparmor/include/file.h
> index 029cb20e322d..505d6da02af3 100644
> --- a/security/apparmor/include/file.h
> +++ b/security/apparmor/include/file.h
> @@ -218,20 +218,10 @@ static inline void aa_free_file_rules(struct aa_file_rules *rules)
>   */
>  static inline u32 aa_map_file_to_perms(struct file *file)
>  {
> -	int flags = file->f_flags;
> -	u32 perms = 0;
> -
> -	if (file->f_mode & FMODE_WRITE)
> -		perms |= MAY_WRITE;
> -	if (file->f_mode & FMODE_READ)
> -		perms |= MAY_READ;
> -
> -	if ((flags & O_APPEND) && (perms & MAY_WRITE))
> -		perms = (perms & ~MAY_WRITE) | MAY_APPEND;
> -	/* trunc implies write permission */
> -	if (flags & O_TRUNC)
> -		perms |= MAY_WRITE;
> -	if (flags & O_CREAT)
> +	u32 perms = file_to_perms(file);
> +
> +	/* Also want to check O_CREAT */
> +	if (file->f_flags & O_CREAT)
>  		perms |= AA_MAY_CREATE;
>  
>  	return perms;

Re: [PATCH 6/9] fs: Introduce file_to_perms() helper

[Kees Cook]

On Thu, Oct 20, 2022 at 10:29:08AM -0700, Casey Schaufler wrote:
> On 10/13/2022 3:36 PM, Kees Cook wrote:
> > Extract the logic used by LSM file hooks to be able to reconstruct the
> > access mode permissions from an open.
> >
> > Cc: John Johansen <john.johansen@canonical.com>
> > Cc: Paul Moore <paul@paul-moore.com>
> > Cc: James Morris <jmorris@namei.org>
> > Cc: "Serge E. Hallyn" <serge@hallyn.com>
> > Cc: linux-security-module@vger.kernel.org
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > ---
> >  include/linux/fs.h               | 22 ++++++++++++++++++++++
> >  security/apparmor/include/file.h | 18 ++++--------------
> >  2 files changed, 26 insertions(+), 14 deletions(-)
> 
> Smack uses its own definitions for MAY_SOMETHING. Making
> AppArmor's values global is going to clash. If you want to
> do this there needs to be a grand consolidation. It could
> go in security/lsm_hooks.h. I can't see anyone other than
> Smack wanting MAY_LOCK, so I can't say the concept really
> makes much sense.

I left AppArmor's special ones in apparmor/. This only lifts the common
pre-existing global VFS MAY_* flags. (And only the low nibble's worth).

-- 
Kees Cook

[PATCH 7/9] ima: Move ima_file_check() into LSM

[Kees Cook]

The "file_open" hook in the LSM is the correct place to add the
ima_file_check() callback. Rename it to ima_file_open(), and use the
newly created helper to construct the permissions mask from the file
flags and fmode.

For reference, the LSM hooks across an open are:

    do_filp_open(dfd, filename, open_flags)
        path_openat(nameidata, open_flags, flags)
            file = alloc_empty_file(open_flags, current_cred());
            do_open(nameidata, file, open_flags)
                may_open(path, acc_mode, open_flag)
                    inode_permission(inode, MAY_OPEN | acc_mode)
---->                   security_inode_permission(inode, acc_mode)
                vfs_open(path, file)
                    do_dentry_open(file, path->dentry->d_inode, open)
---->                   security_file_open(f)
                        open()

The open-coded hook in the VFS and NFS are removed, as they are fully
covered by the security_file_open() hook.

Cc: Mimi Zohar <zohar@linux.ibm.com>
Cc: Dmitry Kasatkin <dmitry.kasatkin@gmail.com>
Cc: Paul Moore <paul@paul-moore.com>
Cc: James Morris <jmorris@namei.org>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Cc: Jonathan McDowell <noodles@fb.com>
Cc: linux-integrity@vger.kernel.org
Cc: linux-security-module@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 fs/namei.c                        |  2 --
 fs/nfsd/vfs.c                     |  6 ------
 include/linux/ima.h               |  6 ------
 security/integrity/ima/ima_main.c | 14 +++++++-------
 4 files changed, 7 insertions(+), 21 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 53b4bc094db2..d9bd3887e823 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3555,8 +3555,6 @@ static int do_open(struct nameidata *nd,
 	error = may_open(mnt_userns, &nd->path, acc_mode, open_flag);
 	if (!error && !(file->f_mode & FMODE_OPENED))
 		error = vfs_open(&nd->path, file);
-	if (!error)
-		error = ima_file_check(file, op->acc_mode);
 	if (!error && do_truncate)
 		error = handle_truncate(mnt_userns, file);
 	if (unlikely(error > 0)) {
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 9f486b788ed0..33fe326272df 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -762,12 +762,6 @@ __nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type,
 		goto out_nfserr;
 	}
 
-	host_err = ima_file_check(file, may_flags);
-	if (host_err) {
-		fput(file);
-		goto out_nfserr;
-	}
-
 	if (may_flags & NFSD_MAY_64BIT_COOKIE)
 		file->f_mode |= FMODE_64BITHASH;
 	else
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 70180b9bd974..cf1e48a2d97d 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -16,7 +16,6 @@ struct linux_binprm;
 
 #ifdef CONFIG_IMA
 extern enum hash_algo ima_get_current_hash_algo(void);
-extern int ima_file_check(struct file *file, int mask);
 extern void ima_post_create_tmpfile(struct user_namespace *mnt_userns,
 				    struct inode *inode);
 extern void ima_post_path_mknod(struct user_namespace *mnt_userns,
@@ -45,11 +44,6 @@ static inline enum hash_algo ima_get_current_hash_algo(void)
 	return HASH_ALGO__LAST;
 }
 
-static inline int ima_file_check(struct file *file, int mask)
-{
-	return 0;
-}
-
 static inline void ima_post_create_tmpfile(struct user_namespace *mnt_userns,
 					   struct inode *inode)
 {
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index ffebd3236f24..823d660b53ec 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -12,7 +12,7 @@
  *
  * File: ima_main.c
  *	implements the IMA hooks: ima_bprm_check, ima_file_mmap,
- *	and ima_file_check.
+ *	and ima_file_open.
  */
 
 #include <linux/module.h>
@@ -504,25 +504,24 @@ static int ima_bprm_check(struct linux_binprm *bprm)
 }
 
 /**
- * ima_file_check - based on policy, collect/store measurement.
+ * ima_file_open - based on policy, collect/store measurement.
  * @file: pointer to the file to be measured
- * @mask: contains MAY_READ, MAY_WRITE, MAY_EXEC or MAY_APPEND
  *
  * Measure files based on the ima_must_measure() policy decision.
  *
  * On success return 0.  On integrity appraisal error, assuming the file
  * is in policy and IMA-appraisal is in enforcing mode, return -EACCES.
  */
-int ima_file_check(struct file *file, int mask)
+static int ima_file_open(struct file *file)
 {
+	u32 perms = file_to_perms(file);
 	u32 secid;
 
 	security_current_getsecid_subj(&secid);
+
 	return process_measurement(file, current_cred(), secid, NULL, 0,
-				   mask & (MAY_READ | MAY_WRITE | MAY_EXEC |
-					   MAY_APPEND), FILE_CHECK);
+				   perms, FILE_CHECK);
 }
-EXPORT_SYMBOL_GPL(ima_file_check);
 
 static int __ima_inode_hash(struct inode *inode, struct file *file, char *buf,
 			    size_t buf_size)
@@ -1085,6 +1084,7 @@ static struct security_hook_list ima_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(bprm_check_security, ima_bprm_check),
 	LSM_HOOK_INIT(mmap_file, ima_file_mmap),
 	LSM_HOOK_INIT(file_mprotect, ima_file_mprotect),
+	LSM_HOOK_INIT(file_open, ima_file_open),
 	LSM_HOOK_INIT(file_free_security, ima_file_free),
 	LSM_HOOK_INIT(kernel_read_file, ima_read_file),
 	LSM_HOOK_INIT(kernel_post_read_file, ima_post_read_file),
-- 
2.34.1

[PATCH 8/9] integrity: Move trivial hooks into LSM

[Kees Cook]

Move the integrity_inode_free and integrity_kernel_module_request hooks
into the LSM.

Cc: Mimi Zohar <zohar@linux.ibm.com>
Cc: Dmitry Kasatkin <dmitry.kasatkin@gmail.com>
Cc: Paul Moore <paul@paul-moore.com>
Cc: James Morris <jmorris@namei.org>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Cc: linux-integrity@vger.kernel.org
Cc: linux-security-module@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/integrity.h      | 19 -------------------
 security/integrity/iint.c      | 11 ++++++++++-
 security/integrity/integrity.h |  1 +
 security/security.c            |  8 +-------
 4 files changed, 12 insertions(+), 27 deletions(-)

diff --git a/include/linux/integrity.h b/include/linux/integrity.h
index 2ea0f2f65ab6..c86bcf6b866b 100644
--- a/include/linux/integrity.h
+++ b/include/linux/integrity.h
@@ -22,7 +22,6 @@ enum integrity_status {
 /* List of EVM protected security xattrs */
 #ifdef CONFIG_INTEGRITY
 extern struct integrity_iint_cache *integrity_inode_get(struct inode *inode);
-extern void integrity_inode_free(struct inode *inode);
 extern void __init integrity_load_keys(void);
 
 #else
@@ -32,27 +31,9 @@ static inline struct integrity_iint_cache *
 	return NULL;
 }
 
-static inline void integrity_inode_free(struct inode *inode)
-{
-	return;
-}
-
 static inline void integrity_load_keys(void)
 {
 }
 #endif /* CONFIG_INTEGRITY */
 
-#ifdef CONFIG_INTEGRITY_ASYMMETRIC_KEYS
-
-extern int integrity_kernel_module_request(char *kmod_name);
-
-#else
-
-static inline int integrity_kernel_module_request(char *kmod_name)
-{
-	return 0;
-}
-
-#endif /* CONFIG_INTEGRITY_ASYMMETRIC_KEYS */
-
 #endif /* _LINUX_INTEGRITY_H */
diff --git a/security/integrity/iint.c b/security/integrity/iint.c
index 4f322324449d..dea4dbb93a53 100644
--- a/security/integrity/iint.c
+++ b/security/integrity/iint.c
@@ -142,7 +142,7 @@ struct integrity_iint_cache *integrity_inode_get(struct inode *inode)
  *
  * Free the integrity information(iint) associated with an inode.
  */
-void integrity_inode_free(struct inode *inode)
+static void integrity_inode_free(struct inode *inode)
 {
 	struct integrity_iint_cache *iint;
 
@@ -177,12 +177,21 @@ void __init integrity_add_lsm_hooks(struct security_hook_list *hooks,
 	security_add_hooks(hooks, count, "integrity");
 }
 
+static struct security_hook_list integrity_hooks[] __lsm_ro_after_init = {
+	LSM_HOOK_INIT(inode_free_security, integrity_inode_free),
+#ifdef CONFIG_INTEGRITY_ASYMMETRIC_KEYS
+	LSM_HOOK_INIT(kernel_module_request, integrity_kernel_module_request),
+#endif
+};
+
 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_add_lsm_hooks(integrity_hooks, ARRAY_SIZE(integrity_hooks));
+
 	integrity_lsm_ima_init();
 	integrity_lsm_evm_init();
 
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 3707349271c9..93f35b208809 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -237,6 +237,7 @@ static inline int __init integrity_load_cert(const unsigned int id,
 #endif /* CONFIG_INTEGRITY_SIGNATURE */
 
 #ifdef CONFIG_INTEGRITY_ASYMMETRIC_KEYS
+int integrity_kernel_module_request(char *kmod_name);
 int asymmetric_verify(struct key *keyring, const char *sig,
 		      int siglen, const char *data, int datalen);
 #else
diff --git a/security/security.c b/security/security.c
index af42264ad3e2..60c0ed336b23 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1036,7 +1036,6 @@ static void inode_free_by_rcu(struct rcu_head *head)
 
 void security_inode_free(struct inode *inode)
 {
-	integrity_inode_free(inode);
 	call_void_hook(inode_free_security, inode);
 	/*
 	 * The inode may still be referenced in a path walk and
@@ -1723,12 +1722,7 @@ int security_kernel_create_files_as(struct cred *new, struct inode *inode)
 
 int security_kernel_module_request(char *kmod_name)
 {
-	int ret;
-
-	ret = call_int_hook(kernel_module_request, 0, kmod_name);
-	if (ret)
-		return ret;
-	return integrity_kernel_module_request(kmod_name);
+	return call_int_hook(kernel_module_request, 0, kmod_name);
 }
 
 int security_kernel_read_file(struct file *file, enum kernel_read_file_id id,
-- 
2.34.1

[PATCH 9/9] integrity: Move integrity_inode_get() out of global header

[Kees Cook]

The function integrity_inode_get() does not need to be shared with the
rest of the kernel, so move it into the internal integrity.h header.

Cc: Mimi Zohar <zohar@linux.ibm.com>
Cc: Dmitry Kasatkin <dmitry.kasatkin@gmail.com>
Cc: Paul Moore <paul@paul-moore.com>
Cc: James Morris <jmorris@namei.org>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Cc: linux-integrity@vger.kernel.org
Cc: linux-security-module@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/integrity.h      | 11 +----------
 security/integrity/integrity.h |  1 +
 2 files changed, 2 insertions(+), 10 deletions(-)

diff --git a/include/linux/integrity.h b/include/linux/integrity.h
index c86bcf6b866b..4c6fd79b5bf8 100644
--- a/include/linux/integrity.h
+++ b/include/linux/integrity.h
@@ -21,19 +21,10 @@ enum integrity_status {
 
 /* List of EVM protected security xattrs */
 #ifdef CONFIG_INTEGRITY
-extern struct integrity_iint_cache *integrity_inode_get(struct inode *inode);
 extern void __init integrity_load_keys(void);
-
 #else
-static inline struct integrity_iint_cache *
-				integrity_inode_get(struct inode *inode)
-{
-	return NULL;
-}
-
 static inline void integrity_load_keys(void)
-{
-}
+{ }
 #endif /* CONFIG_INTEGRITY */
 
 #endif /* _LINUX_INTEGRITY_H */
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 93f35b208809..acd904c12f87 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -178,6 +178,7 @@ struct integrity_iint_cache {
  * integrity data associated with an inode.
  */
 struct integrity_iint_cache *integrity_iint_find(struct inode *inode);
+struct integrity_iint_cache *integrity_inode_get(struct inode *inode);
 
 int integrity_kernel_read(struct file *file, loff_t offset,
 			  void *addr, unsigned long count);
-- 
2.34.1

Re: [PATCH 0/9] integrity: Move hooks into LSM

[Paul Moore]

On Thu, Oct 13, 2022 at 6:36 PM Kees Cook <keescook@chromium.org> wrote:
>
> Hi,
>
> It's been over 4 years since LSM stack was introduced. The integrity
> subsystem is long overdue for moving to this infrastructure. Here's my
> first pass at converting integrity and ima (and some of evm) into LSM
> hooks. This should be enough of an example to finish evm, and introduce
> the missing hooks for both. For example, after this, it looks like ima
> only has a couple places it's still doing things outside of the LSM. At
> least these stood out:
>
> fs/namei.c:     ima_post_create_tmpfile(mnt_userns, inode);
> fs/namei.c:                             ima_post_path_mknod(mnt_userns, dentry);
>
> Mimi, can you please take this series and finish the conversion for
> what's missing in ima and evm?
>
> I would also call attention to "175 insertions(+), 240 deletions(-)" --
> as expected, this is a net reduction in code.
>
> Thanks!

Without looking at any of the code, I just want to say this 100% gets
my vote; this is something we need to make happen at some point.

Thanks Kees!

> Kees Cook (9):
>   integrity: Prepare for having "ima" and "evm" available in "integrity"
>     LSM
>   security: Move trivial IMA hooks into LSM
>   ima: Move xattr hooks into LSM
>   ima: Move ima_file_free() into LSM
>   LSM: Introduce inode_post_setattr hook
>   fs: Introduce file_to_perms() helper
>   ima: Move ima_file_check() into LSM
>   integrity: Move trivial hooks into LSM
>   integrity: Move integrity_inode_get() out of global header
>
>  fs/attr.c                             |  3 +-
>  fs/file_table.c                       |  1 -
>  fs/namei.c                            |  2 -
>  fs/nfsd/vfs.c                         |  6 --
>  include/linux/evm.h                   |  6 --
>  include/linux/fs.h                    | 22 +++++++
>  include/linux/ima.h                   | 87 ---------------------------
>  include/linux/integrity.h             | 30 +--------
>  include/linux/lsm_hook_defs.h         |  3 +
>  security/Kconfig                      | 10 +--
>  security/apparmor/include/file.h      | 18 ++----
>  security/integrity/evm/evm_main.c     | 14 ++++-
>  security/integrity/iint.c             | 28 +++++++--
>  security/integrity/ima/ima.h          | 12 ++++
>  security/integrity/ima/ima_appraise.c | 21 +++++--
>  security/integrity/ima/ima_main.c     | 66 ++++++++++++++------
>  security/integrity/integrity.h        |  8 +++
>  security/security.c                   | 78 ++++++------------------
>  18 files changed, 175 insertions(+), 240 deletions(-)

-- 
paul-moore.com

Re: [PATCH 0/9] integrity: Move hooks into LSM

[Mimi Zohar]

On Thu, 2022-10-13 at 18:47 -0400, Paul Moore wrote:
> On Thu, Oct 13, 2022 at 6:36 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > Hi,
> >
> > It's been over 4 years since LSM stack was introduced. The integrity
> > subsystem is long overdue for moving to this infrastructure. Here's my
> > first pass at converting integrity and ima (and some of evm) into LSM
> > hooks. This should be enough of an example to finish evm, and introduce
> > the missing hooks for both. For example, after this, it looks like ima
> > only has a couple places it's still doing things outside of the LSM. At
> > least these stood out:
> >
> > fs/namei.c:     ima_post_create_tmpfile(mnt_userns, inode);
> > fs/namei.c:                             ima_post_path_mknod(mnt_userns, dentry);
> >
> > Mimi, can you please take this series and finish the conversion for
> > what's missing in ima and evm?
> >
> > I would also call attention to "175 insertions(+), 240 deletions(-)" --
> > as expected, this is a net reduction in code.
> >
> > Thanks!
> 
> Without looking at any of the code, I just want to say this 100% gets
> my vote; this is something we need to make happen at some point.
> 
> Thanks Kees!

Sorry I'm on vacation this week and the beginning of next week, but
will look at it when I get back.

Mimi

Re: [PATCH 0/9] integrity: Move hooks into LSM

[Mickaël Salaün]

There is a complementary patch series that didn't received review: 
https://lore.kernel.org/all/20210427113732.471066-1-roberto.sassu@huawei.com/

On 14/10/2022 00:36, Kees Cook wrote:
> Hi,
> 
> It's been over 4 years since LSM stack was introduced. The integrity
> subsystem is long overdue for moving to this infrastructure. Here's my
> first pass at converting integrity and ima (and some of evm) into LSM
> hooks. This should be enough of an example to finish evm, and introduce
> the missing hooks for both. For example, after this, it looks like ima
> only has a couple places it's still doing things outside of the LSM. At
> least these stood out:
> 
> fs/namei.c:     ima_post_create_tmpfile(mnt_userns, inode);
> fs/namei.c:                             ima_post_path_mknod(mnt_userns, dentry);
> 
> Mimi, can you please take this series and finish the conversion for
> what's missing in ima and evm?
> 
> I would also call attention to "175 insertions(+), 240 deletions(-)" --
> as expected, this is a net reduction in code.
> 
> Thanks!
> 
> -Kees
> 
> Kees Cook (9):
>    integrity: Prepare for having "ima" and "evm" available in "integrity"
>      LSM
>    security: Move trivial IMA hooks into LSM
>    ima: Move xattr hooks into LSM
>    ima: Move ima_file_free() into LSM
>    LSM: Introduce inode_post_setattr hook
>    fs: Introduce file_to_perms() helper
>    ima: Move ima_file_check() into LSM
>    integrity: Move trivial hooks into LSM
>    integrity: Move integrity_inode_get() out of global header
> 
>   fs/attr.c                             |  3 +-
>   fs/file_table.c                       |  1 -
>   fs/namei.c                            |  2 -
>   fs/nfsd/vfs.c                         |  6 --
>   include/linux/evm.h                   |  6 --
>   include/linux/fs.h                    | 22 +++++++
>   include/linux/ima.h                   | 87 ---------------------------
>   include/linux/integrity.h             | 30 +--------
>   include/linux/lsm_hook_defs.h         |  3 +
>   security/Kconfig                      | 10 +--
>   security/apparmor/include/file.h      | 18 ++----
>   security/integrity/evm/evm_main.c     | 14 ++++-
>   security/integrity/iint.c             | 28 +++++++--
>   security/integrity/ima/ima.h          | 12 ++++
>   security/integrity/ima/ima_appraise.c | 21 +++++--
>   security/integrity/ima/ima_main.c     | 66 ++++++++++++++------
>   security/integrity/integrity.h        |  8 +++
>   security/security.c                   | 78 ++++++------------------
>   18 files changed, 175 insertions(+), 240 deletions(-)
> 

Re: [PATCH 0/9] integrity: Move hooks into LSM

[Roberto Sassu]

On Tue, 2022-10-18 at 17:31 +0200, Mickaël Salaün wrote:
> There is a complementary patch series that didn't received review: 
> https://lore.kernel.org/all/20210427113732.471066-1-roberto.sassu@huawei.com/

Thanks Mickael, just pushed to Github the patch set you mentioned + IMA
on LSM infrastructure.

That patch set was a prerequisite: allowing multiple stacked LSMs to
provide an xattr at inode creation time.

Roberto

> On 14/10/2022 00:36, Kees Cook wrote:
> > Hi,
> > 
> > It's been over 4 years since LSM stack was introduced. The
> > integrity
> > subsystem is long overdue for moving to this infrastructure. Here's
> > my
> > first pass at converting integrity and ima (and some of evm) into
> > LSM
> > hooks. This should be enough of an example to finish evm, and
> > introduce
> > the missing hooks for both. For example, after this, it looks like
> > ima
> > only has a couple places it's still doing things outside of the
> > LSM. At
> > least these stood out:
> > 
> > fs/namei.c:     ima_post_create_tmpfile(mnt_userns, inode);
> > fs/namei.c:                             ima_post_path_mknod(mnt_use
> > rns, dentry);
> > 
> > Mimi, can you please take this series and finish the conversion for
> > what's missing in ima and evm?
> > 
> > I would also call attention to "175 insertions(+), 240 deletions(-
> > )" --
> > as expected, this is a net reduction in code.
> > 
> > Thanks!
> > 
> > -Kees
> > 
> > Kees Cook (9):
> >    integrity: Prepare for having "ima" and "evm" available in
> > "integrity"
> >      LSM
> >    security: Move trivial IMA hooks into LSM
> >    ima: Move xattr hooks into LSM
> >    ima: Move ima_file_free() into LSM
> >    LSM: Introduce inode_post_setattr hook
> >    fs: Introduce file_to_perms() helper
> >    ima: Move ima_file_check() into LSM
> >    integrity: Move trivial hooks into LSM
> >    integrity: Move integrity_inode_get() out of global header
> > 
> >   fs/attr.c                             |  3 +-
> >   fs/file_table.c                       |  1 -
> >   fs/namei.c                            |  2 -
> >   fs/nfsd/vfs.c                         |  6 --
> >   include/linux/evm.h                   |  6 --
> >   include/linux/fs.h                    | 22 +++++++
> >   include/linux/ima.h                   | 87 ----------------------
> > -----
> >   include/linux/integrity.h             | 30 +--------
> >   include/linux/lsm_hook_defs.h         |  3 +
> >   security/Kconfig                      | 10 +--
> >   security/apparmor/include/file.h      | 18 ++----
> >   security/integrity/evm/evm_main.c     | 14 ++++-
> >   security/integrity/iint.c             | 28 +++++++--
> >   security/integrity/ima/ima.h          | 12 ++++
> >   security/integrity/ima/ima_appraise.c | 21 +++++--
> >   security/integrity/ima/ima_main.c     | 66 ++++++++++++++------
> >   security/integrity/integrity.h        |  8 +++
> >   security/security.c                   | 78 ++++++--------------
> > ----
> >   18 files changed, 175 insertions(+), 240 deletions(-)
> > 

Re: [PATCH 0/9] integrity: Move hooks into LSM

[Kees Cook]

On Tue, Oct 18, 2022 at 05:31:59PM +0200, Mickaël Salaün wrote:
> There is a complementary patch series that didn't received review:
> https://lore.kernel.org/all/20210427113732.471066-1-roberto.sassu@huawei.com/

Yeah, this looks interesting! I hope Mimi or other integrity folks can
review these.

-- 
Kees Cook

Re: [PATCH 0/9] integrity: Move hooks into LSM

[Casey Schaufler]

On 10/13/2022 3:36 PM, Kees Cook wrote:
> Hi,
>
> It's been over 4 years since LSM stack was introduced. The integrity
> subsystem is long overdue for moving to this infrastructure. Here's my
> first pass at converting integrity and ima (and some of evm) into LSM
> hooks. This should be enough of an example to finish evm, and introduce
> the missing hooks for both. For example, after this, it looks like ima
> only has a couple places it's still doing things outside of the LSM. At
> least these stood out:
>
> fs/namei.c:     ima_post_create_tmpfile(mnt_userns, inode);
> fs/namei.c:                             ima_post_path_mknod(mnt_userns, dentry);
>
> Mimi, can you please take this series and finish the conversion for
> what's missing in ima and evm?
>
> I would also call attention to "175 insertions(+), 240 deletions(-)" --
> as expected, this is a net reduction in code.
>
> Thanks!
>
> -Kees

I endorse this effort.

>
> Kees Cook (9):
>   integrity: Prepare for having "ima" and "evm" available in "integrity"
>     LSM
>   security: Move trivial IMA hooks into LSM
>   ima: Move xattr hooks into LSM
>   ima: Move ima_file_free() into LSM
>   LSM: Introduce inode_post_setattr hook
>   fs: Introduce file_to_perms() helper
>   ima: Move ima_file_check() into LSM
>   integrity: Move trivial hooks into LSM
>   integrity: Move integrity_inode_get() out of global header
>
>  fs/attr.c                             |  3 +-
>  fs/file_table.c                       |  1 -
>  fs/namei.c                            |  2 -
>  fs/nfsd/vfs.c                         |  6 --
>  include/linux/evm.h                   |  6 --
>  include/linux/fs.h                    | 22 +++++++
>  include/linux/ima.h                   | 87 ---------------------------
>  include/linux/integrity.h             | 30 +--------
>  include/linux/lsm_hook_defs.h         |  3 +
>  security/Kconfig                      | 10 +--
>  security/apparmor/include/file.h      | 18 ++----
>  security/integrity/evm/evm_main.c     | 14 ++++-
>  security/integrity/iint.c             | 28 +++++++--
>  security/integrity/ima/ima.h          | 12 ++++
>  security/integrity/ima/ima_appraise.c | 21 +++++--
>  security/integrity/ima/ima_main.c     | 66 ++++++++++++++------
>  security/integrity/integrity.h        |  8 +++
>  security/security.c                   | 78 ++++++------------------
>  18 files changed, 175 insertions(+), 240 deletions(-)
>