Received-SPF: pass (google.com: domain of linux-nfs+bounces-1142-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <3e2d5c73-0c77-4b46-a879-c92116eb5ad9@schaufler-ca.com>
Date: Tue, 16 Jan 2024 11:41:12 -0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v9 25/25] integrity: Remove LSM
Content-Language: en-US
To: Roberto Sassu <roberto.sassu@huaweicloud.com>, viro@zeniv.linux.org.uk,
 brauner@kernel.org, chuck.lever@oracle.com, jlayton@kernel.org,
 neilb@suse.de, kolga@netapp.com, Dai.Ngo@oracle.com, tom@talpey.com,
 paul@paul-moore.com, jmorris@namei.org, serge@hallyn.com,
 zohar@linux.ibm.com, dmitry.kasatkin@gmail.com, eric.snowberg@oracle.com,
 dhowells@redhat.com, jarkko@kernel.org, stephen.smalley.work@gmail.com,
 eparis@parisplace.org, shuah@kernel.org, mic@digikod.net
Cc: linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
 linux-nfs@vger.kernel.org, linux-security-module@vger.kernel.org,
 linux-integrity@vger.kernel.org, keyrings@vger.kernel.org,
 selinux@vger.kernel.org, linux-kselftest@vger.kernel.org,
 Roberto Sassu <roberto.sassu@huawei.com>,
 Casey Schaufler <casey@schaufler-ca.com>
References: <20240115181809.885385-1-roberto.sassu@huaweicloud.com>
 <20240115181809.885385-26-roberto.sassu@huaweicloud.com>
From: Casey Schaufler <casey@schaufler-ca.com>
In-Reply-To: <20240115181809.885385-26-roberto.sassu@huaweicloud.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

On 1/15/2024 10:18 AM, Roberto Sassu wrote:
> From: Roberto Sassu <roberto.sassu@huawei.com>
>
> Since now IMA and EVM use their own integrity metadata, it is safe to
> remove the 'integrity' LSM, with its management of integrity metadata.
>
> Keep the iint.c file only for loading IMA and EVM keys at boot, and for
> creating the integrity directory in securityfs (we need to keep it for
> retrocompatibility reasons).
>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>

Reviewed-by: Casey Schaufler <casey@schaufler-ca.com>

