Re: [PATCH v7 23/23] integrity: Switch from rbtree to LSM-managed blob for integrity_iint_cache

From: Roberto Sassu
Date: Thu Nov 30 2023 - 19:01:16 EST

On 12/1/2023 12:19 AM, Roberto Sassu wrote:
From: Roberto Sassu <roberto.sassu@xxxxxxxxxx>

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