From: Roberto Sassu <roberto.sassu@huaweicloud.com>
To: 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,
dhowells@redhat.com, jarkko@kernel.org,
stephen.smalley.work@gmail.com, eparis@parisplace.org,
casey@schaufler-ca.com, mic@digikod.net
Cc: linux-fsdevel@vger.kernel.org, linux-kernel@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, Roberto Sassu <roberto.sassu@huawei.com>
Subject: Re: [PATCH v7 23/23] integrity: Switch from rbtree to LSM-managed blob for integrity_iint_cache
Date: Fri, 1 Dec 2023 01:00:48 +0100 [thread overview]
Message-ID: <f37c47d0-5b20-4293-8bd0-5d465f328ffc@huaweicloud.com> (raw)
In-Reply-To: <20231130231948.2545638-5-roberto.sassu@huaweicloud.com>
On 12/1/2023 12:19 AM, 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.
>
> With the 'integrity' LSM removed, and with the 'ima' LSM taking its role,
> reserve space from the 'ima' LSM for a pointer to the integrity_iint_cache
> structure directly, rather than embedding the whole structure in the inode
> security blob, to minimize the changes and to avoid waste of memory.
>
> If IMA is disabled, EVM faces the same problems as before making it an
> LSM, metadata verification fails for new files due to not setting the
> IMA_NEW_FILE flag in ima_post_path_mknod(), and evm_verifyxattr()
> returns INTEGRITY_UNKNOWN since IMA didn't call integrity_inode_get().
>
> The only difference caused to moving the integrity metadata management
> to the 'ima' LSM is the fact that EVM cannot take advantage of cached
> verification results, and has to do the verification again. However,
> this case should never happen, since the only public API available to
> all kernel components, evm_verifyxattr(), does not work if IMA is
> disabled.
This needs some explanation on how EVM can be used currently. EVM
verifies inode metadata in the set* LSM hooks, eventually updates the
HMAC in the post_set* hooks.
If integrity metadata are not available (IMA is disabled), EVM has to do
the inode metadata verification every time, which means that this patch
set would introduce a performance regression compared to when integrity
metadata were always available and managed by the 'integrity' LSM.
However, the get* LSM hooks are not mediated, user space can freely get
a corrupted inode metadata and EVM would not tell anything.
So, at this point it is clear that the main use case of EVM was a kernel
component querying EVM about the integrity of inode metadata, by calling
evm_verifyxattr(). One suitable place where this function can be called
is the d_instantiate LSM hook, when an LSM is getting xattrs from the
filesystem to populate the inode security blob.
But as I mentioned, evm_verifyxattr() does not work if IMA is disabled,
so there should not be systems using this configuration for which we are
introducing a performance regression.
Roberto
> 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.
>
> Keep the blob size and the new primitives definition at the common level in
> security/integrity rather than moving them in IMA itself, so that EVM can
> still call integrity_inode_get() and integrity_iint_find() while IMA is
> disabled. Just add an extra check in integrity_inode_get() to return NULL
> if that is the case.
>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
> security/integrity/iint.c | 70 ++++---------------------------
> security/integrity/ima/ima_main.c | 1 +
> security/integrity/integrity.h | 20 ++++++++-
> 3 files changed, 29 insertions(+), 62 deletions(-)
>
> diff --git a/security/integrity/iint.c b/security/integrity/iint.c
> index c36054041b84..8fc9455dda11 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 __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;
> + return integrity_inode_get_iint(inode);
> }
>
> #define IMA_MAX_NESTING (FILESYSTEM_MAX_STACK_DEPTH+1)
> @@ -123,9 +92,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;
>
> /*
> * After removing the 'integrity' LSM, the 'ima' LSM calls
> @@ -144,31 +111,10 @@ struct integrity_iint_cache *integrity_inode_get(struct inode *inode)
>
> 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);
> + integrity_inode_set_iint(inode, iint);
>
> - write_unlock(&integrity_iint_lock);
> return iint;
> }
>
> @@ -185,10 +131,8 @@ 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);
> }
> @@ -212,6 +156,10 @@ int __init integrity_iintcache_init(void)
> return 0;
> }
>
> +struct lsm_blob_sizes integrity_blob_sizes __ro_after_init = {
> + .lbs_inode = sizeof(struct integrity_iint_cache *),
> +};
> +
> /*
> * integrity_kernel_read - read data from the file
> *
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 3f59cce3fa02..52b4a3bba45a 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -1162,6 +1162,7 @@ DEFINE_LSM(ima) = {
> .name = "ima",
> .init = init_ima_lsm,
> .order = LSM_ORDER_LAST,
> + .blobs = &integrity_blob_sizes,
> };
>
> late_initcall(init_ima); /* Start IMA after the TPM is available */
> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
> index 26d3b08dca1c..2fb35c67d64d 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 */
> @@ -194,6 +193,25 @@ int integrity_kernel_read(struct file *file, loff_t offset,
> #define INTEGRITY_KEYRING_MAX 4
>
> 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;
>
prev parent reply other threads:[~2023-12-01 0:01 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-30 23:19 [PATCH v7 19/23] ima: Move to LSM infrastructure Roberto Sassu
2023-11-30 23:19 ` [PATCH v7 20/23] ima: Move IMA-Appraisal " Roberto Sassu
2023-11-30 23:19 ` [PATCH v7 21/23] evm: Move " Roberto Sassu
2023-11-30 23:19 ` [PATCH v7 22/23] integrity: Remove 'integrity' LSM and move integrity functions to 'ima' LSM Roberto Sassu
2023-11-30 23:19 ` [PATCH v7 23/23] integrity: Switch from rbtree to LSM-managed blob for integrity_iint_cache Roberto Sassu
2023-12-01 0:00 ` Roberto Sassu [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f37c47d0-5b20-4293-8bd0-5d465f328ffc@huaweicloud.com \
--to=roberto.sassu@huaweicloud.com \
--cc=Dai.Ngo@oracle.com \
--cc=brauner@kernel.org \
--cc=casey@schaufler-ca.com \
--cc=chuck.lever@oracle.com \
--cc=dhowells@redhat.com \
--cc=dmitry.kasatkin@gmail.com \
--cc=eparis@parisplace.org \
--cc=jarkko@kernel.org \
--cc=jlayton@kernel.org \
--cc=jmorris@namei.org \
--cc=keyrings@vger.kernel.org \
--cc=kolga@netapp.com \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-integrity@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-nfs@vger.kernel.org \
--cc=linux-security-module@vger.kernel.org \
--cc=mic@digikod.net \
--cc=neilb@suse.de \
--cc=paul@paul-moore.com \
--cc=roberto.sassu@huawei.com \
--cc=selinux@vger.kernel.org \
--cc=serge@hallyn.com \
--cc=stephen.smalley.work@gmail.com \
--cc=tom@talpey.com \
--cc=viro@zeniv.linux.org.uk \
--cc=zohar@linux.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).