> ---
>  include/linux/integrity.h      |  14 ---
>  security/integrity/iint.c      | 197 +--------------------------------
>  security/integrity/integrity.h |  25 -----
>  security/security.c            |   2 -
>  4 files changed, 2 insertions(+), 236 deletions(-)
>
> diff --git a/include/linux/integrity.h b/include/linux/integrity.h
> index ef0f63ef5ebc..459b79683783 100644
> --- a/include/linux/integrity.h
> +++ b/include/linux/integrity.h
> @@ -19,24 +19,10 @@ enum integrity_status {
>  	INTEGRITY_UNKNOWN,
>  };
>  
> -/* List of EVM protected security xattrs */
>  #ifdef CONFIG_INTEGRITY
> -extern struct integrity_iint_cache *integrity_inode_get(struct inode *inode);
> -extern void integrity_inode_free(struct inode *inode);
>  extern void __init integrity_load_keys(void);
>  
>  #else
> -static inline struct integrity_iint_cache *
> -				integrity_inode_get(struct inode *inode)
> -{
> -	return NULL;
> -}
> -
> -static inline void integrity_inode_free(struct inode *inode)
> -{
> -	return;
> -}
> -
>  static inline void integrity_load_keys(void)
>  {
>  }
> diff --git a/security/integrity/iint.c b/security/integrity/iint.c
> index d4419a2a1e24..068ac6c2ae1e 100644
> --- a/security/integrity/iint.c
> +++ b/security/integrity/iint.c
> @@ -6,207 +6,14 @@
>   * Mimi Zohar <zohar@us.ibm.com>
>   *
>   * File: integrity_iint.c
> - *	- implements the integrity hooks: integrity_inode_alloc,
> - *	  integrity_inode_free
> - *	- cache integrity information associated with an inode
> - *	  using a rbtree tree.
> + *	- initialize the integrity directory in securityfs
> + *	- load IMA and EVM keys
>   */
> -#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;
> -}
> -
> -#define IMA_MAX_NESTING (FILESYSTEM_MAX_STACK_DEPTH+1)
> -
> -/*
> - * It is not clear that IMA should be nested at all, but as long is it measures
> - * files both on overlayfs and on underlying fs, we need to annotate the iint
> - * mutex to avoid lockdep false positives related to IMA + overlayfs.
> - * See ovl_lockdep_annotate_inode_mutex_key() for more details.
> - */
> -static inline void iint_lockdep_annotate(struct integrity_iint_cache *iint,
> -					 struct inode *inode)
> -{
> -#ifdef CONFIG_LOCKDEP
> -	static struct lock_class_key iint_mutex_key[IMA_MAX_NESTING];
> -
> -	int depth = inode->i_sb->s_stack_depth;
> -
> -	if (WARN_ON_ONCE(depth < 0 || depth >= IMA_MAX_NESTING))
> -		depth = 0;
> -
> -	lockdep_set_class(&iint->mutex, &iint_mutex_key[depth]);
> -#endif
> -}
> -
> -static void iint_init_always(struct integrity_iint_cache *iint,
> -			     struct inode *inode)
> -{
> -	iint->ima_hash = NULL;
> -	iint->version = 0;
> -	iint->flags = 0UL;
> -	iint->atomic_flags = 0UL;
> -	iint->ima_file_status = INTEGRITY_UNKNOWN;
> -	iint->ima_mmap_status = INTEGRITY_UNKNOWN;
> -	iint->ima_bprm_status = INTEGRITY_UNKNOWN;
> -	iint->ima_read_status = INTEGRITY_UNKNOWN;
> -	iint->ima_creds_status = INTEGRITY_UNKNOWN;
> -	iint->evm_status = INTEGRITY_UNKNOWN;
> -	iint->measured_pcrs = 0;
> -	mutex_init(&iint->mutex);
> -	iint_lockdep_annotate(iint, inode);
> -}
> -
> -static void iint_free(struct integrity_iint_cache *iint)
> -{
> -	kfree(iint->ima_hash);
> -	mutex_destroy(&iint->mutex);
> -	kmem_cache_free(iint_cache, iint);
> -}
> -
> -/**
> - * integrity_inode_get - find or allocate an iint associated with an inode
> - * @inode: pointer to the inode
> - * @return: allocated iint
> - *
> - * Caller must lock i_mutex
> - */
> -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;
> -
> -	iint = integrity_iint_find(inode);
> -	if (iint)
> -		return iint;
> -
> -	iint = kmem_cache_alloc(iint_cache, GFP_NOFS);
> -	if (!iint)
> -		return NULL;
> -
> -	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);
> -
> -	write_unlock(&integrity_iint_lock);
> -	return iint;
> -}
> -
> -/**
> - * integrity_inode_free - called on security_inode_free
> - * @inode: pointer to the inode
> - *
> - * Free the integrity information(iint) associated with an inode.
> - */
> -void integrity_inode_free(struct inode *inode)
> -{
> -	struct integrity_iint_cache *iint;
> -
> -	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_free(iint);
> -}
> -
> -static void iint_init_once(void *foo)
> -{
> -	struct integrity_iint_cache *iint = (struct integrity_iint_cache *) foo;
> -
> -	memset(iint, 0, sizeof(*iint));
> -}
> -
> -static int __init integrity_iintcache_init(void)
> -{
> -	iint_cache =
> -	    kmem_cache_create("iint_cache", sizeof(struct integrity_iint_cache),
> -			      0, SLAB_PANIC, iint_init_once);
> -	return 0;
> -}
> -DEFINE_LSM(integrity) = {
> -	.name = "integrity",
> -	.init = integrity_iintcache_init,
> -	.order = LSM_ORDER_LAST,
> -};
> -
> -
>  /*
>   * integrity_kernel_read - read data from the file
>   *
> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
> index 671fc50255f9..50d6f798e613 100644
> --- a/security/integrity/integrity.h
> +++ b/security/integrity/integrity.h
> @@ -102,31 +102,6 @@ struct ima_file_id {
>  	__u8 hash[HASH_MAX_DIGESTSIZE];
>  } __packed;
>  
> -/* 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 */
> -	unsigned long flags;
> -	unsigned long measured_pcrs;
> -	unsigned long atomic_flags;
> -	unsigned long real_ino;
> -	dev_t real_dev;
> -	enum integrity_status ima_file_status:4;
> -	enum integrity_status ima_mmap_status:4;
> -	enum integrity_status ima_bprm_status:4;
> -	enum integrity_status ima_read_status:4;
> -	enum integrity_status ima_creds_status:4;
> -	enum integrity_status evm_status:4;
> -	struct ima_digest_data *ima_hash;
> -};
> -
> -/* rbtree tree calls to lookup, insert, delete
> - * integrity data associated with an inode.
> - */
> -struct integrity_iint_cache *integrity_iint_find(struct inode *inode);
> -
>  int integrity_kernel_read(struct file *file, loff_t offset,
>  			  void *addr, unsigned long count);
>  
> diff --git a/security/security.c b/security/security.c
> index f811cc376a7a..df87c0a7eaac 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -19,7 +19,6 @@
>  #include <linux/kernel.h>
>  #include <linux/kernel_read_file.h>
>  #include <linux/lsm_hooks.h>
> -#include <linux/integrity.h>
>  #include <linux/fsnotify.h>
>  #include <linux/mman.h>
>  #include <linux/mount.h>
> @@ -1597,7 +1596,6 @@ static void inode_free_by_rcu(struct rcu_head *head)
>   */
>  void security_inode_free(struct inode *inode)
>  {
> -	integrity_inode_free(inode);
>  	call_void_hook(inode_free_security, inode);
>  	/*
>  	 * The inode may still be referenced in a path walk and