On 7/10/2024 4:40 AM, Paul Moore wrote:
The LSM framework has an existing inode_free_security() hook which
is used by LSMs that manage state associated with an inode, but
due to the use of RCU to protect the inode, special care must be
taken to ensure that the LSMs do not fully release the inode state
until it is safe from a RCU perspective.
This patch implements a new inode_free_security_rcu() implementation
hook which is called when it is safe to free the LSM's internal inode
state. Unfortunately, this new hook does not have access to the inode
itself as it may already be released, so the existing
inode_free_security() hook is retained for those LSMs which require
access to the inode.
Signed-off-by: Paul Moore <paul@xxxxxxxxxxxxxx>
---
include/linux/lsm_hook_defs.h | 1 +
security/integrity/ima/ima.h | 2 +-
security/integrity/ima/ima_iint.c | 20 ++++++++------------
security/integrity/ima/ima_main.c | 2 +-
security/landlock/fs.c | 9 ++++++---
security/security.c | 26 +++++++++++++-------------
6 files changed, 30 insertions(+), 30 deletions(-)
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 8fd87f823d3a..abe6b0ef892a 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -114,6 +114,7 @@ LSM_HOOK(int, 0, path_notify, const struct path *path, u64 mask,
unsigned int obj_type)
LSM_HOOK(int, 0, inode_alloc_security, struct inode *inode)
LSM_HOOK(void, LSM_RET_VOID, inode_free_security, struct inode *inode)
+LSM_HOOK(void, LSM_RET_VOID, inode_free_security_rcu, void *)
LSM_HOOK(int, -EOPNOTSUPP, inode_init_security, struct inode *inode,
struct inode *dir, const struct qstr *qstr, struct xattr *xattrs,
int *xattr_count)
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 3e568126cd48..e2a2e4c7eab6 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -223,7 +223,7 @@ static inline void ima_inode_set_iint(const struct inode *inode,
struct ima_iint_cache *ima_iint_find(struct inode *inode);
struct ima_iint_cache *ima_inode_get(struct inode *inode);
-void ima_inode_free(struct inode *inode);
+void ima_inode_free_rcu(void *inode_sec);
void __init ima_iintcache_init(void);
extern const int read_idmap[];
diff --git a/security/integrity/ima/ima_iint.c b/security/integrity/ima/ima_iint.c
index e23412a2c56b..54480df90bdc 100644
--- a/security/integrity/ima/ima_iint.c
+++ b/security/integrity/ima/ima_iint.c
@@ -109,22 +109,18 @@ struct ima_iint_cache *ima_inode_get(struct inode *inode)
}
/**
- * ima_inode_free - Called on inode free
- * @inode: Pointer to the inode
+ * ima_inode_free_rcu - Called to free an inode via a RCU callback
+ * @inode_sec: The inode::i_security pointer
*
- * Free the iint associated with an inode.
+ * Free the IMA data associated with an inode.
*/
-void ima_inode_free(struct inode *inode)
+void ima_inode_free_rcu(void *inode_sec)
{
- struct ima_iint_cache *iint;
+ struct ima_iint_cache **iint_p = inode_sec + ima_blob_sizes.lbs_inode;
- if (!IS_IMA(inode))
- return;
-
- iint = ima_iint_find(inode);
- ima_inode_set_iint(inode, NULL);
-
- ima_iint_free(iint);
+ /* *iint_p should be NULL if !IS_IMA(inode) */
+ if (*iint_p)
+ ima_iint_free(*iint_p);
It looks ok for me. The only thing we are not doing is to set the
pointer to NULL, but I guess does not matter, since the inode security
blob is being freed.
I also ran some UML kernel tests in the CI, and everything looks good:
https://github.com/robertosassu/ima-evm-utils/actions/runs/9880817007/job/27291259487
Will think a bit, if I'm missing something.
Roberto
}
static void ima_iint_init_once(void *foo)
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index f04f43af651c..5b3394864b21 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -1193,7 +1193,7 @@ static struct security_hook_list ima_hooks[] __ro_after_init = {
#ifdef CONFIG_INTEGRITY_ASYMMETRIC_KEYS
LSM_HOOK_INIT(kernel_module_request, ima_kernel_module_request),
#endif
- LSM_HOOK_INIT(inode_free_security, ima_inode_free),
+ LSM_HOOK_INIT(inode_free_security_rcu, ima_inode_free_rcu),
};
static const struct lsm_id ima_lsmid = {
diff --git a/security/landlock/fs.c b/security/landlock/fs.c
index 22d8b7c28074..f583f8cec345 100644
--- a/security/landlock/fs.c
+++ b/security/landlock/fs.c
@@ -1198,13 +1198,16 @@ static int current_check_refer_path(struct dentry *const old_dentry,
/* Inode hooks */
-static void hook_inode_free_security(struct inode *const inode)
+static void hook_inode_free_security_rcu(void *inode_sec)
{
+ struct landlock_inode_security *lisec;
+
/*
* All inodes must already have been untied from their object by
* release_inode() or hook_sb_delete().
*/
- WARN_ON_ONCE(landlock_inode(inode)->object);
+ lisec = inode_sec + landlock_blob_sizes.lbs_inode;
+ WARN_ON_ONCE(lisec->object);
}
/* Super-block hooks */
@@ -1628,7 +1631,7 @@ static int hook_file_ioctl_compat(struct file *file, unsigned int cmd,
}
static struct security_hook_list landlock_hooks[] __ro_after_init = {
- LSM_HOOK_INIT(inode_free_security, hook_inode_free_security),
+ LSM_HOOK_INIT(inode_free_security_rcu, hook_inode_free_security_rcu),
LSM_HOOK_INIT(sb_delete, hook_sb_delete),
LSM_HOOK_INIT(sb_mount, hook_sb_mount),
diff --git a/security/security.c b/security/security.c
index b52e81ac5526..bc6805f7332e 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1596,9 +1596,8 @@ int security_inode_alloc(struct inode *inode)
static void inode_free_by_rcu(struct rcu_head *head)
{
- /*
- * The rcu head is at the start of the inode blob
- */
+ /* The rcu head is at the start of the inode blob */
+ call_void_hook(inode_free_security_rcu, head);
kmem_cache_free(lsm_inode_cache, head);
}
@@ -1606,20 +1605,21 @@ static void inode_free_by_rcu(struct rcu_head *head)
* security_inode_free() - Free an inode's LSM blob
* @inode: the inode
*
- * Deallocate the inode security structure and set @inode->i_security to NULL.
+ * Release any LSM resources associated with @inode, although due to the
+ * inode's RCU protections it is possible that the resources will not be
+ * fully released until after the current RCU grace period has elapsed.
+ *
+ * It is important for LSMs to note that despite being present in a call to
+ * security_inode_free(), @inode may still be referenced in a VFS path walk
+ * and calls to security_inode_permission() may be made during, or after,
+ * a call to security_inode_free(). For this reason the inode->i_security
+ * field is released via a call_rcu() callback and any LSMs which need to
+ * retain inode state for use in security_inode_permission() should only
+ * release that state in the inode_free_security_rcu() LSM hook callback.
*/
void security_inode_free(struct inode *inode)
{
call_void_hook(inode_free_security, inode);
- /*
- * The inode may still be referenced in a path walk and
- * a call to security_inode_permission() can be made
- * after inode_free_security() is called. Ideally, the VFS
- * wouldn't do this, but fixing that is a much harder
- * job. For now, simply free the i_security via RCU, and
- * leave the current inode->i_security pointer intact.
- * The inode will be freed after the RCU grace period too.
- */
if (inode->i_security)
call_rcu((struct rcu_head *)inode->i_security,
inode_free_by_rcu);
