Re: [PATCH v5 23/23] integrity: Switch from rbtree to LSM-managed blob for integrity_iint_cache
From: Roberto Sassu
Date: Wed Nov 29 2023 - 08:59:22 EST
On Wed, 2023-11-29 at 13:27 +0100, Roberto Sassu wrote:
> On Mon, 2023-11-20 at 16:06 -0500, Paul Moore wrote:
> > On Mon, Nov 20, 2023 at 3:16 AM Roberto Sassu
> > <roberto.sassu@xxxxxxxxxxxxxxx> wrote:
> > > On Fri, 2023-11-17 at 15:57 -0500, Paul Moore wrote:
> > > > On Nov 7, 2023 Roberto Sassu <roberto.sassu@xxxxxxxxxxxxxxx> wrote:
> > > > >
> > > > > 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.
> > > > >
> > > > > Because of this alternative storage mechanism, there was no need to use
> > > > > disjoint inode metadata, so IMA and EVM today still share them.
> > > > >
> > > > > 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. However, since IMA and EVM share the
> > > > > inode metadata, they cannot directly reserve the space for them.
> > > > >
> > > > > Instead, request from the 'integrity' LSM a space in the security blob for
> > > > > the pointer of inode metadata (integrity_iint_cache structure). The other
> > > > > reason for keeping the 'integrity' LSM is to preserve the original ordering
> > > > > of IMA and EVM functions as when they were hardcoded.
> > > > >
> > > > > Prefer reserving space for a pointer 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@xxxxxxxxxx>
> > > > > Reviewed-by: Casey Schaufler <casey@xxxxxxxxxxxxxxxx>
> > > > > Reviewed-by: Mimi Zohar <zohar@xxxxxxxxxxxxx>
> > > > > ---
> > > > > security/integrity/iint.c | 71 +++++-----------------------------
> > > > > security/integrity/integrity.h | 20 +++++++++-
> > > > > 2 files changed, 29 insertions(+), 62 deletions(-)
> > > > >
> > > > > diff --git a/security/integrity/iint.c b/security/integrity/iint.c
> > > > > index 882fde2a2607..a5edd3c70784 100644
> > > > > --- a/security/integrity/iint.c
> > > > > +++ b/security/integrity/iint.c
> > > > > @@ -231,6 +175,10 @@ static int __init integrity_lsm_init(void)
> > > > > return 0;
> > > > > }
> > > > >
> > > > > +struct lsm_blob_sizes integrity_blob_sizes __ro_after_init = {
> > > > > + .lbs_inode = sizeof(struct integrity_iint_cache *),
> > > > > +};
> > > >
> > > > I'll admit that I'm likely missing an important detail, but is there
> > > > a reason why you couldn't stash the integrity_iint_cache struct
> > > > directly in the inode's security blob instead of the pointer? For
> > > > example:
> > > >
> > > > struct lsm_blob_sizes ... = {
> > > > .lbs_inode = sizeof(struct integrity_iint_cache),
> > > > };
> > > >
> > > > struct integrity_iint_cache *integrity_inode_get(inode)
> > > > {
> > > > if (unlikely(!inode->isecurity))
> > > > return NULL;
> > > > return inode->i_security + integrity_blob_sizes.lbs_inode;
> > > > }
> > >
> > > It would increase memory occupation. Sometimes the IMA policy
> > > encompasses a small subset of the inodes. Allocating the full
> > > integrity_iint_cache would be a waste of memory, I guess?
> >
> > Perhaps, but if it allows us to remove another layer of dynamic memory
> > I would argue that it may be worth the cost. It's also worth
> > considering the size of integrity_iint_cache, while it isn't small, it
> > isn't exactly huge either.
> >
> > > On the other hand... (did not think fully about that) if we embed the
> > > full structure in the security blob, we already have a mutex available
> > > to use, and we don't need to take the inode lock (?).
> >
> > That would be excellent, getting rid of a layer of locking would be significant.
> >
> > > I'm fully convinced that we can improve the implementation
> > > significantly. I just was really hoping to go step by step and not
> > > accumulating improvements as dependency for moving IMA and EVM to the
> > > LSM infrastructure.
> >
> > I understand, and I agree that an iterative approach is a good idea, I
> > just want to make sure we keep things tidy from a user perspective,
> > i.e. not exposing the "integrity" LSM when it isn't required.
>
> Ok, I went back to it again.
>
> I think trying to separate integrity metadata is premature now, too
> many things at the same time.
>
> I started to think, does EVM really need integrity metadata or it can
> work without?
>
> The fact is that CONFIG_IMA=n and CONFIG_EVM=y is allowed, so we have
> the same problem now. What if we make IMA the one that manages
> integrity metadata, so that we can remove the 'integrity' LSM?
>
> So, no embedding the full structure in the security blob now, move
> integrity_inode_free() and integrity_kernel_module_request() to IMA,
> call integrity_iintcache_init() from IMA.
>
> EVM verification of new files would fail without IMA, but it would be
> the same now.
>
> Also, evm_verifyxattr() would only work with IMA, as it assumes that
> the latter creates integrity metadata and passes them as argument.
>
> Regarding the LSM order, I would take Casey's suggestion of introducing
> LSM_ORDER_REALLY_LAST, for EVM.
I attach the diff v5..v7.
Tests passes with both IMA and EVM enabled. I did minor tweaks to the
tests to take into account the possibility that IMA is disabled, and
tests pass also in this case.
Roberto
diff --git a/fs/file_table.c b/fs/file_table.c
index e64b0057fa72..0401ac98281c 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -384,7 +384,7 @@ static void __fput(struct file *file)
eventpoll_release(file);
locks_remove_file(file);
- security_file_pre_free(file);
+ security_file_release(file);
if (unlikely(file->f_flags & FASYNC)) {
if (file->f_op->fasync)
file->f_op->fasync(-1, file, 0);
diff --git a/fs/xattr.c b/fs/xattr.c
index 2660bc7effdc..f8b643f91a98 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -552,7 +552,7 @@ __vfs_removexattr_locked(struct mnt_idmap *idmap,
error = __vfs_removexattr(idmap, dentry, name);
if (error)
- goto out;
+ return error;
fsnotify_xattr(dentry);
security_inode_post_removexattr(dentry, name);
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 9601df10ea28..2679905f4260 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -181,7 +181,7 @@ LSM_HOOK(int, 0, kernfs_init_security, struct kernfs_node *kn_dir,
struct kernfs_node *kn)
LSM_HOOK(int, 0, file_permission, struct file *file, int mask)
LSM_HOOK(int, 0, file_alloc_security, struct file *file)
-LSM_HOOK(void, LSM_RET_VOID, file_pre_free_security, struct file *file)
+LSM_HOOK(void, LSM_RET_VOID, file_release, struct file *file)
LSM_HOOK(void, LSM_RET_VOID, file_free_security, struct file *file)
LSM_HOOK(int, 0, file_ioctl, struct file *file, unsigned int cmd,
unsigned long arg)
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index a2ade0ffe9e7..8b0c96dd7c90 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -125,7 +125,8 @@ extern void security_add_hooks(struct security_hook_list *hooks, int count,
enum lsm_order {
LSM_ORDER_FIRST = -1, /* This is only for capabilities. */
LSM_ORDER_MUTABLE = 0,
- LSM_ORDER_LAST = 1, /* This is only for integrity. */
+ LSM_ORDER_LAST = 1, /* For always enabled LSMs after mutable ones. */
+ LSM_ORDER_REALLY_LAST = 2, /* After the last ones. */
};
struct lsm_info {
diff --git a/include/linux/security.h b/include/linux/security.h
index 1cd84970ab4c..766eaccc4679 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -402,7 +402,7 @@ int security_kernfs_init_security(struct kernfs_node *kn_dir,
struct kernfs_node *kn);
int security_file_permission(struct file *file, int mask);
int security_file_alloc(struct file *file);
-void security_file_pre_free(struct file *file);
+void security_file_release(struct file *file);
void security_file_free(struct file *file);
int security_file_ioctl(struct file *file, unsigned int cmd, unsigned long arg);
int security_mmap_file(struct file *file, unsigned long prot,
@@ -1028,7 +1028,7 @@ static inline int security_file_alloc(struct file *file)
return 0;
}
-static inline void security_file_pre_free(struct file *file)
+static inline void security_file_release(struct file *file)
{ }
static inline void security_file_free(struct file *file)
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index 21560874e5fc..fa54166e1a3d 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -1037,37 +1037,20 @@ static const struct lsm_id evm_lsmid = {
.id = LSM_ID_EVM,
};
-/* Return the EVM LSM ID, if EVM is enabled or NULL if not. */
-const struct lsm_id *evm_get_lsm_id(void)
-{
- return &evm_lsmid;
-}
-
-/*
- * Since with the LSM_ORDER_LAST there is no guarantee about the ordering
- * within the .lsm_info.init section, ensure that IMA hooks are before EVM
- * ones, by letting the 'integrity' LSM call init_evm_lsm() to initialize the
- * 'ima' and 'evm' LSMs in this sequence.
- */
-void __init init_evm_lsm(void)
+static int __init init_evm_lsm(void)
{
security_add_hooks(evm_hooks, ARRAY_SIZE(evm_hooks), &evm_lsmid);
+ return 0;
}
static struct lsm_blob_sizes evm_blob_sizes __ro_after_init = {
.lbs_xattr_count = 1,
};
-/* Introduce a dummy function as 'evm' init method (it cannot be NULL). */
-static int __init dummy_init_evm_lsm(void)
-{
- return 0;
-}
-
DEFINE_LSM(evm) = {
.name = "evm",
- .init = dummy_init_evm_lsm,
- .order = LSM_ORDER_LAST,
+ .init = init_evm_lsm,
+ .order = LSM_ORDER_REALLY_LAST,
.blobs = &evm_blob_sizes,
};
diff --git a/security/integrity/iint.c b/security/integrity/iint.c
index a5edd3c70784..8fc9455dda11 100644
--- a/security/integrity/iint.c
+++ b/security/integrity/iint.c
@@ -94,6 +94,13 @@ struct integrity_iint_cache *integrity_inode_get(struct inode *inode)
{
struct integrity_iint_cache *iint;
+ /*
+ * After removing the 'integrity' LSM, the 'ima' LSM calls
+ * integrity_iintcache_init() to initialize iint_cache.
+ */
+ if (!IS_ENABLED(CONFIG_IMA))
+ return NULL;
+
iint = integrity_iint_find(inode);
if (iint)
return iint;
@@ -117,7 +124,7 @@ struct integrity_iint_cache *integrity_inode_get(struct inode *inode)
*
* Free the integrity information(iint) associated with an inode.
*/
-static void integrity_inode_free(struct inode *inode)
+void integrity_inode_free(struct inode *inode)
{
struct integrity_iint_cache *iint;
@@ -137,41 +144,15 @@ static void iint_init_once(void *foo)
memset(iint, 0, sizeof(*iint));
}
-static struct security_hook_list integrity_hooks[] __ro_after_init = {
- LSM_HOOK_INIT(inode_free_security, integrity_inode_free),
-#ifdef CONFIG_INTEGRITY_ASYMMETRIC_KEYS
- LSM_HOOK_INIT(kernel_module_request, integrity_kernel_module_request),
-#endif
-};
-
/*
- * Perform the initialization of the 'integrity', 'ima' and 'evm' LSMs to
- * ensure that the management of integrity metadata is working at the time
- * IMA and EVM hooks are registered to the LSM infrastructure, and to keep
- * the original ordering of IMA and EVM functions as when they were hardcoded.
+ * Initialize the integrity metadata cache from IMA, since it is the only LSM
+ * that really needs it. EVM can work without it.
*/
-static int __init integrity_lsm_init(void)
+int __init integrity_iintcache_init(void)
{
- const struct lsm_id *lsmid;
-
iint_cache =
kmem_cache_create("iint_cache", sizeof(struct integrity_iint_cache),
0, SLAB_PANIC, iint_init_once);
- /*
- * Obtain either the IMA or EVM LSM ID to register integrity-specific
- * hooks under that LSM, since there is no LSM ID assigned to the
- * 'integrity' LSM.
- */
- lsmid = ima_get_lsm_id();
- if (!lsmid)
- lsmid = evm_get_lsm_id();
- /* No point in continuing, since both IMA and EVM are disabled. */
- if (!lsmid)
- return 0;
-
- security_add_hooks(integrity_hooks, ARRAY_SIZE(integrity_hooks), lsmid);
- init_ima_lsm();
- init_evm_lsm();
return 0;
}
@@ -179,17 +160,6 @@ struct lsm_blob_sizes integrity_blob_sizes __ro_after_init = {
.lbs_inode = sizeof(struct integrity_iint_cache *),
};
-/*
- * Keep it until IMA and EVM can use disjoint integrity metadata, and their
- * initialization order can be swapped without change in their behavior.
- */
-DEFINE_LSM(integrity) = {
- .name = "integrity",
- .init = integrity_lsm_init,
- .order = LSM_ORDER_LAST,
- .blobs = &integrity_blob_sizes,
-};
-
/*
* 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 9aabbc37916c..52b4a3bba45a 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -1126,7 +1126,7 @@ static struct security_hook_list ima_hooks[] __ro_after_init = {
LSM_HOOK_INIT(bprm_check_security, ima_bprm_check),
LSM_HOOK_INIT(file_post_open, ima_file_check),
LSM_HOOK_INIT(inode_post_create_tmpfile, ima_post_create_tmpfile),
- LSM_HOOK_INIT(file_pre_free_security, ima_file_free),
+ LSM_HOOK_INIT(file_release, ima_file_free),
LSM_HOOK_INIT(mmap_file, ima_file_mmap),
LSM_HOOK_INIT(file_mprotect, ima_file_mprotect),
LSM_HOOK_INIT(kernel_load_data, ima_load_data),
@@ -1138,6 +1138,10 @@ static struct security_hook_list ima_hooks[] __ro_after_init = {
#endif
#ifdef CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS
LSM_HOOK_INIT(key_post_create_or_update, ima_post_key_create_or_update),
+#endif
+ LSM_HOOK_INIT(inode_free_security, integrity_inode_free),
+#ifdef CONFIG_INTEGRITY_ASYMMETRIC_KEYS
+ LSM_HOOK_INIT(kernel_module_request, integrity_kernel_module_request),
#endif
};
@@ -1146,34 +1150,19 @@ static const struct lsm_id ima_lsmid = {
.id = LSM_ID_IMA,
};
-/* Return the IMA LSM ID, if IMA is enabled or NULL if not. */
-const struct lsm_id *ima_get_lsm_id(void)
-{
- return &ima_lsmid;
-}
-
-/*
- * Since with the LSM_ORDER_LAST there is no guarantee about the ordering
- * within the .lsm_info.init section, ensure that IMA hooks are before EVM
- * ones, by letting the 'integrity' LSM call init_ima_lsm() to initialize the
- * 'ima' and 'evm' LSMs in this sequence.
- */
-void __init init_ima_lsm(void)
+static int __init init_ima_lsm(void)
{
+ integrity_iintcache_init();
security_add_hooks(ima_hooks, ARRAY_SIZE(ima_hooks), &ima_lsmid);
init_ima_appraise_lsm(&ima_lsmid);
-}
-
-/* Introduce a dummy function as 'ima' init method (it cannot be NULL). */
-static int __init dummy_init_ima_lsm(void)
-{
return 0;
}
DEFINE_LSM(ima) = {
.name = "ima",
- .init = dummy_init_ima_lsm,
+ .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 ef2689b5264d..2fb35c67d64d 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -180,6 +180,8 @@ struct integrity_iint_cache {
*/
struct integrity_iint_cache *integrity_iint_find(struct inode *inode);
struct integrity_iint_cache *integrity_inode_get(struct inode *inode);
+void integrity_inode_free(struct inode *inode);
+int __init integrity_iintcache_init(void);
int integrity_kernel_read(struct file *file, loff_t offset,
void *addr, unsigned long count);
@@ -213,36 +215,6 @@ static inline void integrity_inode_set_iint(const struct inode *inode,
struct modsig;
-#ifdef CONFIG_IMA
-const struct lsm_id *ima_get_lsm_id(void);
-void __init init_ima_lsm(void);
-#else
-static inline const struct lsm_id *ima_get_lsm_id(void)
-{
- return NULL;
-}
-
-static inline void __init init_ima_lsm(void)
-{
-}
-
-#endif
-
-#ifdef CONFIG_EVM
-const struct lsm_id *evm_get_lsm_id(void);
-void __init init_evm_lsm(void);
-#else
-static inline const struct lsm_id *evm_get_lsm_id(void)
-{
- return NULL;
-}
-
-static inline void __init init_evm_lsm(void)
-{
-}
-
-#endif
-
#ifdef CONFIG_INTEGRITY_SIGNATURE
int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
diff --git a/security/security.c b/security/security.c
index 0d9eaa4cd260..4e3dbeef09fa 100644
--- a/security/security.c
+++ b/security/security.c
@@ -331,12 +331,18 @@ static void __init ordered_lsm_parse(const char *order, const char *origin)
}
}
- /* LSM_ORDER_LAST is always last. */
+ /* LSM_ORDER_LAST after mutable ones. */
for (lsm = __start_lsm_info; lsm < __end_lsm_info; lsm++) {
if (lsm->order == LSM_ORDER_LAST)
append_ordered_lsm(lsm, " last");
}
+ /* LSM_ORDER_REALLY_LAST after LSM_ORDER_LAST. */
+ for (lsm = __start_lsm_info; lsm < __end_lsm_info; lsm++) {
+ if (lsm->order == LSM_ORDER_REALLY_LAST)
+ append_ordered_lsm(lsm, " really last");
+ }
+
/* Disable all LSMs not in the ordered list. */
for (lsm = __start_lsm_info; lsm < __end_lsm_info; lsm++) {
if (exists_ordered_lsm(lsm))
@@ -2746,14 +2752,14 @@ int security_file_alloc(struct file *file)
}
/**
- * security_file_pre_free() - Perform actions before releasing the file ref
+ * security_file_release() - Perform actions before releasing the file ref
* @file: the file
*
* Perform actions before releasing the last reference to a file.
*/
-void security_file_pre_free(struct file *file)
+void security_file_release(struct file *file)
{
- call_void_hook(file_pre_free_security, file);
+ call_void_hook(file_release, file);
}
/**