Re: [PATCH v2] scsi: target: file: avoid recursive configfs open in fd_init_prot()

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Double fetch of `d_name.len` allows concurrent rename to cause a heap buffer overflow and use-after-free
- [High] AB-BA deadlock between the directory inode lock (`i_rwsem`) and filesystem freeze protection (`sb_writers`)
- [High] Synthesizing a `struct path` using a bind mount's vfsmount and the underlying dcache parent creates an invalid VFS object state
- [High] Opening an existing protection sidecar file does not verify its file type, allowing an unprivileged user to panic the kernel
- [Medium] Usage of the unexported static function `lookup_one_qstr_excl()` causes build failures, and passing an uninitialized `qstr.hash` corrupts the dcache
- [Medium] Missing validation of the backing file's parent directory and hashed state after acquiring the parent inode lock
--

--- Patch [1]: [PATCH v2] scsi: target: file: avoid recursive configfs open in fd_init_prot() ---
commit dff9df1c1570ab279457bc3f5bedd541efae3c74
Author: Runyu Xiao <[email protected]>
scsi: target: file: avoid recursive configfs open in fd_init_prot()

This commit addresses a potential recursive configfs open issue when creating
protection sidecar files. It introduces fd_open_prot_file() to lookup, and
either create or open the protection sidecar directly from the configured
backing file's parent directory, instead of using filp_open() which might
re-enter configfs.

> diff --git a/drivers/target/target_core_file.c b/drivers/target/target_core_file.c
> index 62ced9f5102ff..9872f8eda1ac4 100644
> --- a/drivers/target/target_core_file.c
> +++ b/drivers/target/target_core_file.c

[ ... ]

> @@ -86,6 +90,86 @@ static struct se_device *fd_alloc_device(struct se_hba *hba, const char *name)
>  	return &fd_dev->dev;
>  }
>  
> +static bool fd_backing_file_is_configfs(struct file *file)
> +{
> +	return d_real(file_dentry(file), D_REAL_DATA)->d_sb->s_magic ==
> +	       CONFIGFS_MAGIC;
> +}
> +
> +static int fd_open_prot_file(struct file *file, int flags, umode_t mode,
> +			     struct file **filep)
> +{
> +	struct dentry *backing_dentry = file_dentry(file);
> +	struct path parent_path = {
> +		.mnt = mntget(file->f_path.mnt),
> +		.dentry = dget_parent(backing_dentry),
> +	};

[Severity: High]
Does this create an invalid VFS object state if the backing file is a
file-level bind mount?

If the file was provided via a file-level bind mount, the vfsmount's root is
the file itself. Using dget_parent() escapes this root to the underlying parent
directory, and passing this disjoint path to dentry_open() or dentry_create()
could violate VFS invariants.

> +	struct path prot_path = { .mnt = parent_path.mnt };
> +	struct file *prot_file;
> +	struct qstr prot_qname;
> +	char *prot_name;
> +	size_t prot_name_len;
> +	int ret;
> +
> +	if (fd_backing_file_is_configfs(file)) {
> +		ret = -EINVAL;
> +		goto out_put_parent;
> +	}
> +
> +	prot_name_len = backing_dentry->d_name.len + strlen(".protection");
> +	if (prot_name_len > NAME_MAX) {
> +		ret = -ENAMETOOLONG;
> +		goto out_put_parent;
> +	}
> +
> +	prot_name = kmalloc(prot_name_len + 1, GFP_KERNEL);
> +	if (!prot_name) {
> +		ret = -ENOMEM;
> +		goto out_put_parent;
> +	}
> +
> +	memcpy(prot_name, backing_dentry->d_name.name, backing_dentry->d_name.len);

[Severity: High]
Is there a risk of a heap buffer overflow here?

The length of d_name is read without locks to allocate prot_name via kmalloc().

If an unprivileged user renames the backing file concurrently to a longer name,
couldn't the second fetch of d_name.len during memcpy() exceed the allocated
buffer size?

> +	memcpy(prot_name + backing_dentry->d_name.len, ".protection",
> +	       strlen(".protection") + 1);
> +	prot_qname = QSTR_INIT(prot_name, prot_name_len);
> +
> +	inode_lock_nested(d_inode(parent_path.dentry), I_MUTEX_PARENT);

[Severity: Medium]
Is there a missing check for concurrent renames or unlinks here?

The parent dentry was fetched locklessly via dget_parent(), but there is no
re-validation that backing_dentry->d_parent still matches parent_path.dentry,
or that the dentry is still hashed, after acquiring the inode lock.

> +	prot_path.dentry = lookup_one_qstr_excl(&prot_qname, parent_path.dentry,
> +						LOOKUP_CREATE);

[Severity: Medium]
Will this cause a build failure when compiled as a module?

lookup_one_qstr_excl() is a static function in fs/namei.c and is not exported
to modules.

Additionally, prot_qname is initialized above using QSTR_INIT() without
computing its hash. Does passing a qstr with a zero hash to low-level VFS
functions force dentries into bucket 0 and corrupt the dcache hash table?

> +	if (IS_ERR(prot_path.dentry)) {
> +		ret = PTR_ERR(prot_path.dentry);
> +		goto out_unlock;
> +	}
> +
> +	if (d_is_negative(prot_path.dentry)) {
> +		ret = mnt_want_write_file(file);

[Severity: High]
Does this invert the standard VFS lock ordering and risk an AB-BA deadlock?

The directory's i_rwsem is currently held via inode_lock_nested(), and
mnt_want_write_file() internally acquires sb_writers (read). The standard
order is sb_writers before i_rwsem, so this could deadlock against a
filesystem freeze which takes sb_writers exclusively.

> +		if (ret)
> +			goto out_dput;
> +
> +		prot_file = dentry_create(&prot_path, flags, mode, current_cred());
> +		mnt_drop_write_file(file);
> +	} else {
> +		prot_file = dentry_open(&prot_path, flags & ~O_CREAT,
> +					current_cred());

[Severity: High]
Is there a check missing to ensure the existing protection sidecar is a regular
file?

If an unprivileged user preemptively creates a symlink named with .protection,
opening it directly via dentry_open() triggers a WARN_ON in do_dentry_open()
because symlinks generally have a NULL i_fop. This can panic the kernel on
systems with panic_on_warn.

> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1