Re: [PATCH] scsi: target: file: reject configfs-backed paths in configfs stores

[sashiko-bot@kernel.org]

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

New issues:
- [High] A Time-of-Check to Time-of-Use (TOCTOU) race condition allows bypassing the configfs path validation.
- [High] Validation can be bypassed using stacked filesystems like OverlayFS.
- [Medium] Redundant parent directory validation logic guards against an impossible deadlock scenario.
- [Low] Call to unexported symbol `kern_path_parent` breaks modular builds.

Pre-existing issues:
- [High] Path validation performs VFS path lookups while holding `frag_sem`, creating an AB-BA deadlock with `configfs_rmdir`.
--

--- Patch [1]: [PATCH] scsi: target: file: reject configfs-backed paths in configfs stores ---
commit db2f5e66532db6f370e1ba2ab87399c4fbea7e7e
Author: Runyu Xiao <runyu.xiao@seu.edu.cn>
scsi: target: file: reject configfs-backed paths in configfs stores

This commit adds path validation to prevent FILEIO backend paths from
resolving inside configfs, which can cause a deadlock on frag_sem. It
introduces fd_validate_fileio_path() to check the filesystem type of
the resolved path and its parent before calling filp_open().

> diff --git a/drivers/target/target_core_file.c b/drivers/target/target_core_file.c
> index 62ced9f5102ff..ca41e217fa848 100644
> --- a/drivers/target/target_core_file.c
> +++ b/drivers/target/target_core_file.c
[ ... ]
> @@ -86,6 +87,33 @@ static struct se_device *fd_alloc_device(struct se_hba *hba, const char *name)
>  	return &fd_dev->dev;
>  }
>  
> +static int fd_validate_fileio_path(const char *path)
> +{
> +	struct path lookup_path = {};
> +	struct dentry *dentry;
> +	int ret;
> +
> +	ret = kern_path(path, LOOKUP_FOLLOW, &lookup_path);

[Severity: High]
This isn't a bug introduced by this patch, but does performing VFS path
walks here create an AB-BA deadlock with configfs_rmdir()?

A configfs store callback already holds frag_sem for reading. If kern_path()
or filp_open() traverses a configfs directory, it will block waiting for
i_rwsem to be shared. Concurrently, vfs_rmdir() on that configfs directory
holds i_rwsem exclusively and calls configfs_rmdir(), which will block
waiting for frag_sem exclusively.

> +	if (!ret) {
> +		ret = !strcmp(lookup_path.dentry->d_sb->s_type->name, "configfs") ?

[Severity: High]
Is this string comparison sufficient to catch configfs files accessed through
stacked filesystems?

If the path points to an OverlayFS mount with configfs as a lower directory,
the superblock name will be "overlay", allowing it to bypass this check. The
subsequent filp_open() would then delegate to the real configfs file, still
triggering the frag_sem deadlock.

> +			-EINVAL : 0;
> +		path_put(&lookup_path);
> +		return ret;
> +	}
> +	if (ret != -ENOENT)
> +		return ret;
> +
> +	dentry = kern_path_parent(path, &lookup_path);

[Severity: Low]
Does this break modular builds? kern_path_parent() is not an exported
symbol, so building with CONFIG_TCM_FILEIO=m will result in a linker error.

[Severity: Medium]
Is this parent directory validation necessary?

The configfs filesystem does not implement the .create callback in
configfs_dir_inode_operations. Because of this, if a file doesn't exist,
VFS lookup_open() will return -EACCES before ever calling
configfs_open_file(). It seems a deadlock is structurally impossible for
non-existent files.

> +	if (IS_ERR(dentry))
> +		return PTR_ERR(dentry);
> +
> +	ret = !strcmp(lookup_path.dentry->d_sb->s_type->name, "configfs") ?
> +		-EINVAL : 0;
> +	dput(dentry);
> +	path_put(&lookup_path);
> +	return ret;
> +}
> +
[ ... ]
> @@ -137,6 +165,17 @@ static int fd_configure_device(struct se_device *dev)
>  		flags &= ~O_DSYNC;
>  	}
>  
> +	ret = fd_validate_fileio_path(fd_dev->fd_dev_name);
> +	if (ret) {
> +		if (ret == -EINVAL)
> +			pr_err("configfs-backed path is not valid for FILEIO backend: %s\n",
> +			       fd_dev->fd_dev_name);
> +		else
> +			pr_err("FILEIO backend path lookup failed for %s: %d\n",
> +			       fd_dev->fd_dev_name, ret);
> +		goto fail;
> +	}
> +
>  	file = filp_open(fd_dev->fd_dev_name, flags, 0600);

[Severity: High]
Can a Time-of-Check to Time-of-Use (TOCTOU) race happen here?

There is a window between fd_validate_fileio_path() returning success
and filp_open() resolving the path again. A user could provide a path
containing a symlink, wait for the validation check to complete, and then
replace the symlink to point into configfs before filp_open() is called,
bypassing the protection.

[ ... ]
> @@ -847,6 +886,17 @@ static int fd_init_prot(struct se_device *dev)
>  	snprintf(buf, FD_MAX_DEV_PROT_NAME, "%s.protection",
>  		 fd_dev->fd_dev_name);
>  
> +	ret = fd_validate_fileio_path(buf);
> +	if (ret) {
> +		if (ret == -EINVAL)
> +			pr_err("configfs-backed path is not valid for FILEIO protection: %s\n",
> +			       buf);
> +		else
> +			pr_err("FILEIO protection path lookup failed for %s: %d\n",
> +			       buf, ret);
> +		return ret;
> +	}
> +
>  	prot_file = filp_open(buf, flags, 0600);

[Severity: High]
Does the same TOCTOU race exist here between path validation and
filp_open()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260818045807.1519433-1-runyu.xiao@seu.edu.cn?part=1