diff mbox series

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

Message ID 20231130231948.2545638-5-roberto.sassu@huaweicloud.com (mailing list archive)
State Handled Elsewhere
Delegated to: Paul Moore
Headers show
Series None | expand

Commit Message

Roberto Sassu Nov. 30, 2023, 11:19 p.m. UTC
From: Roberto Sassu <roberto.sassu@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.

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@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(-)

Comments

Roberto Sassu Dec. 1, 2023, midnight UTC | #1
On 12/1/2023 12:19 AM, Roberto Sassu wrote:
> From: Roberto Sassu <roberto.sassu@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@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;
>
diff mbox series

Patch

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;