[PATCH] scsi: target: reject configfs db_root for ALUA metadata

[PATCH] scsi: target: reject configfs db_root for ALUA metadata

[Runyu Xiao]

The db_root configfs attribute only validates that the supplied path
resolves to a directory. Later ALUA metadata writes build file names
under db_root and open them with filp_open().

If db_root itself points at a configfs mount, an ALUA transition that
updates metadata can re-enter configfs through that filp_open() path
while configfs is still servicing the original store. That makes the
metadata path recurse back into the same frag_sem.

Reject configfs-backed db_root values in target_core_item_dbroot_store()
so later metadata users keep running on a normal filesystem instead of
re-entering configfs.

Fixes: a96e9783e058 ("target: make target db location configurable")
Cc: stable@vger.kernel.org
Signed-off-by: Runyu Xiao <runyu.xiao@seu.edu.cn>
---
 drivers/target/target_core_configfs.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
index a2bd2e8..5ee6f9c 100644
--- a/drivers/target/target_core_configfs.c
+++ b/drivers/target/target_core_configfs.c
@@ -138,8 +138,16 @@ static ssize_t target_core_item_dbroot_store(struct config_item *item,
 			pr_err("db_root: not a directory: %s\n", db_root_stage);
 		goto unlock;
 	}
+	if (!strcmp(path.dentry->d_sb->s_type->name, "configfs")) {
+		pr_err("db_root: configfs is not a valid target database root: %s\n",
+		       db_root_stage);
+		r = -EINVAL;
+	}
 	path_put(&path);

+	if (r)
+		goto unlock;
+
 	strscpy(db_root, db_root_stage);
 	pr_debug("Target_Core_ConfigFS: db_root set to %s\n", db_root);

-- 
2.34.1

Re: [PATCH] scsi: target: reject configfs db_root for ALUA metadata

[sashiko-bot]

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

New issues:
- [High] Time-Of-Check to Time-Of-Use (TOCTOU) race bypasses the configfs path validation, leaving the deadlock vulnerability open.

Pre-existing issues:
- [High] Pre-existing lock inversion between `target_devices_lock` and the VFS directory `i_rwsem` causes an ABBA deadlock.
--

--- Patch [1]: [PATCH] scsi: target: reject configfs db_root for ALUA metadata ---
commit 2cce6aeea624aad95569faa866567fc7a0a6be25
Author: Runyu Xiao <runyu.xiao@seu.edu.cn>

scsi: target: reject configfs db_root for ALUA metadata

The commit adds a check to target_core_item_dbroot_store to reject db_root
paths that resolve to a configfs filesystem. This is to prevent ALUA metadata
writes from recursively re-entering configfs and deadlocking.

> diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
> index 2b19a956007b7..2c79e78105ea6 100644
> --- a/drivers/target/target_core_configfs.c
> +++ b/drivers/target/target_core_configfs.c
> @@ -138,8 +138,16 @@ static ssize_t target_core_item_dbroot_store(struct config_item *item,
>  			pr_err("db_root: not a directory: %s\n", db_root_stage);
>  		goto unlock;
>  	}

[Severity: High]
This is a pre-existing issue, but does calling kern_path() while holding
target_devices_lock introduce a lock inversion with i_rwsem?

Looking at target_core_item_dbroot_store(), it acquires target_devices_lock
and then resolves a path using kern_path(). If the provided path forces a walk
through an uncached directory, VFS acquires the directory's i_rwsem:

drivers/target/target_core_configfs.c:target_core_item_dbroot_store() {
    mutex_lock(&target_devices_lock);
    ...
    r = kern_path(db_root_stage, LOOKUP_FOLLOW | LOOKUP_DIRECTORY, &path);
    ...
}

Conversely, if a user creates a directory in configfs, vfs_mkdir() acquires
the parent directory's i_rwsem and calls target_core_make_subdev(), which
subsequently tries to acquire target_devices_lock:

drivers/target/target_core_configfs.c:target_core_make_subdev() {
    ...
    mutex_lock(&target_devices_lock);
    ...
}

Could this result in an ABBA deadlock if a user concurrently creates a
directory in configfs while writing an uncached configfs path to dbroot?

> +	if (!strcmp(path.dentry->d_sb->s_type->name, "configfs")) {
> +		pr_err("db_root: configfs is not a valid target database root: %s\n",
> +		       db_root_stage);
> +		r = -EINVAL;
> +	}
>  	path_put(&path);
>  
> +	if (r)
> +		goto unlock;
> +
>  	strscpy(db_root, db_root_stage);

