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
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 <[email protected]>
Cc: James Morris <[email protected]>
Cc: "Serge E. Hallyn" <[email protected]>
Cc: Mimi Zohar <[email protected]>
Cc: Dmitry Kasatkin <[email protected]>
Cc: "Mickaël Salaün" <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Kees Cook <[email protected]>
---
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
Move the xattr IMA hooks into normal LSM layer. As with SELinux and
Smack, handle calling cap_inode_setxattr() internally.
Cc: Mimi Zohar <[email protected]>
Cc: Dmitry Kasatkin <[email protected]>
Cc: Paul Moore <[email protected]>
Cc: James Morris <[email protected]>
Cc: "Serge E. Hallyn" <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Jonathan McDowell <[email protected]>
Cc: Takashi Iwai <[email protected]>
Cc: Petr Vorel <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Kees Cook <[email protected]>
---
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
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 <[email protected]>
Cc: Dmitry Kasatkin <[email protected]>
Cc: Paul Moore <[email protected]>
Cc: James Morris <[email protected]>
Cc: "Serge E. Hallyn" <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Kees Cook <[email protected]>
---
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
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 <[email protected]>
Cc: Dmitry Kasatkin <[email protected]>
Cc: Paul Moore <[email protected]>
Cc: James Morris <[email protected]>
Cc: "Serge E. Hallyn" <[email protected]>
Cc: Jonathan McDowell <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Kees Cook <[email protected]>
---
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
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 <[email protected]>
Cc: Dmitry Kasatkin <[email protected]>
Cc: Paul Moore <[email protected]>
Cc: James Morris <[email protected]>
Cc: "Serge E. Hallyn" <[email protected]>
Cc: Takashi Iwai <[email protected]>
Cc: Jonathan McDowell <[email protected]>
Cc: Casey Schaufler <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Kees Cook <[email protected]>
---
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
On Thu, Oct 13, 2022 at 6:36 PM Kees Cook <[email protected]> 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
On Thu, 2022-10-13 at 18:47 -0400, Paul Moore wrote:
> On Thu, Oct 13, 2022 at 6:36 PM Kees Cook <[email protected]> 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
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 <[email protected]>
> Cc: James Morris <[email protected]>
> Cc: "Serge E. Hallyn" <[email protected]>
> Cc: Mimi Zohar <[email protected]>
> Cc: Dmitry Kasatkin <[email protected]>
> Cc: "Mickaël Salaün" <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Kees Cook <[email protected]>
> ---
> 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/[email protected]/
> help
> A comma-separated list of LSMs, in initialization order.
> Any LSMs left off this list will be ignored. This can be
> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> index 2e6fb6e2ffd2..1ef965089417 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -904,3 +904,7 @@ static int __init init_evm(void)
> }
>
> late_initcall(init_evm);
> +
> +void __init integrity_lsm_evm_init(void)
> +{
> +}
> diff --git a/security/integrity/iint.c b/security/integrity/iint.c
> index 8638976f7990..4f322324449d 100644
> --- a/security/integrity/iint.c
> +++ b/security/integrity/iint.c
> @@ -18,7 +18,6 @@
> #include <linux/file.h>
> #include <linux/uaccess.h>
> #include <linux/security.h>
> -#include <linux/lsm_hooks.h>
> #include "integrity.h"
>
> static struct rb_root integrity_iint_tree = RB_ROOT;
> @@ -172,19 +171,29 @@ static void init_once(void *foo)
> mutex_init(&iint->mutex);
> }
>
> -static int __init integrity_iintcache_init(void)
> +void __init integrity_add_lsm_hooks(struct security_hook_list *hooks,
> + int count)
> +{
> + security_add_hooks(hooks, count, "integrity");
> +}
> +
> +static int __init integrity_lsm_init(void)
> {
> iint_cache =
> kmem_cache_create("iint_cache", sizeof(struct integrity_iint_cache),
> 0, SLAB_PANIC, init_once);
> +
> + integrity_lsm_ima_init();
> + integrity_lsm_evm_init();
> +
> return 0;
> }
> +
> DEFINE_LSM(integrity) = {
> .name = "integrity",
> - .init = integrity_iintcache_init,
> + .init = integrity_lsm_init,
For backward compatibility, there should be an ".order =
LSM_ORDER_FIRST," here.
On Fri, Oct 14, 2022 at 04:40:01PM +0200, Micka?l Sala?n wrote:
> This is not backward compatible
Why? Nothing will be running LSM hooks until init finishes, at which
point the integrity inode cache will be allocated. And ima and evm don't
start up until lateinit.
>, but can easily be fixed thanks to
> DEFINE_LSM().order
That forces the LSM to be enabled, which may not be desired?
> Side node: I proposed an alternative to that but it was Nacked:
> https://lore.kernel.org/all/[email protected]/
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
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/[email protected]/
>
> 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.
Hi Kees,
I love your patch! Yet something to improve:
[auto build test ERROR on kees/for-next/pstore]
[also build test ERROR on pcmoore-audit/next pcmoore-selinux/next linus/master v6.1-rc1 next-20221017]
[cannot apply to zohar-integrity/next-integrity]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Kees-Cook/integrity-Move-hooks-into-LSM/20221014-063953
base: https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git for-next/pstore
patch link: https://lore.kernel.org/r/20221013223654.659758-5-keescook%40chromium.org
patch subject: [PATCH 5/9] LSM: Introduce inode_post_setattr hook
config: m68k-randconfig-r002-20221012
compiler: m68k-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/4a05d49d9e6e09539a85db353b335221f1181f38
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Kees-Cook/integrity-Move-hooks-into-LSM/20221014-063953
git checkout 4a05d49d9e6e09539a85db353b335221f1181f38
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
fs/attr.c: In function 'notify_change':
>> fs/attr.c:426:17: error: implicit declaration of function 'security_inode_post_setattr'; did you mean 'security_inode_post_setxattr'? [-Werror=implicit-function-declaration]
426 | security_inode_post_setattr(mnt_userns, dentry, ia_valid);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~
| security_inode_post_setxattr
cc1: some warnings being treated as errors
vim +426 fs/attr.c
290
291 /**
292 * notify_change - modify attributes of a filesytem object
293 * @mnt_userns: user namespace of the mount the inode was found from
294 * @dentry: object affected
295 * @attr: new attributes
296 * @delegated_inode: returns inode, if the inode is delegated
297 *
298 * The caller must hold the i_mutex on the affected object.
299 *
300 * If notify_change discovers a delegation in need of breaking,
301 * it will return -EWOULDBLOCK and return a reference to the inode in
302 * delegated_inode. The caller should then break the delegation and
303 * retry. Because breaking a delegation may take a long time, the
304 * caller should drop the i_mutex before doing so.
305 *
306 * Alternatively, a caller may pass NULL for delegated_inode. This may
307 * be appropriate for callers that expect the underlying filesystem not
308 * to be NFS exported. Also, passing NULL is fine for callers holding
309 * the file open for write, as there can be no conflicting delegation in
310 * that case.
311 *
312 * If the inode has been found through an idmapped mount the user namespace of
313 * the vfsmount must be passed through @mnt_userns. This function will then
314 * take care to map the inode according to @mnt_userns before checking
315 * permissions. On non-idmapped mounts or if permission checking is to be
316 * performed on the raw inode simply passs init_user_ns.
317 */
318 int notify_change(struct user_namespace *mnt_userns, struct dentry *dentry,
319 struct iattr *attr, struct inode **delegated_inode)
320 {
321 struct inode *inode = dentry->d_inode;
322 umode_t mode = inode->i_mode;
323 int error;
324 struct timespec64 now;
325 unsigned int ia_valid = attr->ia_valid;
326
327 WARN_ON_ONCE(!inode_is_locked(inode));
328
329 error = may_setattr(mnt_userns, inode, ia_valid);
330 if (error)
331 return error;
332
333 if ((ia_valid & ATTR_MODE)) {
334 umode_t amode = attr->ia_mode;
335 /* Flag setting protected by i_mutex */
336 if (is_sxid(amode))
337 inode->i_flags &= ~S_NOSEC;
338 }
339
340 now = current_time(inode);
341
342 attr->ia_ctime = now;
343 if (!(ia_valid & ATTR_ATIME_SET))
344 attr->ia_atime = now;
345 else
346 attr->ia_atime = timestamp_truncate(attr->ia_atime, inode);
347 if (!(ia_valid & ATTR_MTIME_SET))
348 attr->ia_mtime = now;
349 else
350 attr->ia_mtime = timestamp_truncate(attr->ia_mtime, inode);
351
352 if (ia_valid & ATTR_KILL_PRIV) {
353 error = security_inode_need_killpriv(dentry);
354 if (error < 0)
355 return error;
356 if (error == 0)
357 ia_valid = attr->ia_valid &= ~ATTR_KILL_PRIV;
358 }
359
360 /*
361 * We now pass ATTR_KILL_S*ID to the lower level setattr function so
362 * that the function has the ability to reinterpret a mode change
363 * that's due to these bits. This adds an implicit restriction that
364 * no function will ever call notify_change with both ATTR_MODE and
365 * ATTR_KILL_S*ID set.
366 */
367 if ((ia_valid & (ATTR_KILL_SUID|ATTR_KILL_SGID)) &&
368 (ia_valid & ATTR_MODE))
369 BUG();
370
371 if (ia_valid & ATTR_KILL_SUID) {
372 if (mode & S_ISUID) {
373 ia_valid = attr->ia_valid |= ATTR_MODE;
374 attr->ia_mode = (inode->i_mode & ~S_ISUID);
375 }
376 }
377 if (ia_valid & ATTR_KILL_SGID) {
378 if ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)) {
379 if (!(ia_valid & ATTR_MODE)) {
380 ia_valid = attr->ia_valid |= ATTR_MODE;
381 attr->ia_mode = inode->i_mode;
382 }
383 attr->ia_mode &= ~S_ISGID;
384 }
385 }
386 if (!(attr->ia_valid & ~(ATTR_KILL_SUID | ATTR_KILL_SGID)))
387 return 0;
388
389 /*
390 * Verify that uid/gid changes are valid in the target
391 * namespace of the superblock.
392 */
393 if (ia_valid & ATTR_UID &&
394 !vfsuid_has_fsmapping(mnt_userns, inode->i_sb->s_user_ns,
395 attr->ia_vfsuid))
396 return -EOVERFLOW;
397 if (ia_valid & ATTR_GID &&
398 !vfsgid_has_fsmapping(mnt_userns, inode->i_sb->s_user_ns,
399 attr->ia_vfsgid))
400 return -EOVERFLOW;
401
402 /* Don't allow modifications of files with invalid uids or
403 * gids unless those uids & gids are being made valid.
404 */
405 if (!(ia_valid & ATTR_UID) &&
406 !vfsuid_valid(i_uid_into_vfsuid(mnt_userns, inode)))
407 return -EOVERFLOW;
408 if (!(ia_valid & ATTR_GID) &&
409 !vfsgid_valid(i_gid_into_vfsgid(mnt_userns, inode)))
410 return -EOVERFLOW;
411
412 error = security_inode_setattr(mnt_userns, dentry, attr);
413 if (error)
414 return error;
415 error = try_break_deleg(inode, delegated_inode);
416 if (error)
417 return error;
418
419 if (inode->i_op->setattr)
420 error = inode->i_op->setattr(mnt_userns, dentry, attr);
421 else
422 error = simple_setattr(mnt_userns, dentry, attr);
423
424 if (!error) {
425 fsnotify_change(dentry, ia_valid);
> 426 security_inode_post_setattr(mnt_userns, dentry, ia_valid);
--
0-DAY CI Kernel Test Service
https://01.org/lkp
Hi Kees,
I love your patch! Perhaps something to improve:
[auto build test WARNING on kees/for-next/pstore]
[also build test WARNING on pcmoore-audit/next pcmoore-selinux/next linus/master v6.1-rc1 next-20221017]
[cannot apply to zohar-integrity/next-integrity]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Kees-Cook/integrity-Move-hooks-into-LSM/20221014-063953
base: https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git for-next/pstore
patch link: https://lore.kernel.org/r/20221013223654.659758-5-keescook%40chromium.org
patch subject: [PATCH 5/9] LSM: Introduce inode_post_setattr hook
config: x86_64-randconfig-a013
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/4a05d49d9e6e09539a85db353b335221f1181f38
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Kees-Cook/integrity-Move-hooks-into-LSM/20221014-063953
git checkout 4a05d49d9e6e09539a85db353b335221f1181f38
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
>> security/security.c:1336:6: warning: no previous prototype for 'security_inode_post_setattr' [-Wmissing-prototypes]
1336 | void security_inode_post_setattr(struct user_namespace *mnt_userns,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~
vim +/security_inode_post_setattr +1336 security/security.c
1335
> 1336 void security_inode_post_setattr(struct user_namespace *mnt_userns,
1337 struct dentry *dentry, unsigned int ia_valid)
1338 {
1339 if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
1340 return;
1341 call_void_hook(inode_post_setattr, mnt_userns, dentry, ia_valid);
1342 }
1343
--
0-DAY CI Kernel Test Service
https://01.org/lkp
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/[email protected]/
> >
> > 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
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 <[email protected]>
> Cc: Dmitry Kasatkin <[email protected]>
> Cc: Paul Moore <[email protected]>
> Cc: James Morris <[email protected]>
> Cc: "Serge E. Hallyn" <[email protected]>
> Cc: Takashi Iwai <[email protected]>
> Cc: Jonathan McDowell <[email protected]>
> Cc: Casey Schaufler <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Kees Cook <[email protected]>
> ---
> 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.
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 <[email protected]>
> Cc: Dmitry Kasatkin <[email protected]>
> Cc: Paul Moore <[email protected]>
> Cc: James Morris <[email protected]>
> Cc: "Serge E. Hallyn" <[email protected]>
> Cc: Borislav Petkov <[email protected]>
> Cc: Jonathan McDowell <[email protected]>
> Cc: Takashi Iwai <[email protected]>
> Cc: Petr Vorel <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Kees Cook <[email protected]>
> ---
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?
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/[email protected]/
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(-)
> >
There is a complementary patch series that didn't received review:
https://lore.kernel.org/all/[email protected]/
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(-)
>
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/[email protected]/
Yeah, this looks interesting! I hope Mimi or other integrity folks can
review these.
--
Kees Cook
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 <[email protected]>
> > Cc: Dmitry Kasatkin <[email protected]>
> > Cc: Paul Moore <[email protected]>
> > Cc: James Morris <[email protected]>
> > Cc: "Serge E. Hallyn" <[email protected]>
> > Cc: Borislav Petkov <[email protected]>
> > Cc: Jonathan McDowell <[email protected]>
> > Cc: Takashi Iwai <[email protected]>
> > Cc: Petr Vorel <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > Signed-off-by: Kees Cook <[email protected]>
> > ---
>
> 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
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
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
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
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
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
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(-)
>