Re: [PATCH v5 23/23] integrity: Switch from rbtree to LSM-managed blob for integrity_iint_cache

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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);
 }
 
 /**

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [Linux Kernel]     [Linux Kernel Hardening]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux SCSI]

  Powered by Linux