[Severity: High]
Does saving the path string instead of the path reference leave a TOCTOU
window that could bypass the new validation?

The new validation checks if the resolved path is on configfs, but then drops
the path reference and stores only the string. If a user writes a path to a
symlink pointing to a normal directory, it would pass this validation check.

If the user then modifies the symlink to point to /sys/kernel/config after
the check succeeds, later ALUA operations that use filp_open() on the string
will follow the symlink directly into configfs. Could this allow the recursive
deadlock to still be triggered?

>  	pr_debug("Target_Core_ConfigFS: db_root set to %s\n", db_root);
>

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

[PATCH v2] scsi: target: pin db_root for metadata writes

[Runyu Xiao]

db_root is configured from configfs as a pathname string. ALUA and
APTPL later build metadata filenames under that string and open them
with filp_open().

Validating db_root once in target_core_item_dbroot_store() is not
enough. A later symlink retarget can bypass a one-time check, and doing
path resolution under target_devices_lock keeps the VFS lock-order
concern in the configfs store path.

Resolve db_root to a directory path once, keep a pinned struct path
reference, and open metadata files relative to that fixed root with
file_open_root(). Also reject configfs-backed roots before publishing
them and move the path walk out from under target_devices_lock.

This closes the ALUA recursive configfs re-entry path without leaving a
string-based TOCTOU gap, and keeps the same fixed-root semantics for
APTPL metadata writes.

Fixes: a96e9783e058 ("target: make target db location configurable")
Cc: stable@vger.kernel.org
Signed-off-by: Runyu Xiao <runyu.xiao@seu.edu.cn>
---
v2:
- replace one-time store-path validation with a pinned db_root path
- move db_root path resolution out from under target_devices_lock
- open ALUA and APTPL metadata files relative to the pinned root
- close the string-based TOCTOU gap raised in review

 drivers/target/target_core_alua.c     | 40 ++++++++-----
 drivers/target/target_core_configfs.c | 83 ++++++++++++++++++---------
 drivers/target/target_core_internal.h |  3 +
 drivers/target/target_core_pr.c       | 19 ++++--
 4 files changed, 96 insertions(+), 49 deletions(-)

diff --git a/drivers/target/target_core_alua.c b/drivers/target/target_core_alua.c
index 10250aca5a81..cfe4c30e534a 100644
--- a/drivers/target/target_core_alua.c
+++ b/drivers/target/target_core_alua.c
@@ -856,17 +856,27 @@ static int core_alua_write_tpg_metadata(
 	unsigned char *md_buf,
 	u32 md_buf_len)
 {
-	struct file *file = filp_open(path, O_RDWR | O_CREAT | O_TRUNC, 0600);
+	struct file *file;
 	loff_t pos = 0;
 	int ret;
 
+	if (!db_root_path.dentry) {
+		pr_err("db_root is not initialized for ALUA metadata path: %s/%s\n",
+		       db_root, path);
+		return -ENODEV;
+	}
+
+	file = file_open_root(&db_root_path, path, O_RDWR | O_CREAT | O_TRUNC,
+			      0600);
 	if (IS_ERR(file)) {
-		pr_err("filp_open(%s) for ALUA metadata failed\n", path);
+		pr_err("file_open_root(%s/%s) for ALUA metadata failed\n",
+		       db_root, path);
 		return -ENODEV;
 	}
 	ret = kernel_write(file, md_buf, md_buf_len, &pos);
 	if (ret < 0)
-		pr_err("Error writing ALUA metadata file: %s\n", path);
+		pr_err("Error writing ALUA metadata file: %s/%s\n", db_root,
+		       path);
 	fput(file);
 	return (ret < 0) ? -EIO : 0;
 }
@@ -896,9 +906,9 @@ static int core_alua_update_tpg_primary_metadata(
 			tg_pt_gp->tg_pt_gp_alua_access_status);
 
 	rc = -ENOMEM;
