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

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 <[email protected]>

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/[email protected]?part=1