Message-ID: <3e2d5c73-0c77-4b46-a879-c92116eb5ad9@schaufler-ca.com>
Date: Tue, 16 Jan 2024 11:41:12 -0800
From: Casey Schaufler <casey@...aufler-ca.com>
To: Roberto Sassu <roberto.sassu@...weicloud.com>, viro@...iv.linux.org.uk,
 brauner@...nel.org, chuck.lever@...cle.com, jlayton@...nel.org,
 neilb@...e.de, kolga@...app.com, Dai.Ngo@...cle.com, tom@...pey.com,
 paul@...l-moore.com, jmorris@...ei.org, serge@...lyn.com,
 zohar@...ux.ibm.com, dmitry.kasatkin@...il.com, eric.snowberg@...cle.com,
 dhowells@...hat.com, jarkko@...nel.org, stephen.smalley.work@...il.com,
 eparis@...isplace.org, shuah@...nel.org, mic@...ikod.net
Cc: linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
 linux-nfs@...r.kernel.org, linux-security-module@...r.kernel.org,
 linux-integrity@...r.kernel.org, keyrings@...r.kernel.org,
 selinux@...r.kernel.org, linux-kselftest@...r.kernel.org,
 Roberto Sassu <roberto.sassu@...wei.com>,
 Casey Schaufler <casey@...aufler-ca.com>
Subject: Re: [PATCH v9 25/25] integrity: Remove LSM

On 1/15/2024 10:18 AM, Roberto Sassu wrote:
> From: Roberto Sassu <roberto.sassu@...wei.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@...wei.com>

Reviewed-by: Casey Schaufler <casey@...aufler-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@...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

Powered by blists - more mailing lists