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] The error path in alloc_empty_backing_file() triggers a double execution of the backing_file_free LSM hooks with a NULL security blob, violating the LSM API contract.
--

commit 5c2fdae294837ef2192abffb715e055fefb686b4
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 new LSM hooks
(security_backing_file_alloc, security_backing_file_free, and
security_mmap_backing_file). This provides the necessary mechanisms for LSMs
to enforce access controls on the mmap() and mprotect() operations in 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
[ ... ]
> @@ -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);
>  	}

[ ... ]

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

[Severity: Medium]
Does this error path lead to a double execution of the free hooks?

If call_int_hook() fails, security_backing_file_alloc() cleans up by calling
security_backing_file_free().

The error then propagates back to init_backing_file(), and then to
alloc_empty_backing_file(), which calls fput() on the error path.

The fput() call initiates the VFS teardown path, which eventually calls
backing_file_free() and executes security_backing_file_free() for a second
time.

Similarly, if lsm_backing_file_alloc() fails with -ENOMEM, it returns an error
without calling any hooks, but the subsequent fput() will still execute the
free hook without the alloc hook ever running.

> +
> +	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);

Because call_void_hook() is executed unconditionally even if blob is NULL,
could this cause state corruption for BPF LSM programs tracking file lifecycles,
or a NULL pointer dereference in future LSMs implementing the free hook?

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

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