Re: [stable/linux-6.12.y 1/2] 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:
- [High] Double-free and NULL pointer dereference in backing_file error path.
- [Low] Compiler warning due to pointer signedness mismatch in `lsm_blob_sizes`.
--

commit c06a83c8499b5a2cfb9d2bf689d02826faee9cb1
Author: Paul Moore <paul@paul-moore.com>
Subject: lsm: add backing_file LSM hooks

This commit adds a new LSM security blob to the backing_file struct and
introduces three new LSM hooks to manage its lifecycle and authorize mmap()
operations. These changes provide the necessary mechanisms for LSMs to
enforce access controls on mmap() and mprotect() for stacked filesystems.

> diff --git a/fs/file_table.c b/fs/file_table.c
> index 2a08bc93b0b9c..a771be30eb43d 100644
> --- a/fs/file_table.c
> +++ b/fs/file_table.c
[ ... ]
> @@ -289,7 +308,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: High]
Does this fput() trigger a double-free on the error path?

FMODE_BACKING is set on the file before init_backing_file() is called.
If init_backing_file() fails (which delegates to
security_backing_file_alloc()), this fput() will eventually call
backing_file_free() and then security_backing_file_free().

However, security_backing_file_alloc() already cleans up after itself on
failure by calling security_backing_file_free() explicitly, leading to it
being called twice.

>  		return ERR_PTR(error);
>  	}
[ ... ]
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 090d1d3e19fed..12b1576d5b027 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: Low]
Does this cause a compiler warning due to a pointer signedness mismatch?

The other fields in struct lsm_blob_sizes are declared as int, but this
is unsigned int. When passed to lsm_set_blob_size() in security.c, it
expects an int pointer, which could break builds configured with -Werror.

>  	int lbs_ib;
>  	int lbs_inode;
[ ... ]
> diff --git a/security/security.c b/security/security.c
> index 6e4deac6ec073..aef71585d49ef 100644
> --- a/security/security.c
> +++ b/security/security.c
[ ... ]
> @@ -2893,6 +2925,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
> + *
[ ... ]
> + */
> +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);

[Severity: High]
This explicit cleanup on failure seems to conflict with the fput() in
alloc_empty_backing_file() mentioned above. Will this result in
security_backing_file_free() being called twice for the same 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: High]
Can this unconditionally trigger an invalid pointer dereference?

If security_backing_file_free() is called on an error path where the blob
allocation failed (or on the second call of a double-free), the security
blob will be NULL.

Calling the LSM free hooks before checking if the blob is NULL can cause
LSMs to dereference a NULL pointer when calculating their internal state
pointers.

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

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