On 1/15/2024 10:18 AM, Roberto Sassu wrote: > From: Roberto Sassu <[email protected]> > > Define a new structure for EVM-specific metadata, called evm_iint_cache, > and embed it in the inode security blob. Introduce evm_iint_inode() to > retrieve metadata, and register evm_inode_alloc_security() for the > inode_alloc_security LSM hook, to initialize the structure (before > splitting metadata, this task was done by iint_init_always()). > > Keep the non-NULL checks after calling evm_iint_inode() except in > evm_inode_alloc_security(), to take into account inodes for which > security_inode_alloc() was not called. When using shared metadata, > obtaining a NULL pointer from integrity_iint_find() meant that the file > wasn't in the IMA policy. Now, because IMA and EVM use disjoint metadata, > the EVM status has to be stored for every inode regardless of the IMA > policy. > > Given that from now on EVM relies on its own metadata, remove the iint > parameter from evm_verifyxattr(). Also, directly retrieve the iint in > evm_verify_hmac(), called by both evm_verifyxattr() and > evm_verify_current_integrity(), since now there is no performance penalty > in retrieving EVM metadata (constant time). > > Replicate the management of the IMA_NEW_FILE flag, by introducing > evm_post_path_mknod() and evm_file_release() to respectively set and clear > the newly introduced flag EVM_NEW_FILE, at the same time IMA does. Like for > IMA, select CONFIG_SECURITY_PATH when EVM is enabled, to ensure that files > are marked as new. > > Unlike ima_post_path_mknod(), evm_post_path_mknod() cannot check if a file > must be appraised. Thus, it marks all affected files. Also, it does not > clear EVM_NEW_FILE depending on i_version, but that is not a problem > because IMA_NEW_FILE is always cleared when set in ima_check_last_writer(). > > Move the EVM-specific flag EVM_IMMUTABLE_DIGSIG to > security/integrity/evm/evm.h, since that definition is now unnecessary in > the common integrity layer. > > Finally, switch to the LSM reservation mechanism for the EVM xattr, and > consequently decrement by one the number of xattrs to allocate in > security_inode_init_security(). > > Signed-off-by: Roberto Sassu <[email protected]>
Reviewed-by: Casey Schaufler <[email protected]> > --- > include/linux/evm.h | 8 +-- > security/integrity/evm/Kconfig | 1 + > security/integrity/evm/evm.h | 19 +++++++ > security/integrity/evm/evm_crypto.c | 4 +- > security/integrity/evm/evm_main.c | 76 ++++++++++++++++++++------- > security/integrity/ima/ima_appraise.c | 2 +- > security/integrity/integrity.h | 1 - > security/security.c | 4 +- > 8 files changed, 83 insertions(+), 32 deletions(-) > > diff --git a/include/linux/evm.h b/include/linux/evm.h > index cb481eccc967..d48d6da32315 100644 > --- a/include/linux/evm.h > +++ b/include/linux/evm.h > @@ -12,15 +12,12 @@ > #include <linux/integrity.h> > #include <linux/xattr.h> > > -struct integrity_iint_cache; > - > #ifdef CONFIG_EVM > extern int evm_set_key(void *key, size_t keylen); > extern enum integrity_status evm_verifyxattr(struct dentry *dentry, > const char *xattr_name, > void *xattr_value, > - size_t xattr_value_len, > - struct integrity_iint_cache *iint); > + size_t xattr_value_len); > int evm_inode_init_security(struct inode *inode, struct inode *dir, > const struct qstr *qstr, struct xattr *xattrs, > int *xattr_count); > @@ -48,8 +45,7 @@ static inline int evm_set_key(void *key, size_t keylen) > static inline enum integrity_status evm_verifyxattr(struct dentry *dentry, > const char *xattr_name, > void *xattr_value, > - size_t xattr_value_len, > - struct integrity_iint_cache *iint) > + size_t xattr_value_len) > { > return INTEGRITY_UNKNOWN; > } > diff --git a/security/integrity/evm/Kconfig b/security/integrity/evm/Kconfig > index fba9ee359bc9..861b3bacab82 100644 > --- a/security/integrity/evm/Kconfig > +++ b/security/integrity/evm/Kconfig > @@ -6,6 +6,7 @@ config EVM > select CRYPTO_HMAC > select CRYPTO_SHA1 > select CRYPTO_HASH_INFO > + select SECURITY_PATH > default n > help > EVM protects a file's security extended attributes against > diff --git a/security/integrity/evm/evm.h b/security/integrity/evm/evm.h > index 53bd7fec93fa..eb1a2c343bd7 100644 > --- a/security/integrity/evm/evm.h > +++ b/security/integrity/evm/evm.h > @@ -32,6 +32,25 @@ struct xattr_list { > bool enabled; > }; > > +#define EVM_NEW_FILE 0x00000001 > +#define EVM_IMMUTABLE_DIGSIG 0x00000002 > + > +/* EVM integrity metadata associated with an inode */ > +struct evm_iint_cache { > + unsigned long flags; > + enum integrity_status evm_status:4; > +}; > + > +extern struct lsm_blob_sizes evm_blob_sizes; > + > +static inline struct evm_iint_cache *evm_iint_inode(const struct inode > *inode) > +{ > + if (unlikely(!inode->i_security)) > + return NULL; > + > + return inode->i_security + evm_blob_sizes.lbs_inode; > +} > + > extern int evm_initialized; > > #define EVM_ATTR_FSUUID 0x0001 > diff --git a/security/integrity/evm/evm_crypto.c > b/security/integrity/evm/evm_crypto.c > index b1ffd4cc0b44..7552d49d0725 100644 > --- a/security/integrity/evm/evm_crypto.c > +++ b/security/integrity/evm/evm_crypto.c > @@ -322,10 +322,10 @@ int evm_calc_hash(struct dentry *dentry, const char > *req_xattr_name, > static int evm_is_immutable(struct dentry *dentry, struct inode *inode) > { > const struct evm_ima_xattr_data *xattr_data = NULL; > - struct integrity_iint_cache *iint; > + struct evm_iint_cache *iint; > int rc = 0; > > - iint = integrity_iint_find(inode); > + iint = evm_iint_inode(inode); > if (iint && (iint->flags & EVM_IMMUTABLE_DIGSIG)) > return 1; > > diff --git a/security/integrity/evm/evm_main.c > b/security/integrity/evm/evm_main.c > index f65dbf3e9256..14c8f38e61d6 100644 > --- a/security/integrity/evm/evm_main.c > +++ b/security/integrity/evm/evm_main.c > @@ -178,14 +178,14 @@ static int is_unsupported_fs(struct dentry *dentry) > static enum integrity_status evm_verify_hmac(struct dentry *dentry, > const char *xattr_name, > char *xattr_value, > - size_t xattr_value_len, > - struct integrity_iint_cache *iint) > + size_t xattr_value_len) > { > struct evm_ima_xattr_data *xattr_data = NULL; > struct signature_v2_hdr *hdr; > enum integrity_status evm_status = INTEGRITY_PASS; > struct evm_digest digest; > - struct inode *inode; > + struct inode *inode = d_backing_inode(dentry); > + struct evm_iint_cache *iint = evm_iint_inode(inode); > int rc, xattr_len, evm_immutable = 0; > > if (iint && (iint->evm_status == INTEGRITY_PASS || > @@ -254,8 +254,6 @@ static enum integrity_status evm_verify_hmac(struct > dentry *dentry, > (const char *)xattr_data, xattr_len, > digest.digest, digest.hdr.length); > if (!rc) { > - inode = d_backing_inode(dentry); > - > if (xattr_data->type == EVM_XATTR_PORTABLE_DIGSIG) { > if (iint) > iint->flags |= EVM_IMMUTABLE_DIGSIG; > @@ -403,7 +401,6 @@ int evm_read_protected_xattrs(struct dentry *dentry, u8 > *buffer, > * @xattr_name: requested xattr > * @xattr_value: requested xattr value > * @xattr_value_len: requested xattr value length > - * @iint: inode integrity metadata > * > * Calculate the HMAC for the given dentry and verify it against the stored > * security.evm xattr. For performance, use the xattr value and length > @@ -416,8 +413,7 @@ int evm_read_protected_xattrs(struct dentry *dentry, u8 > *buffer, > */ > enum integrity_status evm_verifyxattr(struct dentry *dentry, > const char *xattr_name, > - void *xattr_value, size_t xattr_value_len, > - struct integrity_iint_cache *iint) > + void *xattr_value, size_t xattr_value_len) > { > if (!evm_key_loaded() || !evm_protected_xattr(xattr_name)) > return INTEGRITY_UNKNOWN; > @@ -425,13 +421,8 @@ enum integrity_status evm_verifyxattr(struct dentry > *dentry, > if (is_unsupported_fs(dentry)) > return INTEGRITY_UNKNOWN; > > - if (!iint) { > - iint = integrity_iint_find(d_backing_inode(dentry)); > - if (!iint) > - return INTEGRITY_UNKNOWN; > - } > return evm_verify_hmac(dentry, xattr_name, xattr_value, > - xattr_value_len, iint); > + xattr_value_len); > } > EXPORT_SYMBOL_GPL(evm_verifyxattr); > > @@ -448,7 +439,7 @@ static enum integrity_status > evm_verify_current_integrity(struct dentry *dentry) > > if (!evm_key_loaded() || !S_ISREG(inode->i_mode) || evm_fixmode) > return INTEGRITY_PASS; > - return evm_verify_hmac(dentry, NULL, NULL, 0, NULL); > + return evm_verify_hmac(dentry, NULL, NULL, 0); > } > > /* > @@ -526,14 +517,14 @@ static int evm_protect_xattr(struct mnt_idmap *idmap, > > evm_status = evm_verify_current_integrity(dentry); > if (evm_status == INTEGRITY_NOXATTRS) { > - struct integrity_iint_cache *iint; > + struct evm_iint_cache *iint; > > /* Exception if the HMAC is not going to be calculated. */ > if (evm_hmac_disabled()) > return 0; > > - iint = integrity_iint_find(d_backing_inode(dentry)); > - if (iint && (iint->flags & IMA_NEW_FILE)) > + iint = evm_iint_inode(d_backing_inode(dentry)); > + if (iint && (iint->flags & EVM_NEW_FILE)) > return 0; > > /* exception for pseudo filesystems */ > @@ -735,9 +726,9 @@ static int evm_inode_remove_acl(struct mnt_idmap *idmap, > struct dentry *dentry, > > static void evm_reset_status(struct inode *inode) > { > - struct integrity_iint_cache *iint; > + struct evm_iint_cache *iint; > > - iint = integrity_iint_find(inode); > + iint = evm_iint_inode(inode); > if (iint) > iint->evm_status = INTEGRITY_UNKNOWN; > } > @@ -1019,6 +1010,42 @@ int evm_inode_init_security(struct inode *inode, > struct inode *dir, > } > EXPORT_SYMBOL_GPL(evm_inode_init_security); > > +static int evm_inode_alloc_security(struct inode *inode) > +{ > + struct evm_iint_cache *iint = evm_iint_inode(inode); > + > + /* Called by security_inode_alloc(), it cannot be NULL. */ > + iint->flags = 0UL; > + iint->evm_status = INTEGRITY_UNKNOWN; > + > + return 0; > +} > + > +static void evm_file_release(struct file *file) > +{ > + struct inode *inode = file_inode(file); > + struct evm_iint_cache *iint = evm_iint_inode(inode); > + fmode_t mode = file->f_mode; > + > + if (!S_ISREG(inode->i_mode) || !(mode & FMODE_WRITE)) > + return; > + > + if (iint && atomic_read(&inode->i_writecount) == 1) > + iint->flags &= ~EVM_NEW_FILE; > +} > + > +static void evm_post_path_mknod(struct mnt_idmap *idmap, struct dentry > *dentry) > +{ > + struct inode *inode = d_backing_inode(dentry); > + struct evm_iint_cache *iint = evm_iint_inode(inode); > + > + if (!S_ISREG(inode->i_mode)) > + return; > + > + if (iint) > + iint->flags |= EVM_NEW_FILE; > +} > + > #ifdef CONFIG_EVM_LOAD_X509 > void __init evm_load_x509(void) > { > @@ -1071,6 +1098,9 @@ static struct security_hook_list evm_hooks[] > __ro_after_init = { > 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), > + LSM_HOOK_INIT(inode_alloc_security, evm_inode_alloc_security), > + LSM_HOOK_INIT(file_release, evm_file_release), > + LSM_HOOK_INIT(path_post_mknod, evm_post_path_mknod), > }; > > static const struct lsm_id evm_lsmid = { > @@ -1084,10 +1114,16 @@ static int __init init_evm_lsm(void) > return 0; > } > > +struct lsm_blob_sizes evm_blob_sizes __ro_after_init = { > + .lbs_inode = sizeof(struct evm_iint_cache), > + .lbs_xattr_count = 1, > +}; > + > DEFINE_LSM(evm) = { > .name = "evm", > .init = init_evm_lsm, > .order = LSM_ORDER_LAST, > + .blobs = &evm_blob_sizes, > }; > > late_initcall(init_evm); > diff --git a/security/integrity/ima/ima_appraise.c > b/security/integrity/ima/ima_appraise.c > index 076451109637..1dd6ee72a20a 100644 > --- a/security/integrity/ima/ima_appraise.c > +++ b/security/integrity/ima/ima_appraise.c > @@ -520,7 +520,7 @@ int ima_appraise_measurement(enum ima_hooks func, > } > > status = evm_verifyxattr(dentry, XATTR_NAME_IMA, xattr_value, > - rc < 0 ? 0 : rc, iint); > + rc < 0 ? 0 : rc); > switch (status) { > case INTEGRITY_PASS: > case INTEGRITY_PASS_IMMUTABLE: > diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h > index 59eaddd84434..7a97c269a072 100644 > --- a/security/integrity/integrity.h > +++ b/security/integrity/integrity.h > @@ -37,7 +37,6 @@ > #define IMA_DIGSIG_REQUIRED 0x01000000 > #define IMA_PERMIT_DIRECTIO 0x02000000 > #define IMA_NEW_FILE 0x04000000 > -#define EVM_IMMUTABLE_DIGSIG 0x08000000 > #define IMA_FAIL_UNVERIFIABLE_SIGS 0x10000000 > #define IMA_MODSIG_ALLOWED 0x20000000 > #define IMA_CHECK_BLACKLIST 0x40000000 > diff --git a/security/security.c b/security/security.c > index a44740640a9a..f811cc376a7a 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -1716,8 +1716,8 @@ int security_inode_init_security(struct inode *inode, > struct inode *dir, > return 0; > > if (initxattrs) { > - /* Allocate +1 for EVM and +1 as terminator. */ > - new_xattrs = kcalloc(blob_sizes.lbs_xattr_count + 2, > + /* Allocate +1 as terminator. */ > + new_xattrs = kcalloc(blob_sizes.lbs_xattr_count + 1, > sizeof(*new_xattrs), GFP_NOFS); > if (!new_xattrs) > return -ENOMEM;
