Re: [PATCH v2 stable/linux-6.18.y 1/2] lsm: add backing_file LSM hooks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Double execution of the `backing_file_free` LSM hook on allocation failure, causing potential NULL pointer dereferences.
--

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

lsm: add backing_file LSM hooks

This commit adds a security blob to the backing_file struct and
introduces new LSM hooks (security_backing_file_alloc,
security_backing_file_free, and security_mmap_backing_file) to
allow LSMs to properly enforce access controls on mmap() and mprotect()
operations for stacked filesystems.

> diff --git a/fs/file_table.c b/fs/file_table.c
> index 762f03dcbcd77..987e01da99389 100644
> --- a/fs/file_table.c
> +++ b/fs/file_table.c
[ ... ]
> @@ -301,7 +319,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)
>  {
>  	struct backing_file *ff;
>  	int error;
> @@ -318,7 +337,7 @@ struct file *alloc_empty_backing_file(int flags, const struct cred *cred)
>  
>  	/* The f_mode flags must be set before fput(). */
>  	ff->file.f_mode |= FMODE_BACKING | FMODE_NOACCOUNT;
> -	error = init_backing_file(ff);
> +	error = init_backing_file(ff, user_file);
>  	if (unlikely(error)) {
>  		fput(&ff->file);

[Severity: Medium]
Will this lead to a double execution of the backing_file_free hook?

If security_backing_file_alloc() fails after successfully calling
lsm_backing_file_alloc(), it internally calls security_backing_file_free() to
clean up. It then returns an error back through init_backing_file() to here.

Because FMODE_BACKING is already set, this fput() triggers the VFS
teardown via __fput() -> file_free() -> backing_file_free() ->
security_backing_file_free(), which will call the free hook a second time.

>  		return ERR_PTR(error);
>  	}
[ ... ]
> diff --git a/security/security.c b/security/security.c
> index 603c3c6d5635d..9285909908ab8 100644
> --- a/security/security.c
> +++ b/security/security.c
[ ... ]
> @@ -2965,6 +2997,57 @@ void security_file_free(struct file *file)
>  	}
>  }
>  
> +/**
> + * security_backing_file_alloc() - Allocate and setup a backing file blob
> + * @backing_file: the backing file
> + * @user_file: the associated user visible file
> + *
> + * Allocate a backing file LSM blob and perform any necessary initialization of
> + * the LSM blob.  There will be some operations where the LSM will not have
> + * access to @user_file after this point, so any important state associated
> + * with @user_file that is important to the LSM should be captured in the
> + * backing file's LSM blob.
> + *
> + * LSM's should avoid taking a reference to @user_file in this hook as it will
> + * result in problems later when the system attempts to drop/put the file
> + * references due to a circular dependency.
> + *
> + * Return: Return 0 if the hook is successful, negative values otherwise.
> + */
> +int security_backing_file_alloc(struct file *backing_file,
> +				const struct file *user_file)
> +{
> +	int rc;
> +
> +	rc = lsm_backing_file_alloc(backing_file);
> +	if (rc)
> +		return rc;
> +	rc = call_int_hook(backing_file_alloc, backing_file, user_file);
> +	if (unlikely(rc))
> +		security_backing_file_free(backing_file);
> +
> +	return rc;
> +}
> +
> +/**
> + * security_backing_file_free() - Free a backing file blob
> + * @backing_file: the backing file
> + *
> + * Free any LSM state associate with a backing file's LSM blob, including the
> + * blob itself.
> + */
> +void security_backing_file_free(struct file *backing_file)
> +{
> +	void *blob = backing_file_security(backing_file);
> +
> +	call_void_hook(backing_file_free, backing_file);

[Severity: Medium]
If the free hook is called twice (as mentioned above), or called after
lsm_backing_file_alloc() returns ENOMEM without executing any alloc hooks,
"blob" will be NULL.

Could this cause a NULL pointer dereference if an LSM or BPF program
implements the free hook and attempts to access the security blob?

> +
> +	if (blob) {
> +		backing_file_set_security(backing_file, NULL);
> +		kmem_cache_free(lsm_backing_file_cache, blob);
> +	}
> +}

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