diff mbox series

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

Message ID 20230904134049.1802006-6-roberto.sassu@huaweicloud.com (mailing list archive)
State New, archived
Headers show
Series security: Move IMA and EVM to the LSM infrastructure | expand

Commit Message

Roberto Sassu Sept. 4, 2023, 1:40 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. 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@huawei.com>
Reviewed-by: Casey Schaufler <casey@schaufler-ca.com>
---
 security/integrity/iint.c      | 67 +++-------------------------------
 security/integrity/integrity.h | 19 +++++++++-
 2 files changed, 24 insertions(+), 62 deletions(-)

Comments

Stefan Berger Sept. 12, 2023, 4:19 p.m. UTC | #1 
On 9/4/23 09:40, 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. 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@huawei.com>
> Reviewed-by: Casey Schaufler <casey@schaufler-ca.com>
> ---
>
> @@ -145,10 +91,8 @@ static void integrity_inode_free(struct inode *inode)
>   	if (!IS_IMA(inode))
>   		return;

I think you can remove this check !IS_IMA()  as well since the next 
function called here integrity_iint_find() already has this check:

struct integrity_iint_cache *integrity_iint_find(struct inode *inode)
{
         if (!IS_IMA(inode))
                 return NULL;

         return integrity_inode_get_iint(inode);
}


>   
> -	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);
>   }
Roberto Sassu Sept. 15, 2023, 9:39 a.m. UTC | #2 
On Tue, 2023-09-12 at 12:19 -0400, Stefan Berger wrote:
> On 9/4/23 09:40, 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. 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@huawei.com>
> > Reviewed-by: Casey Schaufler <casey@schaufler-ca.com>
> > ---
> > 
> > @@ -145,10 +91,8 @@ static void integrity_inode_free(struct inode *inode)
> >   	if (!IS_IMA(inode))
> >   		return;
> 
> I think you can remove this check !IS_IMA()  as well since the next 
> function called here integrity_iint_find() already has this check:
> 
> struct integrity_iint_cache *integrity_iint_find(struct inode *inode)
> {
>          if (!IS_IMA(inode))
>                  return NULL;
> 
>          return integrity_inode_get_iint(inode);
> }

I agree, thanks!

Roberto

> >   
> > -	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);
> >   }
Roberto Sassu Oct. 13, 2023, 11:31 a.m. UTC | #3 
On Fri, 2023-09-15 at 11:39 +0200, Roberto Sassu wrote:
> On Tue, 2023-09-12 at 12:19 -0400, Stefan Berger wrote:
> > On 9/4/23 09:40, 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. 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@huawei.com>
> > > Reviewed-by: Casey Schaufler <casey@schaufler-ca.com>
> > > ---
> > > 
> > > @@ -145,10 +91,8 @@ static void integrity_inode_free(struct inode *inode)
> > >   	if (!IS_IMA(inode))
> > >   		return;
> > 
> > I think you can remove this check !IS_IMA()  as well since the next 
> > function called here integrity_iint_find() already has this check:
> > 
> > struct integrity_iint_cache *integrity_iint_find(struct inode *inode)
> > {
> >          if (!IS_IMA(inode))
> >                  return NULL;
> > 
> >          return integrity_inode_get_iint(inode);
> > }
> 
> I agree, thanks!

Actually, I had to keep it otherwise, without a check on iint,
iint_free() can get NULL as argument.

Roberto

> Roberto
> 
> > >   
> > > -	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);
> > >   }
>
diff mbox series

Patch

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