[PATCH] scsi: virtio_scsi: set DRIVER_OK before registering the SCSI host

[PATCH] scsi: virtio_scsi: set DRIVER_OK before registering the SCSI host

[Jasper Wise]

virtscsi_probe() registers the SCSI host, and scsi_add_host() adds the
class device carrying the host's write-only "scan" attribute. A write to it
reaches scsi_scan_host_selected(), which submits INQUIRY commands on a
request virtqueue, notifying the device.

virtscsi_probe() calls virtio_device_ready() only after scsi_add_host() has
returned, by which point that attribute is already writable. The virtio
spec is explicit about that ordering in 3.1 Device Initialization:
  |  The driver MUST NOT send any buffer available notifications to the
  |  device before setting DRIVER_OK.

Set DRIVER_OK before registering the host, as done for the same reason in
commit f5866db64f34 ("virtio_console: enable VQs early") and commit
1d774589f924 ("i2c: virtio: mark device ready before registering the
adapter"). scsi_add_host() can fail and now runs with DRIVER_OK set, so
reset the device on that path using the driver's existing
virtscsi_remove_vqs() helper; the core does not reset it when probe fails.

Fixes: 4fe74b1cb051 ("[SCSI] virtio-scsi: SCSI driver for QEMU based virtual machines")
Signed-off-by: Jasper Wise <jaspwise@amazon.co.uk>
---
 drivers/scsi/virtio_scsi.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index 5fdaa71f0652..b394de0335d8 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -976,12 +976,12 @@ static int virtscsi_probe(struct virtio_device *vdev)
 	}
 #endif
 
+	virtio_device_ready(vdev);
+
 	err = scsi_add_host(shost, &vdev->dev);
 	if (err)
 		goto scsi_add_host_failed;
 
-	virtio_device_ready(vdev);
-
 	for (int i = 0; i < VIRTIO_SCSI_EVENT_LEN; i++)
 		INIT_WORK(&vscsi->event_list[i].work, virtscsi_handle_event);
 
@@ -991,7 +991,7 @@ static int virtscsi_probe(struct virtio_device *vdev)
 	return 0;
 
 scsi_add_host_failed:
-	vdev->config->del_vqs(vdev);
+	virtscsi_remove_vqs(vdev);
 virtscsi_init_failed:
 	scsi_host_put(shost);
 	return err;

base-commit: f5098b6bae761e346ebcd9da7f95622c04733cff
-- 
2.50.1

Re: [PATCH] scsi: virtio_scsi: set DRIVER_OK before registering the SCSI host

[sashiko-bot]

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

Pre-existing issues:
- [High] Preexisting Use-After-Free in the SCSI error handler or block layer when `scsi_add_host()` fails after a concurrent sysfs scan.
--

--- Patch [1]: [PATCH] scsi: virtio_scsi: set DRIVER_OK before registering the SCSI host ---
commit 58209fcc60c7b4a593d3a3456eb44d5ff9a7db6b
Author: Jasper Wise <jaspwise@amazon.co.uk>

scsi: virtio_scsi: set DRIVER_OK before registering the SCSI host

This commit alters the virtio-scsi device initialization to set DRIVER_OK
prior to calling scsi_add_host(). It also updates the probe error handling
to ensure the device is correctly reset if host registration fails.

> diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
> --- a/drivers/scsi/virtio_scsi.c
> +++ b/drivers/scsi/virtio_scsi.c
[ ... ]
> @@ -991,7 +991,7 @@ static int virtscsi_probe(struct virtio_device *vdev)
>  	return 0;
>  
>  scsi_add_host_failed:
> -	vdev->config->del_vqs(vdev);
> +	virtscsi_remove_vqs(vdev);

[Severity: High]
This is a pre-existing issue, but could this error path trigger a
use-after-free if scsi_add_host() fails?

When scsi_add_host() runs, it calls scsi_add_host_with_dma(), which exposes
the scan sysfs attribute via device_add():

drivers/scsi/hosts.c:scsi_add_host_with_dma() {
    ...
    error = device_add(&shost->shost_dev);
    if (error)
        goto out_del_gendev;
    ...
}

If a subsequent initialization step inside device_add() fails,
scsi_add_host() will unwind and return an error.

If a concurrent write to the scan attribute happens before the unwind,
scsi_scan_host_selected() can create child SCSI targets. Because
scsi_add_host() does not reap these concurrently created children on its
error path, they remain orphaned.

If we proceed to free the virtqueues here via virtscsi_remove_vqs(),
could a later SCSI error handler invocation such as virtscsi_abort()
dereference the freed virtqueues?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260813084405.610322-1-jaspwise@amazon.co.uk?part=1