Re: [PATCH stable/linux-5.10.y 6/7] lsm: add backing_file LSM hooks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Permanent struct file reference count leak of 'realfile' when `security_mmap_backing_file` fails in `ovl_mmap`.

Pre-existing issues:
- [High] Slab corruption when `init_file` fails in `alloc_empty_backing_file` due to a mismatch between `kzalloc` and `kmem_cache_free`.
--

commit d56daad1df4b9fc063314a1e9538b4eee20fd64d
Author: Paul Moore <paul@paul-moore.com>

lsm: add backing_file LSM hooks

Stacked filesystems such as overlayfs do not currently provide the
necessary mechanisms for LSMs to properly enforce access controls on the
mmap() and mprotect() operations. In order to resolve this gap, a LSM
security blob is being added to the backing_file struct and the following
new LSM hooks are being created.

> diff --git a/fs/file_table.c b/fs/file_table.c
> index 6792d0bce2461..829dfd35365c3 100644
> --- a/fs/file_table.c
> +++ b/fs/file_table.c
[ ... ]
> @@ -229,7 +244,8 @@ static int init_backing_file(struct backing_file *ff)
>   * This is only for kernel internal use, and the allocate file must not be
>   * installed into file tables or such.
>   */
> -struct file *alloc_empty_backing_file(int flags, const struct cred *cred)
> +struct file *alloc_empty_backing_file(int flags, const struct cred *cred,
> +				      const struct file *user_file)
>  {

[Severity: High]
This is a pre-existing issue, but does alloc_empty_backing_file() cause slab
corruption if init_file() fails?

In alloc_empty_backing_file(), we allocate a struct backing_file using
kzalloc(). Then we call init_file(&ff->file, flags, cred).

If init_file() fails (e.g., if security_file_alloc() returns -ENOMEM), it cleans
up by directly calling file_free_rcu():

fs/file_table.c:init_file() {
    ...
	error = security_file_alloc(f);
	if (unlikely(error)) {
		file_free_rcu(&f->f_u.fu_rcuhead);
		return error;
	}
    ...
}

Inside file_free_rcu(), it uses the FMODE_BACKING flag to decide how to free:

fs/file_table.c:file_free_rcu() {
    ...
	if (unlikely(f->f_mode & FMODE_BACKING))
		kfree(backing_file(f));
	else
		kmem_cache_free(filp_cachep, f);
}

Since FMODE_BACKING is only set after init_file() returns successfully, the
flag is missing during the error path, and we end up passing the kzalloc'd
pointer to kmem_cache_free(filp_cachep, f).

>  	struct backing_file *ff;
>  	int error;
[ ... ]
> diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
> index 460748b638063..318ec1d99aa16 100644
> --- a/fs/overlayfs/file.c
> +++ b/fs/overlayfs/file.c
[ ... ]
> @@ -504,6 +505,11 @@ static int ovl_mmap(struct file *file, struct vm_area_struct *vma)
>  	vma->vm_file = get_file(realfile);
>  
>  	old_cred = ovl_override_creds(file_inode(file)->i_sb);
> +	ret = security_mmap_backing_file(vma, realfile, user_file);
> +	if (ret) {
> +		revert_creds(old_cred);
> +		return ret;

[Severity: High]
Does this code leak the realfile reference on error?

When security_mmap_backing_file() fails, we return early without dropping
the reference to realfile acquired just above via get_file(realfile) and
stored in vma->vm_file.

The caller (__mmap_region()) handles the error by clearing vma->vm_file = NULL
and only drops the reference to the original user_file, which orphans the
reference to realfile.

> +	}
>  	ret = call_mmap(vma->vm_file, vma);
>  	revert_creds(old_cred);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260629070653.580879-1-caixinchen1@huawei.com?part=6