[PATCH v7 23/23] integrity: Switch from rbtree to LSM-managed blob for integrity_iint_cache
Roberto Sassu
roberto.sassu at huaweicloud.com
Fri Dec 1 00:00:48 UTC 2023
On 12/1/2023 12:19 AM, Roberto Sassu wrote:
> From: Roberto Sassu <roberto.sassu at huawei.com>
>
> Before the security field of kernel objects could be shared among LSMs with
> the LSM stacking feature, IMA and EVM had to rely on an alternative storage
> of inode metadata. The association between inode metadata and inode is
> maintained through an rbtree.
>
> With the reservation mechanism offered by the LSM infrastructure, the
> rbtree is no longer necessary, as each LSM could reserve a space in the
> security blob for each inode.
>
> With the 'integrity' LSM removed, and with the 'ima' LSM taking its role,
> reserve space from the 'ima' LSM for a pointer to the integrity_iint_cache
> structure directly, rather than embedding the whole structure in the inode
> security blob, to minimize the changes and to avoid waste of memory.
>
> If IMA is disabled, EVM faces the same problems as before making it an
> LSM, metadata verification fails for new files due to not setting the
> IMA_NEW_FILE flag in ima_post_path_mknod(), and evm_verifyxattr()
> returns INTEGRITY_UNKNOWN since IMA didn't call integrity_inode_get().
>
> The only difference caused to moving the integrity metadata management
> to the 'ima' LSM is the fact that EVM cannot take advantage of cached
> verification results, and has to do the verification again. However,
> this case should never happen, since the only public API available to
> all kernel components, evm_verifyxattr(), does not work if IMA is
> disabled.
This needs some explanation on how EVM can be used currently. EVM
verifies inode metadata in the set* LSM hooks, eventually updates the
HMAC in the post_set* hooks.
If integrity metadata are not available (IMA is disabled), EVM has to do
the inode metadata verification every time, which means that this patch
set would introduce a performance regression compared to when integrity
metadata were always available and managed by the 'integrity' LSM.
However, the get* LSM hooks are not mediated, user space can freely get
a corrupted inode metadata and EVM would not tell anything.
So, at this point it is clear that the main use case of EVM was a kernel
component querying EVM about the integrity of inode metadata, by calling
evm_verifyxattr(). One suitable place where this function can be called
is the d_instantiate LSM hook, when an LSM is getting xattrs from the
filesystem to populate the inode security blob.
But as I mentioned, evm_verifyxattr() does not work if IMA is disabled,
so there should not be systems using this configuration for which we are
introducing a performance regression.
Roberto
> Introduce two primitives for getting and setting the pointer of
> integrity_iint_cache in the security blob, respectively
> integrity_inode_get_iint() and integrity_inode_set_iint(). This would make
> the code more understandable, as they directly replace rbtree operations.
>
> Locking is not needed, as access to inode metadata is not shared, it is per
> inode.
>
> Keep the blob size and the new primitives definition at the common level in
> security/integrity rather than moving them in IMA itself, so that EVM can
> still call integrity_inode_get() and integrity_iint_find() while IMA is
> disabled. Just add an extra check in integrity_inode_get() to return NULL
> if that is the case.
>
> Signed-off-by: Roberto Sassu <roberto.sassu at huawei.com>
> ---
> security/integrity/iint.c | 70 ++++---------------------------
> security/integrity/ima/ima_main.c | 1 +
> security/integrity/integrity.h | 20 ++++++++-
> 3 files changed, 29 insertions(+), 62 deletions(-)
>
> diff --git a/security/integrity/iint.c b/security/integrity/iint.c
> index c36054041b84..8fc9455dda11 100644
> --- a/security/integrity/iint.c
> +++ b/security/integrity/iint.c
> @@ -14,56 +14,25 @@
> #include <linux/slab.h>
> #include <linux/init.h>
> #include <linux/spinlock.h>
> -#include <linux/rbtree.h>
> #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;
> -static DEFINE_RWLOCK(integrity_iint_lock);
> static struct kmem_cache *iint_cache __ro_after_init;
>
> struct dentry *integrity_dir;
>
> -/*
> - * __integrity_iint_find - return the iint associated with an inode
> - */
> -static struct integrity_iint_cache *__integrity_iint_find(struct inode *inode)
> -{
> - struct integrity_iint_cache *iint;
> - struct rb_node *n = integrity_iint_tree.rb_node;
> -
> - while (n) {
> - iint = rb_entry(n, struct integrity_iint_cache, rb_node);
> -
> - if (inode < iint->inode)
> - n = n->rb_left;
> - else if (inode > iint->inode)
> - n = n->rb_right;
> - else
> - return iint;
> - }
> -
> - return NULL;
> -}
> -
> /*
> * integrity_iint_find - return the iint associated with an inode
> */
> struct integrity_iint_cache *integrity_iint_find(struct inode *inode)
> {
> - struct integrity_iint_cache *iint;
> -
> if (!IS_IMA(inode))
> return NULL;
>
> - read_lock(&integrity_iint_lock);
> - iint = __integrity_iint_find(inode);
> - read_unlock(&integrity_iint_lock);
> -
> - return iint;
> + return integrity_inode_get_iint(inode);
> }
>
> #define IMA_MAX_NESTING (FILESYSTEM_MAX_STACK_DEPTH+1)
> @@ -123,9 +92,7 @@ static void iint_free(struct integrity_iint_cache *iint)
> */
> struct integrity_iint_cache *integrity_inode_get(struct inode *inode)
> {
> - struct rb_node **p;
> - struct rb_node *node, *parent = NULL;
> - struct integrity_iint_cache *iint, *test_iint;
> + struct integrity_iint_cache *iint;
>
> /*
> * After removing the 'integrity' LSM, the 'ima' LSM calls
> @@ -144,31 +111,10 @@ struct integrity_iint_cache *integrity_inode_get(struct inode *inode)
>
> iint_init_always(iint, inode);
>
> - write_lock(&integrity_iint_lock);
> -
> - p = &integrity_iint_tree.rb_node;
> - while (*p) {
> - parent = *p;
> - test_iint = rb_entry(parent, struct integrity_iint_cache,
> - rb_node);
> - if (inode < test_iint->inode) {
> - p = &(*p)->rb_left;
> - } else if (inode > test_iint->inode) {
> - p = &(*p)->rb_right;
> - } else {
> - write_unlock(&integrity_iint_lock);
> - kmem_cache_free(iint_cache, iint);
> - return test_iint;
> - }
> - }
> -
> iint->inode = inode;
> - node = &iint->rb_node;
> inode->i_flags |= S_IMA;
> - rb_link_node(node, parent, p);
> - rb_insert_color(node, &integrity_iint_tree);
> + integrity_inode_set_iint(inode, iint);
>
> - write_unlock(&integrity_iint_lock);
> return iint;
> }
>
> @@ -185,10 +131,8 @@ void integrity_inode_free(struct inode *inode)
> if (!IS_IMA(inode))
> return;
>
> - write_lock(&integrity_iint_lock);
> - iint = __integrity_iint_find(inode);
> - rb_erase(&iint->rb_node, &integrity_iint_tree);
> - write_unlock(&integrity_iint_lock);
> + iint = integrity_iint_find(inode);
> + integrity_inode_set_iint(inode, NULL);
>
> iint_free(iint);
> }
> @@ -212,6 +156,10 @@ int __init integrity_iintcache_init(void)
> return 0;
> }
>
> +struct lsm_blob_sizes integrity_blob_sizes __ro_after_init = {
> + .lbs_inode = sizeof(struct integrity_iint_cache *),
> +};
> +
> /*
> * 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 3f59cce3fa02..52b4a3bba45a 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -1162,6 +1162,7 @@ DEFINE_LSM(ima) = {
> .name = "ima",
> .init = init_ima_lsm,
> .order = LSM_ORDER_LAST,
> + .blobs = &integrity_blob_sizes,
> };
>
> late_initcall(init_ima); /* Start IMA after the TPM is available */
> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
> index 26d3b08dca1c..2fb35c67d64d 100644
> --- a/security/integrity/integrity.h
> +++ b/security/integrity/integrity.h
> @@ -158,7 +158,6 @@ struct ima_file_id {
>
> /* integrity data associated with an inode */
> struct integrity_iint_cache {
> - struct rb_node rb_node; /* rooted in integrity_iint_tree */
> struct mutex mutex; /* protects: version, flags, digest */
> struct inode *inode; /* back pointer to inode in question */
> u64 version; /* track inode changes */
> @@ -194,6 +193,25 @@ int integrity_kernel_read(struct file *file, loff_t offset,
> #define INTEGRITY_KEYRING_MAX 4
>
> extern struct dentry *integrity_dir;
> +extern struct lsm_blob_sizes integrity_blob_sizes;
> +
> +static inline struct integrity_iint_cache *
> +integrity_inode_get_iint(const struct inode *inode)
> +{
> + struct integrity_iint_cache **iint_sec;
> +
> + iint_sec = inode->i_security + integrity_blob_sizes.lbs_inode;
> + return *iint_sec;
> +}
> +
> +static inline void integrity_inode_set_iint(const struct inode *inode,
> + struct integrity_iint_cache *iint)
> +{
> + struct integrity_iint_cache **iint_sec;
> +
> + iint_sec = inode->i_security + integrity_blob_sizes.lbs_inode;
> + *iint_sec = iint;
> +}
>
> struct modsig;
>
More information about the Linux-security-module-archive
mailing list