-	path = kasprintf(GFP_KERNEL, "%s/alua/tpgs_%s/%s", db_root,
-			&wwn->unit_serial[0],
-			config_item_name(&tg_pt_gp->tg_pt_gp_group.cg_item));
+	path = kasprintf(GFP_KERNEL, "alua/tpgs_%s/%s",
+			 &wwn->unit_serial[0],
+			 config_item_name(&tg_pt_gp->tg_pt_gp_group.cg_item));
 	if (path) {
 		rc = core_alua_write_tpg_metadata(path, md_buf, len);
 		kfree(path);
@@ -1187,16 +1197,16 @@ static int core_alua_update_tpg_secondary_metadata(struct se_lun *lun)
 			lun->lun_tg_pt_secondary_stat);
 
 	if (se_tpg->se_tpg_tfo->tpg_get_tag != NULL) {
-		path = kasprintf(GFP_KERNEL, "%s/alua/%s/%s+%hu/lun_%llu",
-				db_root, se_tpg->se_tpg_tfo->fabric_name,
-				se_tpg->se_tpg_tfo->tpg_get_wwn(se_tpg),
-				se_tpg->se_tpg_tfo->tpg_get_tag(se_tpg),
-				lun->unpacked_lun);
+		path = kasprintf(GFP_KERNEL, "alua/%s/%s+%hu/lun_%llu",
+				 se_tpg->se_tpg_tfo->fabric_name,
+				 se_tpg->se_tpg_tfo->tpg_get_wwn(se_tpg),
+				 se_tpg->se_tpg_tfo->tpg_get_tag(se_tpg),
+				 lun->unpacked_lun);
 	} else {
-		path = kasprintf(GFP_KERNEL, "%s/alua/%s/%s/lun_%llu",
-				db_root, se_tpg->se_tpg_tfo->fabric_name,
-				se_tpg->se_tpg_tfo->tpg_get_wwn(se_tpg),
-				lun->unpacked_lun);
+		path = kasprintf(GFP_KERNEL, "alua/%s/%s/lun_%llu",
+				 se_tpg->se_tpg_tfo->fabric_name,
+				 se_tpg->se_tpg_tfo->tpg_get_wwn(se_tpg),
+				 lun->unpacked_lun);
 	}
 	if (!path) {
 		rc = -ENOMEM;
diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
index a2bd2e81d2c6..7788db0c64eb 100644
--- a/drivers/target/target_core_configfs.c
+++ b/drivers/target/target_core_configfs.c
@@ -96,8 +96,31 @@ static ssize_t target_core_item_version_show(struct config_item *item,
 CONFIGFS_ATTR_RO(target_core_item_, version);
 
 char db_root[DB_ROOT_LEN] = DB_ROOT_DEFAULT;
+struct path db_root_path;
 static char db_root_stage[DB_ROOT_LEN];
 
+static int target_validate_db_root(const char *path_str, struct path *path)
+{
+	int ret;
+
+	ret = kern_path(path_str, LOOKUP_FOLLOW | LOOKUP_DIRECTORY, path);
+	if (ret) {
+		pr_err("db_root: cannot open: %s\n", path_str);
+		if (ret == -ENOTDIR)
+			pr_err("db_root: not a directory: %s\n", path_str);
+		return ret;
+	}
+
+	if (!strcmp(path->dentry->d_sb->s_type->name, "configfs")) {
+		pr_err("db_root: configfs is not a valid target database root: %s\n",
+		       path_str);
+		path_put(path);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static ssize_t target_core_item_dbroot_show(struct config_item *item,
 					    char *page)
 {
@@ -110,43 +133,49 @@ static ssize_t target_core_item_dbroot_store(struct config_item *item,
 	ssize_t read_bytes;
 	ssize_t r = -EINVAL;
 	struct path path = {};
-
-	mutex_lock(&target_devices_lock);
-	if (target_devices) {
-		pr_err("db_root: cannot be changed because it's in use\n");
-		goto unlock;
-	}
+	struct path old_path = {};
+	bool have_old_path = false;
 
 	if (count > (DB_ROOT_LEN - 1)) {
 		pr_err("db_root: count %d exceeds DB_ROOT_LEN-1: %u\n",
 		       (int)count, DB_ROOT_LEN - 1);
-		goto unlock;
+		return r;
 	}
 
 	read_bytes = scnprintf(db_root_stage, DB_ROOT_LEN, "%s", page);
 	if (!read_bytes)
-		goto unlock;
+		return r;
 
 	if (db_root_stage[read_bytes - 1] == '\n')
 		db_root_stage[read_bytes - 1] = '\0';
 
 	/* validate new db root before accepting it */
-	r = kern_path(db_root_stage, LOOKUP_FOLLOW | LOOKUP_DIRECTORY, &path);
-	if (r) {
-		pr_err("db_root: cannot open: %s\n", db_root_stage);
-		if (r == -ENOTDIR)
-			pr_err("db_root: not a directory: %s\n", db_root_stage);
-		goto unlock;
+	r = target_validate_db_root(db_root_stage, &path);
+	if (r)
+		return r;
+
+	mutex_lock(&target_devices_lock);
+	if (target_devices) {
+		pr_err("db_root: cannot be changed because it's in use\n");
+		goto unlock_put;
 	}
-	path_put(&path);
 
+	have_old_path = db_root_path.dentry;
+	if (have_old_path)
+		old_path = db_root_path;
+	db_root_path = path;
+	path = (struct path){};
 	strscpy(db_root, db_root_stage);
 	pr_debug("Target_Core_ConfigFS: db_root set to %s\n", db_root);
 
 	r = read_bytes;
 
-unlock:
+unlock_put:
 	mutex_unlock(&target_devices_lock);
+	if (path.dentry)
+		path_put(&path);
+	if (have_old_path)
+		path_put(&old_path);
 	return r;
 }
 
@@ -3643,21 +3672,19 @@ void target_setup_backend_cits(struct target_backend *tb)
 
 static void target_init_dbroot(void)
 {
-	struct file *fp;
+	struct path path = {};
+	int ret;
 
-	snprintf(db_root_stage, DB_ROOT_LEN, DB_ROOT_PREFERRED);
-	fp = filp_open(db_root_stage, O_RDONLY, 0);
-	if (IS_ERR(fp)) {
-		pr_err("db_root: cannot open: %s\n", db_root_stage);
-		return;
-	}
-	if (!S_ISDIR(file_inode(fp)->i_mode)) {
-		filp_close(fp, NULL);
-		pr_err("db_root: not a valid directory: %s\n", db_root_stage);
-		return;
+	strscpy(db_root_stage, DB_ROOT_LEN, DB_ROOT_PREFERRED);
+	ret = target_validate_db_root(db_root_stage, &path);
+	if (ret) {
+		strscpy(db_root_stage, DB_ROOT_LEN, DB_ROOT_DEFAULT);
+		ret = target_validate_db_root(db_root_stage, &path);
+		if (ret)
+			return;
 	}
-	filp_close(fp, NULL);
 
+	db_root_path = path;
 	strscpy(db_root, db_root_stage);
 	pr_debug("Target_Core_ConfigFS: db_root set to %s\n", db_root);
 }
diff --git a/drivers/target/target_core_internal.h b/drivers/target/target_core_internal.h
index 20aab1f50565..4cbc6218d4de 100644
--- a/drivers/target/target_core_internal.h
+++ b/drivers/target/target_core_internal.h
@@ -169,6 +169,9 @@ extern struct se_portal_group xcopy_pt_tpg;
 #define	DB_ROOT_DEFAULT		"/var/target"
 #define	DB_ROOT_PREFERRED	"/etc/target"
 
+struct path;
+
 extern char db_root[];
+extern struct path db_root_path;
 
 #endif /* TARGET_CORE_INTERNAL_H */
diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
index 83e172c92238..35ec38c6f528 100644
--- a/drivers/target/target_core_pr.c
+++ b/drivers/target/target_core_pr.c
@@ -1965,15 +1965,21 @@ static int __core_scsi3_write_aptpl_to_file(
 	int ret;
 	loff_t pos = 0;
 
-	path = kasprintf(GFP_KERNEL, "%s/pr/aptpl_%s", db_root,
-			&wwn->unit_serial[0]);
+	path = kasprintf(GFP_KERNEL, "pr/aptpl_%s", &wwn->unit_serial[0]);
 	if (!path)
 		return -ENOMEM;
 
-	file = filp_open(path, flags, 0600);
+	if (!db_root_path.dentry) {
+		pr_err("db_root is not initialized for APTPL metadata path: %s/%s\n",
+		       db_root, path);
+		kfree(path);
+		return -ENODEV;
+	}
+
+	file = file_open_root(&db_root_path, path, flags, 0600);
 	if (IS_ERR(file)) {
-		pr_err("filp_open(%s) for APTPL metadata"
-			" failed\n", path);
+		pr_err("file_open_root(%s/%s) for APTPL metadata failed\n",
+		       db_root, path);
 		kfree(path);
 		return PTR_ERR(file);
 	}
@@ -1983,7 +1989,8 @@ static int __core_scsi3_write_aptpl_to_file(
 	ret = kernel_write(file, buf, pr_aptpl_buf_len, &pos);
 
 	if (ret < 0)
-		pr_debug("Error writing APTPL metadata file: %s\n", path);
+		pr_debug("Error writing APTPL metadata file: %s/%s\n", db_root,
+			 path);
 	fput(file);
 	kfree(path);
 
-- 
2.34.1