Re: [PATCH v9 23/25] evm: Make it independent from 'integrity' LSM

From: Casey Schaufler
Date: Tue Jan 16 2024 - 14:39:27 EST

Next message: Rob Herring: "Re: [PATCH 2/3] dt-bindings: rockchip: rk809 fix compatible string in examples"
Previous message: Tomer Maimon: "Re: [PATCH v22 6/8] dt-bindings: clock: npcm845: replace reg with syscon property"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 1/15/2024 10:18 AM, Roberto Sassu wrote:
> From: Roberto Sassu <roberto.sassu@xxxxxxxxxx>
>
> 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 <roberto.sassu@xxxxxxxxxx>

Reviewed-by: Casey Schaufler <casey@xxxxxxxxxxxxxxxx>

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

Next message: Rob Herring: "Re: [PATCH 2/3] dt-bindings: rockchip: rk809 fix compatible string in examples"
Previous message: Tomer Maimon: "Re: [PATCH v22 6/8] dt-bindings: clock: npcm845: replace reg with syscon property"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]