Re: [RFC PATCH] lsm: add the inode_free_security_rcu() LSM implementation hook

["Mickaël Salaün" <mic@digikod.net>]

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@paul-moore.com>

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@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/r/00000000000076ba3b0617f65cc8@google.com

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
> 
>