On 10. 7. 2024 12:40, Mickaël Salaün wrote:
> On Tue, Jul 09, 2024 at 10:40:30PM -0400, 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>
>
> I like this new hook. It is definitely safer than the current approach.
>
> To make it more consistent, I think we should also rename
> security_inode_free() to security_inode_put() to highlight the fact that
> LSM implementations should not free potential pointers in this blob
> because they could still be dereferenced in a path walk.
>
>> ---
>> 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);
>> }
>>
>> 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;
>
> Please rename "lisec" to "inode_sec" for consistency with
> get_inode_object()'s variables.
>
>> +
>> /*
>> * 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);
>> }
>
> This looks good to me.
>
> We can add these footers:
> Reported-by: syzbot+5446fbf332b0602ede0b@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://lore.kernel.org/r/00000000000076ba3b0617f65cc8@xxxxxxxxxx
Sorry for the questions, but for several weeks I can't find answers to two things related to this RFC:
1) How does this patch close [1]?
As Mickaël pointed in [2], "It looks like security_inode_free() is called two times on the same inode."
Indeed, it does not seem from the backtrace that it is a case of race between destroy_inode and inode_permission,
i.e. referencing the inode in a VFS path walk while destroying it...
Please, can anyone tell me how this situation could have happened? Maybe folks from VFS... I added them to the copy.
2) Is there a guarantee that inode_free_by_rcu and i_callback will be called within the same RCU grace period?
If not, can the security context be released earlier than the inode itself? If yes, can be executed
inode_permission concurrently, leading to UAF of inode security context in security_inode_permission?
Can fsnotify affect this (leading to different RCU grace periods)? (Again probably a question for VFS people.)
I know, this RFC doesn't address exactly that question, but I'd like to know the answer.
Many thanks for the helpful answers and your time,
mY
[1] https://lore.kernel.org/r/00000000000076ba3b0617f65cc8@xxxxxxxxxx
[2] https://lore.kernel.org/linux-security-module/CAHC9VhQUqJkWxhe5KukPOVQMnOhcOH5E+BJ4_b3Qn6edsL5YJQ@xxxxxxxxxxxxxx/T/#m6e6b01b196eac15a7ad99cf366614bbe60b8d9a2
>
> However, I'm wondering if we could backport this patch down to v5.15 .
> I guess not, so I'll need to remove this hook implementation for
> Landlock, backport it to v5.15, and then you'll need to re-add this
> check with this patch. At least it has been useful to spot this inode
> issue, but it could still be useful to spot potential memory leaks with
> a negligible performance impact.
>
>
>>
>> /* 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);
>
> For this to work, we need to extend the inode blob size (lbs_inode) with
> sizeof(struct rcu_head). The current implementation override the
> content of the blob with a new 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);
>
> We should have something like:
> call_rcu(inode->i_security.rcu, inode_free_by_rcu);
>
>> --
>> 2.45.2
>>
>>
>
