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

[Casey Schaufler]

On 8/31/2023 4:38 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. Thus, request from the 'integrity' LSM a
> space in the security blob for the pointer of inode metadata
> (integrity_iint_cache structure).
>
> Prefer this to allocating the integrity_iint_cache structure directly, as
> IMA would require it only for a subset of inodes. Always allocating it
> would cause a waste of memory.
>
> 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.
>
> Signed-off-by: Roberto Sassu <roberto.sassu@xxxxxxxxxx>

Reviewed-by: Casey Schaufler <casey@xxxxxxxxxxxxxxxx>

> ---
>  security/integrity/iint.c      | 67 +++-------------------------------
>  security/integrity/integrity.h | 19 +++++++++-
>  2 files changed, 24 insertions(+), 62 deletions(-)
>
> diff --git a/security/integrity/iint.c b/security/integrity/iint.c
> index 70ee803a33ea..c2fba8afbbdb 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 __read_mostly;
>  
>  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);
>  }
>  
>  static void iint_free(struct integrity_iint_cache *iint)
> @@ -92,9 +61,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;
>  
>  	iint = integrity_iint_find(inode);
>  	if (iint)
> @@ -104,31 +71,10 @@ struct integrity_iint_cache *integrity_inode_get(struct inode *inode)
>  	if (!iint)
>  		return NULL;
>  
> -	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;
>  }
>  
> @@ -145,10 +91,8 @@ static 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);
>  }
> @@ -188,6 +132,7 @@ static int __init integrity_lsm_init(void)
>  }
>  
>  struct lsm_blob_sizes integrity_blob_sizes __ro_after_init = {
> +	.lbs_inode = sizeof(struct integrity_iint_cache *),
>  	.lbs_xattr_count = 1,
>  };
>  
> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
> index e020c365997b..24de4ad4a37e 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 */
> @@ -192,6 +191,24 @@ int integrity_kernel_read(struct file *file, loff_t offset,
>  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;
>  
>  #ifdef CONFIG_IMA