Re: [PATCH v9 25/25] integrity: Remove LSM

From: Casey Schaufler
Date: Tue Jan 16 2024 - 14:41:35 EST

Next message: Sasha Levin: "[PATCH AUTOSEL 6.7 001/108] wifi: rtw89: fix timeout calculation in rtw89_roc_end()"
Previous message: Christophe JAILLET: "Re: [PATCH] seq_buf: make DECLARE_SEQ_BUF() usable"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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

Reviewed-by: Casey Schaufler <casey@xxxxxxxxxxxxxxxx>

> ---
> 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@xxxxxxxxxx>
> *
> * 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

Next message: Sasha Levin: "[PATCH AUTOSEL 6.7 001/108] wifi: rtw89: fix timeout calculation in rtw89_roc_end()"
Previous message: Christophe JAILLET: "Re: [PATCH] seq_buf: make DECLARE_SEQ_BUF() usable"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]