| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ac31c465-7789-46f0-2e54-29725b3bb5da@schaufler-ca.com>
Date: Thu, 31 Aug 2023 16:05:52 -0700
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,
zohar@...ux.ibm.com, dmitry.kasatkin@...il.com,
paul@...l-moore.com, jmorris@...ei.org, serge@...lyn.com,
dhowells@...hat.com, jarkko@...nel.org,
stephen.smalley.work@...il.com, eparis@...isplace.org
Cc: linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-nfs@...r.kernel.org, linux-integrity@...r.kernel.org,
linux-security-module@...r.kernel.org, keyrings@...r.kernel.org,
selinux@...r.kernel.org, Roberto Sassu <roberto.sassu@...wei.com>,
Casey Schaufler <casey@...aufler-ca.com>
Subject: Re: [PATCH v2 25/25] integrity: Switch from rbtree to LSM-managed
blob for integrity_iint_cache
On 8/31/2023 4:38 AM, Roberto Sassu wrote:
> From: Roberto Sassu <roberto.sassu@...wei.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@...wei.com>
Reviewed-by: Casey Schaufler <casey@...aufler-ca.com>
> ---
> 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
Powered by blists - more mailing lists