Re: [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 3 potential issue(s) to consider:
- [High] Error paths in backing file allocation cause the `backing_file_free` LSM hooks to be invoked twice or with uninitialized state, and violate teardown ordering.
- [Medium] `lbs_backing_file` is defined as `unsigned int` instead of `int`, causing `-Wincompatible-pointer-types` warnings.
- [Low] `lsm_backing_file_cache` is missing the `static` keyword, polluting the global namespace and contradicting the commit message.
--

commit c7ac34e119da0f05f75151b3feefa3ded48b06cb
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.

> Mainline declares lsm_backing_file_cache in security/lsm.h.  Linux 6.18.y
> does not have security/lsm_init.c or security/lsm.h; the cache variable
> is defined locally as static struct kmem_cache *lsm_backing_file_cache in
> security/security.c.

[Severity: Low]
This isn't a bug, but the commit message explicitly states the cache
variable is defined locally as static, yet the actual definition in
security/security.c omits the static keyword.

> 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);
>  		return ERR_PTR(error);
>  	}

[Severity: High]
Can this error path cause a double-free regression or execute
uninitialized hooks?

If init_backing_file() fails because a security module hook returns an
error, security_backing_file_alloc() internally rolls back the state by
calling security_backing_file_free() before returning the error.

When the error is returned here, fput(&ff->file) is called.

Since the file mode already contains the backing bit, fput() triggers
a cleanup cascade that calls file_free(), which in turn calls
backing_file_free().

This sequence invokes security_backing_file_free() a second time on the
same file with a null blob.

If the initial blob allocation fails instead and lsm_backing_file_alloc()
returns an out of memory error, it skips the internal free but still
returns an error. The fput() call still drives the cleanup, and the
backing_file_free void hooks are executed unconditionally despite the
allocation hook never having run.

Additionally, in the teardown sequence inside file_free(),
security_file_free(f) is called before backing_file_free().

Does destroying the base object's security state before the backing
file's state expose any security hooks to a use-after-free, if they
attempt to access the base file state during backing file teardown?

> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 79ec5a2bdcca7..e0707ec5a76dc 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -104,6 +104,7 @@ struct security_hook_list {
>  struct lsm_blob_sizes {
>  	int lbs_cred;
>  	int lbs_file;
> +	unsigned int lbs_backing_file;

[Severity: Medium]
Is this intentional? The other blob size fields in struct lsm_blob_sizes
are declared as int.

In security/security.c:lsm_set_blob_sizes(), the address of this field
is passed to lsm_set_blob_size(int *need, int *lbs):

lsm_set_blob_size(&needed->lbs_backing_file, &blob_sizes.lbs_backing_file);

Could passing an unsigned int pointer to an int pointer generate a
pointer type warning that could break builds configured to treat warnings
as errors?

> diff --git a/security/security.c b/security/security.c
> index 603c3c6d5635d..5d5ad55dbca76 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -94,6 +94,7 @@ const char *const lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX + 1] = {
>  static BLOCKING_NOTIFIER_HEAD(blocking_lsm_notifier_chain);
>  
>  static struct kmem_cache *lsm_file_cache;
> +struct kmem_cache *lsm_backing_file_cache;

[Severity: Low]
Would this missing static modifier pollute the global symbol namespace?

As mentioned in the commit message, this was intended to be locally
defined.

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