Re: [PATCH v6 5/6] ata: libata-scsi: probe additional LUNs for multi-LUN ATAPI devices

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Moving `ata_scsi_assign_ofnode()` after `scsi_device_put()` introduces a TOCTOU race leading to a NULL pointer dereference or Use-After-Free.
- [High] Hot-unplugging a multi-LUN ATAPI device fails to detach and free LUNs > 0, resulting in memory leaks and zombie SCSI devices.
--

commit 6a6b71770a262841f2823d6eb0dbe90bb0dcb456
Author: Phil Pemberton <philpem@philpem.me.uk>
ata: libata-scsi: probe additional LUNs for multi-LUN ATAPI devices

After LUN 0 is added for an ATAPI device, this commit checks the BLIST_FORCELUN
flag. If set, it bumps the LUN count to the host's maximum and triggers a
sequential SCSI scan for that target. Non-responding LUNs are silently skipped.

> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 2d714efc855f0..a6f5557014c70 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
[ ... ]
> @@ -4745,12 +4746,30 @@ void ata_scsi_scan_host(struct ata_port *ap, int sync)
>  			sdev = __scsi_add_device(ap->scsi_host, channel, id, 0,
>  						 NULL);
>  			if (!IS_ERR(sdev)) {
> -				dev->sdev[0] = sdev;
> -				ata_scsi_assign_ofnode(dev, ap);
> +				/*
> +				 * For multi-LUN ATAPI (BLIST_FORCELUN), bump
> +				 * dev->nr_luns to the host max so the LUN
> +				 * routing in atapi_xlat() accepts the probe
> +				 * INQUIRYs to LUN > 0, then trigger the
> +				 * sequential scan.  pdt_1f_for_no_lun, set
> +				 * during LUN 0 configure, ensures
> +				 * non-responding LUNs are silently skipped;
> +				 * dev->sdev[] is populated by
> +				 * ata_scsi_dev_config() during the scan.
> +				 */
> +				if (dev->class == ATA_DEV_ATAPI &&
> +				    sdev->sdev_bflags & BLIST_FORCELUN) {
> +					dev->nr_luns = ap->scsi_host->max_lun;

[Severity: High]
Does bumping dev->nr_luns here cause a memory leak during a device
hot-unplug or fatal error reset?

During those events, the libata error handler synchronously calls
ata_eh_detach_dev() followed by ata_dev_init(), which unconditionally resets
dev->nr_luns = 1 without clearing the dev->sdev[] array.

When the asynchronous SCSI hotplug task later calls ata_scsi_remove_dev(),
it bounds its cleanup loop using the newly reset count:

for (lun = dev->nr_luns - 1; lun >= 0; lun--) { ... }

Since dev->nr_luns was reset to 1, any active SCSI devices populated in
dev->sdev[1..7] appear to be skipped. Could this permanently leak the
scsi_device structures and leave unresponsive zombie block devices exposed
to userspace?

> +					scsi_scan_target(
> +						&ap->scsi_host->shost_gendev,
> +						channel, id, SCAN_WILD_CARD,
> +						SCSI_SCAN_RESCAN);
> +				}
>  				scsi_device_put(sdev);
> -			} else {
> -				dev->sdev[0] = NULL;
>  			}
> +
> +			if (dev->sdev[0])
> +				ata_scsi_assign_ofnode(dev, ap);

[Severity: Critical]
Is there a race condition here between dropping the local reference and
checking dev->sdev[0]?

By calling scsi_device_put(sdev) before checking dev->sdev[0] and executing
ata_scsi_assign_ofnode(dev, ap), the object's lifetime relies solely on sysfs
since shost->scan_mutex is not held here.

If a concurrent device removal happens (such as userspace writing to the
delete attribute), the concurrent path executes:

sdev_store_delete()
  scsi_remove_device()
    ata_scsi_sdev_destroy()

This path sets dev->sdev[0] = NULL and drops the sysfs reference.

If this concurrent sysfs removal happens exactly between the unprotected
if (dev->sdev[0]) check and its use inside ata_scsi_assign_ofnode(), could
it result in a NULL pointer dereference, or a Use-After-Free if the object
is freed before accessing sdev->sdev_gendev.of_node?

>  		}
>  	}
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608213443.2296614-1-philpem@philpem.me.uk?part=5