| Message ID | 20231214170834.3324559-1-roberto.sassu@huaweicloud.com |
|---|---|
| Headers | show |
| Series | security: Move IMA and EVM to the LSM infrastructure | expand |
On 12/14/2023 9:08 AM, Roberto Sassu wrote: > From: Roberto Sassu <roberto.sassu@huawei.com> > > In preparation for moving IMA and EVM to the LSM infrastructure, introduce > the inode_post_removexattr hook. > > At inode_removexattr hook, EVM verifies the file's existing HMAC value. At > inode_post_removexattr, EVM re-calculates the file's HMAC with the passed > xattr removed and other file metadata. > > Other LSMs could similarly take some action after successful xattr removal. > > The new hook cannot return an error and cannot cause the operation to be > reverted. > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > Reviewed-by: Stefan Berger <stefanb@linux.ibm.com> > Reviewed-by: Mimi Zohar <zohar@linux.ibm.com> Reviewed-by: Casey Schaufler <casey@schaufler-ca.com> > --- > fs/xattr.c | 9 +++++---- > include/linux/lsm_hook_defs.h | 2 ++ > include/linux/security.h | 5 +++++ > security/security.c | 14 ++++++++++++++ > 4 files changed, 26 insertions(+), 4 deletions(-) > > diff --git a/fs/xattr.c b/fs/xattr.c > index 09d927603433..f891c260a971 100644 > --- a/fs/xattr.c > +++ b/fs/xattr.c > @@ -552,11 +552,12 @@ __vfs_removexattr_locked(struct mnt_idmap *idmap, > goto out; > > error = __vfs_removexattr(idmap, dentry, name); > + if (error) > + return error; > > - if (!error) { > - fsnotify_xattr(dentry); > - evm_inode_post_removexattr(dentry, name); > - } > + fsnotify_xattr(dentry); > + security_inode_post_removexattr(dentry, name); > + evm_inode_post_removexattr(dentry, name); > > out: > return error; > diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h > index 091cddb4e6de..c3199bb69103 100644 > --- a/include/linux/lsm_hook_defs.h > +++ b/include/linux/lsm_hook_defs.h > @@ -149,6 +149,8 @@ LSM_HOOK(int, 0, inode_getxattr, struct dentry *dentry, const char *name) > LSM_HOOK(int, 0, inode_listxattr, struct dentry *dentry) > LSM_HOOK(int, 0, inode_removexattr, struct mnt_idmap *idmap, > struct dentry *dentry, const char *name) > +LSM_HOOK(void, LSM_RET_VOID, inode_post_removexattr, struct dentry *dentry, > + const char *name) > LSM_HOOK(int, 0, inode_set_acl, struct mnt_idmap *idmap, > struct dentry *dentry, const char *acl_name, struct posix_acl *kacl) > LSM_HOOK(int, 0, inode_get_acl, struct mnt_idmap *idmap, > diff --git a/include/linux/security.h b/include/linux/security.h > index 664df46b22a9..922ea7709bae 100644 > --- a/include/linux/security.h > +++ b/include/linux/security.h > @@ -380,6 +380,7 @@ int security_inode_getxattr(struct dentry *dentry, const char *name); > int security_inode_listxattr(struct dentry *dentry); > int security_inode_removexattr(struct mnt_idmap *idmap, > struct dentry *dentry, const char *name); > +void security_inode_post_removexattr(struct dentry *dentry, const char *name); > int security_inode_need_killpriv(struct dentry *dentry); > int security_inode_killpriv(struct mnt_idmap *idmap, struct dentry *dentry); > int security_inode_getsecurity(struct mnt_idmap *idmap, > @@ -940,6 +941,10 @@ static inline int security_inode_removexattr(struct mnt_idmap *idmap, > return cap_inode_removexattr(idmap, dentry, name); > } > > +static inline void security_inode_post_removexattr(struct dentry *dentry, > + const char *name) > +{ } > + > static inline int security_inode_need_killpriv(struct dentry *dentry) > { > return cap_inode_need_killpriv(dentry); > diff --git a/security/security.c b/security/security.c > index ce3bc7642e18..8aa6e9f316dd 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -2452,6 +2452,20 @@ int security_inode_removexattr(struct mnt_idmap *idmap, > return evm_inode_removexattr(idmap, dentry, name); > } > > +/** > + * security_inode_post_removexattr() - Update the inode after a removexattr op > + * @dentry: file > + * @name: xattr name > + * > + * Update the inode after a successful removexattr operation. > + */ > +void security_inode_post_removexattr(struct dentry *dentry, const char *name) > +{ > + if (unlikely(IS_PRIVATE(d_backing_inode(dentry)))) > + return; > + call_void_hook(inode_post_removexattr, dentry, name); > +} > + > /** > * security_inode_need_killpriv() - Check if security_inode_killpriv() required > * @dentry: associated dentry
On 12/14/2023 9:08 AM, Roberto Sassu wrote: > From: Roberto Sassu <roberto.sassu@huawei.com> > > In preparation for moving IMA and EVM to the LSM infrastructure, introduce > the file_release hook. > > IMA calculates at file close the new digest of the file content and writes > it to security.ima, so that appraisal at next file access succeeds. > > LSMs could also take some action before the last reference of a file is > released. You could make this more convincing with an example. Perhaps: An LSM could implement an exclusive access scheme for files, only allowing access to files that have no references. > > The new hook cannot return an error and cannot cause the operation to be > reverted. > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > --- > fs/file_table.c | 1 + > include/linux/lsm_hook_defs.h | 1 + > include/linux/security.h | 4 ++++ > security/security.c | 11 +++++++++++ > 4 files changed, 17 insertions(+) > > diff --git a/fs/file_table.c b/fs/file_table.c > index de4a2915bfd4..c72dc75f2bd3 100644 > --- a/fs/file_table.c > +++ b/fs/file_table.c > @@ -385,6 +385,7 @@ static void __fput(struct file *file) > eventpoll_release(file); > locks_remove_file(file); > > + security_file_release(file); > ima_file_free(file); > if (unlikely(file->f_flags & FASYNC)) { > if (file->f_op->fasync) > diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h > index e2b45fee94e2..175ca00a6b1d 100644 > --- a/include/linux/lsm_hook_defs.h > +++ b/include/linux/lsm_hook_defs.h > @@ -173,6 +173,7 @@ LSM_HOOK(int, 0, kernfs_init_security, struct kernfs_node *kn_dir, > struct kernfs_node *kn) > LSM_HOOK(int, 0, file_permission, struct file *file, int mask) > LSM_HOOK(int, 0, file_alloc_security, struct file *file) > +LSM_HOOK(void, LSM_RET_VOID, file_release, struct file *file) > LSM_HOOK(void, LSM_RET_VOID, file_free_security, struct file *file) > LSM_HOOK(int, 0, file_ioctl, struct file *file, unsigned int cmd, > unsigned long arg) > diff --git a/include/linux/security.h b/include/linux/security.h > index c360458920b1..4c3585e3dcb4 100644 > --- a/include/linux/security.h > +++ b/include/linux/security.h > @@ -395,6 +395,7 @@ int security_kernfs_init_security(struct kernfs_node *kn_dir, > struct kernfs_node *kn); > int security_file_permission(struct file *file, int mask); > int security_file_alloc(struct file *file); > +void security_file_release(struct file *file); > void security_file_free(struct file *file); > int security_file_ioctl(struct file *file, unsigned int cmd, unsigned long arg); > int security_mmap_file(struct file *file, unsigned long prot, > @@ -1006,6 +1007,9 @@ static inline int security_file_alloc(struct file *file) > return 0; > } > > +static inline void security_file_release(struct file *file) > +{ } > + > static inline void security_file_free(struct file *file) > { } > > diff --git a/security/security.c b/security/security.c > index fe6a160afc35..9aa072ca5a19 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -2724,6 +2724,17 @@ int security_file_alloc(struct file *file) > return rc; > } > > +/** > + * security_file_release() - Perform actions before releasing the file ref > + * @file: the file > + * > + * Perform actions before releasing the last reference to a file. > + */ > +void security_file_release(struct file *file) > +{ > + call_void_hook(file_release, file); > +} > + > /** > * security_file_free() - Free a file's LSM blob > * @file: the file
On 12/14/2023 9:08 AM, Roberto Sassu wrote: > From: Roberto Sassu <roberto.sassu@huawei.com> > > As for IMA, move hardcoded EVM function calls from various places in the > kernel to the LSM infrastructure, by introducing a new LSM named 'evm' > (last and always enabled like 'ima'). The order in the Makefile ensures > that 'evm' hooks are executed after 'ima' ones. > > Make EVM functions as static (except for evm_inode_init_security(), which > is exported), and register them as hook implementations in init_evm_lsm(). > > Unlike before (see commit to move IMA to the LSM infrastructure), > evm_inode_post_setattr(), evm_inode_post_set_acl(), > evm_inode_post_remove_acl(), and evm_inode_post_removexattr() are not > executed for private inodes. > > Finally, add the LSM_ID_EVM case in lsm_list_modules_test.c > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> Reviewed-by: Casey Schaufler <casey@schaufler-ca.com> > --- > fs/attr.c | 2 - > fs/posix_acl.c | 3 - > fs/xattr.c | 2 - > include/linux/evm.h | 107 ---------------- > include/uapi/linux/lsm.h | 1 + > security/integrity/evm/evm_main.c | 115 +++++++++++++++--- > security/security.c | 41 ++----- > .../selftests/lsm/lsm_list_modules_test.c | 3 + > 8 files changed, 113 insertions(+), 161 deletions(-) > > diff --git a/fs/attr.c b/fs/attr.c > index 38841f3ebbcb..b51bd7c9b4a7 100644 > --- a/fs/attr.c > +++ b/fs/attr.c > @@ -16,7 +16,6 @@ > #include <linux/fcntl.h> > #include <linux/filelock.h> > #include <linux/security.h> > -#include <linux/evm.h> > > #include "internal.h" > > @@ -502,7 +501,6 @@ int notify_change(struct mnt_idmap *idmap, struct dentry *dentry, > if (!error) { > fsnotify_change(dentry, ia_valid); > security_inode_post_setattr(idmap, dentry, ia_valid); > - evm_inode_post_setattr(idmap, dentry, ia_valid); > } > > return error; > diff --git a/fs/posix_acl.c b/fs/posix_acl.c > index e3fbe1a9f3f5..ae67479cd2b6 100644 > --- a/fs/posix_acl.c > +++ b/fs/posix_acl.c > @@ -26,7 +26,6 @@ > #include <linux/mnt_idmapping.h> > #include <linux/iversion.h> > #include <linux/security.h> > -#include <linux/evm.h> > #include <linux/fsnotify.h> > #include <linux/filelock.h> > > @@ -1138,7 +1137,6 @@ int vfs_set_acl(struct mnt_idmap *idmap, struct dentry *dentry, > if (!error) { > fsnotify_xattr(dentry); > security_inode_post_set_acl(dentry, acl_name, kacl); > - evm_inode_post_set_acl(dentry, acl_name, kacl); > } > > out_inode_unlock: > @@ -1247,7 +1245,6 @@ int vfs_remove_acl(struct mnt_idmap *idmap, struct dentry *dentry, > if (!error) { > fsnotify_xattr(dentry); > security_inode_post_remove_acl(idmap, dentry, acl_name); > - evm_inode_post_remove_acl(idmap, dentry, acl_name); > } > > out_inode_unlock: > diff --git a/fs/xattr.c b/fs/xattr.c > index f891c260a971..f8b643f91a98 100644 > --- a/fs/xattr.c > +++ b/fs/xattr.c > @@ -16,7 +16,6 @@ > #include <linux/mount.h> > #include <linux/namei.h> > #include <linux/security.h> > -#include <linux/evm.h> > #include <linux/syscalls.h> > #include <linux/export.h> > #include <linux/fsnotify.h> > @@ -557,7 +556,6 @@ __vfs_removexattr_locked(struct mnt_idmap *idmap, > > fsnotify_xattr(dentry); > security_inode_post_removexattr(dentry, name); > - evm_inode_post_removexattr(dentry, name); > > out: > return error; > diff --git a/include/linux/evm.h b/include/linux/evm.h > index 437d4076a3b3..cb481eccc967 100644 > --- a/include/linux/evm.h > +++ b/include/linux/evm.h > @@ -21,44 +21,6 @@ extern enum integrity_status evm_verifyxattr(struct dentry *dentry, > void *xattr_value, > size_t xattr_value_len, > struct integrity_iint_cache *iint); > -extern int evm_inode_setattr(struct mnt_idmap *idmap, > - struct dentry *dentry, struct iattr *attr); > -extern void evm_inode_post_setattr(struct mnt_idmap *idmap, > - struct dentry *dentry, int ia_valid); > -extern int evm_inode_setxattr(struct mnt_idmap *idmap, > - struct dentry *dentry, const char *name, > - const void *value, size_t size, int flags); > -extern void evm_inode_post_setxattr(struct dentry *dentry, > - const char *xattr_name, > - const void *xattr_value, > - size_t xattr_value_len, > - int flags); > -extern int evm_inode_removexattr(struct mnt_idmap *idmap, > - struct dentry *dentry, const char *xattr_name); > -extern void evm_inode_post_removexattr(struct dentry *dentry, > - const char *xattr_name); > -static inline void evm_inode_post_remove_acl(struct mnt_idmap *idmap, > - struct dentry *dentry, > - const char *acl_name) > -{ > - evm_inode_post_removexattr(dentry, acl_name); > -} > -extern int evm_inode_set_acl(struct mnt_idmap *idmap, > - struct dentry *dentry, const char *acl_name, > - struct posix_acl *kacl); > -static inline int evm_inode_remove_acl(struct mnt_idmap *idmap, > - struct dentry *dentry, > - const char *acl_name) > -{ > - return evm_inode_set_acl(idmap, dentry, acl_name, NULL); > -} > -static inline void evm_inode_post_set_acl(struct dentry *dentry, > - const char *acl_name, > - struct posix_acl *kacl) > -{ > - return evm_inode_post_setxattr(dentry, acl_name, NULL, 0, 0); > -} > - > int evm_inode_init_security(struct inode *inode, struct inode *dir, > const struct qstr *qstr, struct xattr *xattrs, > int *xattr_count); > @@ -93,75 +55,6 @@ static inline enum integrity_status evm_verifyxattr(struct dentry *dentry, > } > #endif > > -static inline int evm_inode_setattr(struct mnt_idmap *idmap, > - struct dentry *dentry, struct iattr *attr) > -{ > - return 0; > -} > - > -static inline void evm_inode_post_setattr(struct mnt_idmap *idmap, > - struct dentry *dentry, int ia_valid) > -{ > - return; > -} > - > -static inline int evm_inode_setxattr(struct mnt_idmap *idmap, > - struct dentry *dentry, const char *name, > - const void *value, size_t size, int flags) > -{ > - return 0; > -} > - > -static inline void evm_inode_post_setxattr(struct dentry *dentry, > - const char *xattr_name, > - const void *xattr_value, > - size_t xattr_value_len, > - int flags) > -{ > - return; > -} > - > -static inline int evm_inode_removexattr(struct mnt_idmap *idmap, > - struct dentry *dentry, > - const char *xattr_name) > -{ > - return 0; > -} > - > -static inline void evm_inode_post_removexattr(struct dentry *dentry, > - const char *xattr_name) > -{ > - return; > -} > - > -static inline void evm_inode_post_remove_acl(struct mnt_idmap *idmap, > - struct dentry *dentry, > - const char *acl_name) > -{ > - return; > -} > - > -static inline int evm_inode_set_acl(struct mnt_idmap *idmap, > - struct dentry *dentry, const char *acl_name, > - struct posix_acl *kacl) > -{ > - return 0; > -} > - > -static inline int evm_inode_remove_acl(struct mnt_idmap *idmap, > - struct dentry *dentry, > - const char *acl_name) > -{ > - return 0; > -} > - > -static inline void evm_inode_post_set_acl(struct dentry *dentry, > - const char *acl_name, > - struct posix_acl *kacl) > -{ > - return; > -} > - > static inline int evm_inode_init_security(struct inode *inode, struct inode *dir, > const struct qstr *qstr, > struct xattr *xattrs, > diff --git a/include/uapi/linux/lsm.h b/include/uapi/linux/lsm.h > index ee7d034255a9..825339bcd580 100644 > --- a/include/uapi/linux/lsm.h > +++ b/include/uapi/linux/lsm.h > @@ -62,6 +62,7 @@ struct lsm_ctx { > #define LSM_ID_BPF 109 > #define LSM_ID_LANDLOCK 110 > #define LSM_ID_IMA 111 > +#define LSM_ID_EVM 112 > > /* > * LSM_ATTR_XXX definitions identify different LSM attributes > diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c > index ea84a6f835ff..0cd014bfc093 100644 > --- a/security/integrity/evm/evm_main.c > +++ b/security/integrity/evm/evm_main.c > @@ -566,9 +566,9 @@ static int evm_protect_xattr(struct mnt_idmap *idmap, > * userspace from writing HMAC value. Writing 'security.evm' requires > * requires CAP_SYS_ADMIN privileges. > */ > -int evm_inode_setxattr(struct mnt_idmap *idmap, struct dentry *dentry, > - const char *xattr_name, const void *xattr_value, > - size_t xattr_value_len, int flags) > +static int evm_inode_setxattr(struct mnt_idmap *idmap, struct dentry *dentry, > + const char *xattr_name, const void *xattr_value, > + size_t xattr_value_len, int flags) > { > const struct evm_ima_xattr_data *xattr_data = xattr_value; > > @@ -598,8 +598,8 @@ int evm_inode_setxattr(struct mnt_idmap *idmap, struct dentry *dentry, > * Removing 'security.evm' requires CAP_SYS_ADMIN privileges and that > * the current value is valid. > */ > -int evm_inode_removexattr(struct mnt_idmap *idmap, > - struct dentry *dentry, const char *xattr_name) > +static int evm_inode_removexattr(struct mnt_idmap *idmap, struct dentry *dentry, > + const char *xattr_name) > { > /* Policy permits modification of the protected xattrs even though > * there's no HMAC key loaded > @@ -649,9 +649,11 @@ static inline int evm_inode_set_acl_change(struct mnt_idmap *idmap, > * Prevent modifying posix acls causing the EVM HMAC to be re-calculated > * and 'security.evm' xattr updated, unless the existing 'security.evm' is > * valid. > + * > + * Return: zero on success, -EPERM on failure. > */ > -int evm_inode_set_acl(struct mnt_idmap *idmap, struct dentry *dentry, > - const char *acl_name, struct posix_acl *kacl) > +static int evm_inode_set_acl(struct mnt_idmap *idmap, struct dentry *dentry, > + const char *acl_name, struct posix_acl *kacl) > { > enum integrity_status evm_status; > > @@ -690,6 +692,24 @@ int evm_inode_set_acl(struct mnt_idmap *idmap, struct dentry *dentry, > return -EPERM; > } > > +/** > + * evm_inode_remove_acl - Protect the EVM extended attribute from posix acls > + * @idmap: idmap of the mount > + * @dentry: pointer to the affected dentry > + * @acl_name: name of the posix acl > + * > + * Prevent removing posix acls causing the EVM HMAC to be re-calculated > + * and 'security.evm' xattr updated, unless the existing 'security.evm' is > + * valid. > + * > + * Return: zero on success, -EPERM on failure. > + */ > +static int evm_inode_remove_acl(struct mnt_idmap *idmap, struct dentry *dentry, > + const char *acl_name) > +{ > + return evm_inode_set_acl(idmap, dentry, acl_name, NULL); > +} > + > static void evm_reset_status(struct inode *inode) > { > struct integrity_iint_cache *iint; > @@ -738,9 +758,11 @@ bool evm_revalidate_status(const char *xattr_name) > * __vfs_setxattr_noperm(). The caller of which has taken the inode's > * i_mutex lock. > */ > -void evm_inode_post_setxattr(struct dentry *dentry, const char *xattr_name, > - const void *xattr_value, size_t xattr_value_len, > - int flags) > +static void evm_inode_post_setxattr(struct dentry *dentry, > + const char *xattr_name, > + const void *xattr_value, > + size_t xattr_value_len, > + int flags) > { > if (!evm_revalidate_status(xattr_name)) > return; > @@ -756,6 +778,21 @@ void evm_inode_post_setxattr(struct dentry *dentry, const char *xattr_name, > evm_update_evmxattr(dentry, xattr_name, xattr_value, xattr_value_len); > } > > +/** > + * evm_inode_post_set_acl - Update the EVM extended attribute from posix acls > + * @dentry: pointer to the affected dentry > + * @acl_name: name of the posix acl > + * @kacl: pointer to the posix acls > + * > + * Update the 'security.evm' xattr with the EVM HMAC re-calculated after setting > + * posix acls. > + */ > +static void evm_inode_post_set_acl(struct dentry *dentry, const char *acl_name, > + struct posix_acl *kacl) > +{ > + return evm_inode_post_setxattr(dentry, acl_name, NULL, 0, 0); > +} > + > /** > * evm_inode_post_removexattr - update 'security.evm' after removing the xattr > * @dentry: pointer to the affected dentry > @@ -766,7 +803,8 @@ void evm_inode_post_setxattr(struct dentry *dentry, const char *xattr_name, > * No need to take the i_mutex lock here, as this function is called from > * vfs_removexattr() which takes the i_mutex. > */ > -void evm_inode_post_removexattr(struct dentry *dentry, const char *xattr_name) > +static void evm_inode_post_removexattr(struct dentry *dentry, > + const char *xattr_name) > { > if (!evm_revalidate_status(xattr_name)) > return; > @@ -782,6 +820,22 @@ void evm_inode_post_removexattr(struct dentry *dentry, const char *xattr_name) > evm_update_evmxattr(dentry, xattr_name, NULL, 0); > } > > +/** > + * evm_inode_post_remove_acl - Update the EVM extended attribute from posix acls > + * @idmap: idmap of the mount > + * @dentry: pointer to the affected dentry > + * @acl_name: name of the posix acl > + * > + * Update the 'security.evm' xattr with the EVM HMAC re-calculated after > + * removing posix acls. > + */ > +static inline void evm_inode_post_remove_acl(struct mnt_idmap *idmap, > + struct dentry *dentry, > + const char *acl_name) > +{ > + evm_inode_post_removexattr(dentry, acl_name); > +} > + > static int evm_attr_change(struct mnt_idmap *idmap, > struct dentry *dentry, struct iattr *attr) > { > @@ -805,8 +859,8 @@ static int evm_attr_change(struct mnt_idmap *idmap, > * Permit update of file attributes when files have a valid EVM signature, > * except in the case of them having an immutable portable signature. > */ > -int evm_inode_setattr(struct mnt_idmap *idmap, struct dentry *dentry, > - struct iattr *attr) > +static int evm_inode_setattr(struct mnt_idmap *idmap, struct dentry *dentry, > + struct iattr *attr) > { > unsigned int ia_valid = attr->ia_valid; > enum integrity_status evm_status; > @@ -853,8 +907,8 @@ int evm_inode_setattr(struct mnt_idmap *idmap, 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 mnt_idmap *idmap, struct dentry *dentry, > - int ia_valid) > +static void evm_inode_post_setattr(struct mnt_idmap *idmap, > + struct dentry *dentry, int ia_valid) > { > if (!evm_revalidate_status(NULL)) > return; > @@ -964,4 +1018,35 @@ static int __init init_evm(void) > return error; > } > > +static struct security_hook_list evm_hooks[] __ro_after_init = { > + LSM_HOOK_INIT(inode_setattr, evm_inode_setattr), > + LSM_HOOK_INIT(inode_post_setattr, evm_inode_post_setattr), > + LSM_HOOK_INIT(inode_setxattr, evm_inode_setxattr), > + LSM_HOOK_INIT(inode_set_acl, evm_inode_set_acl), > + LSM_HOOK_INIT(inode_post_set_acl, evm_inode_post_set_acl), > + LSM_HOOK_INIT(inode_remove_acl, evm_inode_remove_acl), > + LSM_HOOK_INIT(inode_post_remove_acl, evm_inode_post_remove_acl), > + LSM_HOOK_INIT(inode_post_setxattr, evm_inode_post_setxattr), > + LSM_HOOK_INIT(inode_removexattr, evm_inode_removexattr), > + LSM_HOOK_INIT(inode_post_removexattr, evm_inode_post_removexattr), > + LSM_HOOK_INIT(inode_init_security, evm_inode_init_security), > +}; > + > +static const struct lsm_id evm_lsmid = { > + .name = "evm", > + .id = LSM_ID_EVM, > +}; > + > +static int __init init_evm_lsm(void) > +{ > + security_add_hooks(evm_hooks, ARRAY_SIZE(evm_hooks), &evm_lsmid); > + return 0; > +} > + > +DEFINE_LSM(evm) = { > + .name = "evm", > + .init = init_evm_lsm, > + .order = LSM_ORDER_LAST, > +}; > + > late_initcall(init_evm); > diff --git a/security/security.c b/security/security.c > index d4ead59fb91f..18a70aa707ad 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -20,13 +20,13 @@ > #include <linux/kernel_read_file.h> > #include <linux/lsm_hooks.h> > #include <linux/integrity.h> > -#include <linux/evm.h> > #include <linux/fsnotify.h> > #include <linux/mman.h> > #include <linux/mount.h> > #include <linux/personality.h> > #include <linux/backing-dev.h> > #include <linux/string.h> > +#include <linux/xattr.h> > #include <linux/msg.h> > #include <net/flow.h> > > @@ -50,7 +50,8 @@ > (IS_ENABLED(CONFIG_SECURITY_LOCKDOWN_LSM) ? 1 : 0) + \ > (IS_ENABLED(CONFIG_BPF_LSM) ? 1 : 0) + \ > (IS_ENABLED(CONFIG_SECURITY_LANDLOCK) ? 1 : 0) + \ > - (IS_ENABLED(CONFIG_IMA) ? 1 : 0)) > + (IS_ENABLED(CONFIG_IMA) ? 1 : 0) + \ > + (IS_ENABLED(CONFIG_EVM) ? 1 : 0)) > > /* > * These are descriptions of the reasons that can be passed to the > @@ -1740,10 +1741,6 @@ int security_inode_init_security(struct inode *inode, struct inode *dir, > if (!xattr_count) > goto out; > > - ret = evm_inode_init_security(inode, dir, qstr, new_xattrs, > - &xattr_count); > - if (ret) > - goto out; > ret = initxattrs(inode, new_xattrs, fs_data); > out: > for (; xattr_count > 0; xattr_count--) > @@ -2235,14 +2232,9 @@ int security_inode_permission(struct inode *inode, int mask) > int security_inode_setattr(struct mnt_idmap *idmap, > struct dentry *dentry, struct iattr *attr) > { > - int ret; > - > if (unlikely(IS_PRIVATE(d_backing_inode(dentry)))) > return 0; > - ret = call_int_hook(inode_setattr, 0, idmap, dentry, attr); > - if (ret) > - return ret; > - return evm_inode_setattr(idmap, dentry, attr); > + return call_int_hook(inode_setattr, 0, idmap, dentry, attr); > } > EXPORT_SYMBOL_GPL(security_inode_setattr); > > @@ -2307,9 +2299,7 @@ int security_inode_setxattr(struct mnt_idmap *idmap, > > if (ret == 1) > ret = cap_inode_setxattr(dentry, name, value, size, flags); > - if (ret) > - return ret; > - return evm_inode_setxattr(idmap, dentry, name, value, size, flags); > + return ret; > } > > /** > @@ -2328,15 +2318,10 @@ int security_inode_set_acl(struct mnt_idmap *idmap, > struct dentry *dentry, const char *acl_name, > struct posix_acl *kacl) > { > - int ret; > - > if (unlikely(IS_PRIVATE(d_backing_inode(dentry)))) > return 0; > - ret = call_int_hook(inode_set_acl, 0, idmap, dentry, acl_name, > - kacl); > - if (ret) > - return ret; > - return evm_inode_set_acl(idmap, dentry, acl_name, kacl); > + return call_int_hook(inode_set_acl, 0, idmap, dentry, acl_name, > + kacl); > } > > /** > @@ -2389,14 +2374,9 @@ int security_inode_get_acl(struct mnt_idmap *idmap, > int security_inode_remove_acl(struct mnt_idmap *idmap, > struct dentry *dentry, const char *acl_name) > { > - int ret; > - > if (unlikely(IS_PRIVATE(d_backing_inode(dentry)))) > return 0; > - ret = call_int_hook(inode_remove_acl, 0, idmap, dentry, acl_name); > - if (ret) > - return ret; > - return evm_inode_remove_acl(idmap, dentry, acl_name); > + return call_int_hook(inode_remove_acl, 0, idmap, dentry, acl_name); > } > > /** > @@ -2432,7 +2412,6 @@ void security_inode_post_setxattr(struct dentry *dentry, const char *name, > if (unlikely(IS_PRIVATE(d_backing_inode(dentry)))) > return; > call_void_hook(inode_post_setxattr, dentry, name, value, size, flags); > - evm_inode_post_setxattr(dentry, name, value, size, flags); > } > > /** > @@ -2493,9 +2472,7 @@ int security_inode_removexattr(struct mnt_idmap *idmap, > ret = call_int_hook(inode_removexattr, 1, idmap, dentry, name); > if (ret == 1) > ret = cap_inode_removexattr(idmap, dentry, name); > - if (ret) > - return ret; > - return evm_inode_removexattr(idmap, dentry, name); > + return ret; > } > > /** > diff --git a/tools/testing/selftests/lsm/lsm_list_modules_test.c b/tools/testing/selftests/lsm/lsm_list_modules_test.c > index 17333787cb2f..4d5d4cee2586 100644 > --- a/tools/testing/selftests/lsm/lsm_list_modules_test.c > +++ b/tools/testing/selftests/lsm/lsm_list_modules_test.c > @@ -125,6 +125,9 @@ TEST(correct_lsm_list_modules) > case LSM_ID_IMA: > name = "ima"; > break; > + case LSM_ID_EVM: > + name = "evm"; > + break; > default: > name = "INVALID"; > break;
From: Roberto Sassu <roberto.sassu@huawei.com> IMA and EVM are not effectively LSMs, especially due to the fact that in the past they could not provide a security blob while there is another LSM active. That changed in the recent years, the LSM stacking feature now makes it possible to stack together multiple LSMs, and allows them to provide a security blob for most kernel objects. While the LSM stacking feature has some limitations being worked out, it is already suitable to make IMA and EVM as LSMs. The main purpose of this patch set is to remove IMA and EVM function calls, hardcoded in the LSM infrastructure and other places in the kernel, and to register them as LSM hook implementations, so that those functions are called by the LSM infrastructure like other regular LSMs. This patch set introduces two new LSMs 'ima' and 'evm', so that functions can be registered to their respective LSM, and removes the 'integrity' LSM. integrity_kernel_module_request() was moved to IMA, since it was related to appraisal. integrity_inode_free() was replaced with ima_inode_free_security() (EVM does not need to free memory). In order to make 'ima' and 'evm' independent LSMs, it was necessary to split integrity metadata used by both IMA and EVM, and to let them manage their own. The special case of the IMA_NEW_FILE flag, managed by IMA and used by EVM, was handled by introducing a new flag in EVM, EVM_NEW_FILE, managed by two additional LSM hooks, evm_post_path_mknod() and evm_file_free(), equivalent to their counterparts ima_post_path_mknod() and ima_file_free(). In addition to splitting metadata, it was decided to embed the full structure into the inode security blob, rather than using a cache of objects and allocating them on demand. This opens for new possibilities, such as improving locking in IMA. Another follow-up change was removing the iint parameter from evm_verifyxattr(), that IMA used to pass integrity metadata to EVM. After splitting metadata, and aligning EVM_NEW_FILE with IMA_NEW_FILE, this parameter was not necessary anymore. The last part was to ensure that the order of IMA and EVM functions is respected after they become LSMs. Since the order of lsm_info structures in the .lsm_info.init section depends on the order object files containing those structures are passed to the linker of the kernel image, and since IMA is before EVM in the Makefile, that is sufficient to assert that IMA functions are executed before EVM ones. The patch set is organized as follows. Patches 1-9 make IMA and EVM functions suitable to be registered to the LSM infrastructure, by aligning function parameters. Patches 10-18 add new LSM hooks in the same places where IMA and EVM functions are called, if there is no LSM hook already. Patches 19-21 introduce the new standalone LSMs 'ima' and 'evm', and move hardcoded calls to IMA, EVM and integrity functions to those LSMs. Patches 22-23 remove the dependency on the 'integrity' LSM by splitting integrity metadata, so that the 'ima' and 'evm' LSMs can use their own. They also duplicate iint_lockdep_annotate() in ima_main.c, since the mutex field was moved from integrity_iint_cache to ima_iint_cache. Patch 24 finally removes the 'integrity' LSM, since 'ima' and 'evm' are now self-contained and independent. The patch set applies on top of lsm/dev, commit 80b4ff1d2c9b ("selftests: remove the LSM_ID_IMA check in lsm/lsm_list_modules_test"). The linux-integrity/next-integrity-testing at commit f17167bea279 ("ima: Remove EXPERIMENTAL from Kconfig") was merged. Changelog: v7: - Use return instead of goto in __vfs_removexattr_locked() (suggested by Casey) - Clarify in security/integrity/Makefile that the order of 'ima' and 'evm' LSMs depends on the order in which IMA and EVM are compiled - Move integrity_iint_cache flags to ima.h and evm.h in security/ and duplicate IMA_NEW_FILE to EVM_NEW_FILE - Rename evm_inode_get_iint() to evm_iint_inode() and ima_inode_get_iint() to ima_iint_inode(), check if inode->i_security is NULL, and just return the pointer from the inode security blob - Restore the non-NULL checks after ima_iint_inode() and evm_iint_inode() (suggested by Casey) - Introduce evm_file_free() to clear EVM_NEW_FILE - Remove comment about LSM_ORDER_LAST not guaranteeing the order of 'ima' and 'evm' LSMs - Lock iint->mutex before reading IMA_COLLECTED flag in __ima_inode_hash() and restored ima_policy_flag check - Remove patch about the hardcoded ordering of 'ima' and 'evm' LSMs in security.c - Add missing ima_inode_free_security() to free iint->ima_hash - Add the cases for LSM_ID_IMA and LSM_ID_EVM in lsm_list_modules_test.c - Mention about the change in IMA and EVM post functions for private inodes v6: - See v7 v5: - Rename security_file_pre_free() to security_file_release() and the LSM hook file_pre_free_security to file_release (suggested by Paul) - Move integrity_kernel_module_request() to ima_main.c (renamed to ima_kernel_module_request()) - Split the integrity_iint_cache structure into ima_iint_cache and evm_iint_cache, so that IMA and EVM can use disjoint metadata and reserve space with the LSM infrastructure - Reserve space for the entire ima_iint_cache and evm_iint_cache structures, not just the pointer (suggested by Paul) - Introduce ima_inode_get_iint() and evm_inode_get_iint() to retrieve respectively the ima_iint_cache and evm_iint_cache structure from the security blob - Remove the various non-NULL checks for the ima_iint_cache and evm_iint_cache structures, since the LSM infrastructure ensure that they always exist - Remove the iint parameter from evm_verifyxattr() since IMA and EVM use disjoint integrity metaddata - Introduce the evm_post_path_mknod() to set the IMA_NEW_FILE flag - Register the inode_alloc_security LSM hook in IMA and EVM to initialize the respective integrity metadata structures - Remove the 'integrity' LSM completely and instead make 'ima' and 'evm' proper standalone LSMs - Add the inode parameter to ima_get_verity_digest(), since the inode field is not present in ima_iint_cache - Move iint_lockdep_annotate() to ima_main.c (renamed to ima_iint_lockdep_annotate()) - Remove ima_get_lsm_id() and evm_get_lsm_id(), since IMA and EVM directly register the needed LSM hooks - Enforce 'ima' and 'evm' LSM ordering at LSM infrastructure level v4: - Improve short and long description of security_inode_post_create_tmpfile(), security_inode_post_set_acl(), security_inode_post_remove_acl() and security_file_post_open() (suggested by Mimi) - Improve commit message of 'ima: Move to LSM infrastructure' (suggested by Mimi) v3: - Drop 'ima: Align ima_post_path_mknod() definition with LSM infrastructure' and 'ima: Align ima_post_create_tmpfile() definition with LSM infrastructure', define the new LSM hooks with the same IMA parameters instead (suggested by Mimi) - Do IS_PRIVATE() check in security_path_post_mknod() and security_inode_post_create_tmpfile() on the new inode rather than the parent directory (in the post method it is available) - Don't export ima_file_check() (suggested by Stefan) - Remove redundant check of file mode in ima_post_path_mknod() (suggested by Mimi) - Mention that ima_post_path_mknod() is now conditionally invoked when CONFIG_SECURITY_PATH=y (suggested by Mimi) - Mention when a LSM hook will be introduced in the IMA/EVM alignment patches (suggested by Mimi) - Simplify the commit messages when introducing a new LSM hook - Still keep the 'extern' in the function declaration, until the declaration is removed (suggested by Mimi) - Improve documentation of security_file_pre_free() - Register 'ima' and 'evm' as standalone LSMs (suggested by Paul) - Initialize the 'ima' and 'evm' LSMs from 'integrity', to keep the original ordering of IMA and EVM functions as when they were hardcoded - Return the IMA and EVM LSM IDs to 'integrity' for registration of the integrity-specific hooks - Reserve an xattr slot from the 'evm' LSM instead of 'integrity' - Pass the LSM ID to init_ima_appraise_lsm() v2: - Add description for newly introduced LSM hooks (suggested by Casey) - Clarify in the description of security_file_pre_free() that actions can be performed while the file is still open v1: - Drop 'evm: Complete description of evm_inode_setattr()', 'fs: Fix description of vfs_tmpfile()' and 'security: Introduce LSM_ORDER_LAST', they were sent separately (suggested by Christian Brauner) - Replace dentry with file descriptor parameter for security_inode_post_create_tmpfile() - Introduce mode_stripped and pass it as mode argument to security_path_mknod() and security_path_post_mknod() - Use goto in do_mknodat() and __vfs_removexattr_locked() (suggested by Mimi) - Replace __lsm_ro_after_init with __ro_after_init - Modify short description of security_inode_post_create_tmpfile() and security_inode_post_set_acl() (suggested by Stefan) - Move security_inode_post_setattr() just after security_inode_setattr() (suggested by Mimi) - Modify short description of security_key_post_create_or_update() (suggested by Mimi) - Add back exported functions ima_file_check() and evm_inode_init_security() respectively to ima.h and evm.h (reported by kernel robot) - Remove extern from prototype declarations and fix style issues - Remove unnecessary include of linux/lsm_hooks.h in ima_main.c and ima_appraise.c Roberto Sassu (24): ima: Align ima_inode_post_setattr() definition with LSM infrastructure ima: Align ima_file_mprotect() definition with LSM infrastructure ima: Align ima_inode_setxattr() definition with LSM infrastructure ima: Align ima_inode_removexattr() definition with LSM infrastructure ima: Align ima_post_read_file() definition with LSM infrastructure evm: Align evm_inode_post_setattr() definition with LSM infrastructure evm: Align evm_inode_setxattr() definition with LSM infrastructure evm: Align evm_inode_post_setxattr() definition with LSM infrastructure security: Align inode_setattr hook definition with EVM security: Introduce inode_post_setattr hook security: Introduce inode_post_removexattr hook security: Introduce file_post_open hook security: Introduce file_release hook security: Introduce path_post_mknod hook security: Introduce inode_post_create_tmpfile hook security: Introduce inode_post_set_acl hook security: Introduce inode_post_remove_acl hook security: Introduce key_post_create_or_update hook ima: Move to LSM infrastructure ima: Move IMA-Appraisal to LSM infrastructure evm: Move to LSM infrastructure evm: Make it independent from 'integrity' LSM ima: Make it independent from 'integrity' LSM integrity: Remove LSM fs/attr.c | 5 +- fs/file_table.c | 3 +- fs/namei.c | 12 +- fs/nfsd/vfs.c | 3 +- fs/open.c | 1 - fs/posix_acl.c | 5 +- fs/xattr.c | 9 +- include/linux/evm.h | 111 +------- include/linux/fs.h | 2 - include/linux/ima.h | 142 ---------- include/linux/integrity.h | 27 -- include/linux/lsm_hook_defs.h | 20 +- include/linux/security.h | 59 ++++ include/uapi/linux/lsm.h | 2 + security/integrity/Makefile | 1 + security/integrity/digsig_asymmetric.c | 23 -- security/integrity/evm/evm.h | 19 ++ security/integrity/evm/evm_crypto.c | 4 +- security/integrity/evm/evm_main.c | 195 ++++++++++--- security/integrity/iint.c | 197 +------------ security/integrity/ima/ima.h | 120 +++++++- security/integrity/ima/ima_api.c | 15 +- security/integrity/ima/ima_appraise.c | 64 +++-- security/integrity/ima/ima_init.c | 2 +- security/integrity/ima/ima_main.c | 201 +++++++++++--- security/integrity/ima/ima_policy.c | 2 +- security/integrity/integrity.h | 80 +----- security/keys/key.c | 10 +- security/security.c | 261 +++++++++++------- security/selinux/hooks.c | 3 +- security/smack/smack_lsm.c | 4 +- .../selftests/lsm/lsm_list_modules_test.c | 6 + 32 files changed, 783 insertions(+), 825 deletions(-)