Re: [PATCH v3 10/17] scsi-multipath: block PR commands

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Double-free and use-after-free of `scsi_mpath_head` and its resources on the `dev_set_name` failure path.
- [High] Calling `list_del_init` on an uninitialized list_head and `device_unregister` on a failed device addition.
--

commit 3be4a33456416864c9e49c5cfb76773fb5375009
Author: John Garry <john.g.garry@oracle.com>

scsi-multipath: block PR commands

For initial scsi-multipath support, do not support Persistent Reservations
(PRs). This change intercepts PR SCSI commands for passthrough and rejects
them since PR support for SCSI is quite complicated.

> diff --git a/drivers/scsi/scsi_multipath.c b/drivers/scsi/scsi_multipath.c
> index 61fa2e4cdfdab..b4d9d6518b4fe 100644
> --- a/drivers/scsi/scsi_multipath.c
> +++ b/drivers/scsi/scsi_multipath.c

[ ... ]

While reviewing this file, I noticed a couple of pre-existing issues:

drivers/scsi/scsi_multipath.c:scsi_mpath_alloc_head() {
    ...
	ret = dev_set_name(&scsi_mpath_head->dev, "scsi_mpath_device%d",
				scsi_mpath_head->index);
	if (ret) {
		put_device(&scsi_mpath_head->dev);
		goto out_free_ida;
	}
    ...
}

[Severity: High]
This is a pre-existing issue, but can this lead to a double-free?

If dev_set_name() fails, the error path calls put_device(). Since
device_initialize() sets the kref count to 1, this drops the count to 0 and
immediately invokes the release function scsi_mpath_head_release().

The scsi_mpath_head_release() function will free the scsi_mpath_head memory
via kfree() as well as associated resources like the IDA and bioset.

The code then incorrectly falls through to out_free_ida, executing ida_free(),
mpath_put_head(), bioset_exit(), and a second kfree() on the already freed
scsi_mpath_head pointer. Will this result in a double-free and memory
corruption?

A similar issue appears in a different error path:

drivers/scsi/scsi_multipath.c:scsi_mpath_dev_alloc() {
    ...
	ret = device_add(&scsi_mpath_head->dev);
	if (ret) {
		mutex_unlock(&scsi_mpath_heads_lock);
		goto out_put_head;
	}
    ...
}

Which eventually calls the release function:

drivers/scsi/scsi_multipath.c:scsi_mpath_free_head() {
    ...
	list_del_init(&scsi_mpath_head->entry);
	mutex_unlock(&scsi_mpath_heads_lock);

	device_unregister(&scsi_mpath_head->dev);
    ...
}

[Severity: High]
This is a pre-existing issue, but does this release function operate on an
unlinked list head and a partially initialized device?

In scsi_mpath_dev_alloc(), if device_add() fails, it jumps to out_put_head and
calls scsi_mpath_put_head(). This drops the reference to 0 and invokes
scsi_mpath_free_head().

When scsi_mpath_free_head() calls list_del_init(&scsi_mpath_head->entry), the
entry is uninitialized (zeroed by kzalloc) because device_add() failed before
list_add_tail() was ever called. Can this result in a NULL pointer dereference
when __list_del_entry() attempts to link the pointers?

Furthermore, it then unconditionally calls device_unregister(). Does this
incorrectly attempt to device_del() a device that was never successfully added,
leading to driver core warnings or crashes?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703103402.3725011-1-john.g.garry@oracle.com?part=10