Re: [PATCH v5 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 1 potential issue(s) to consider:
- [High] Moving scsi_device_put(sdev) before ata_scsi_assign_ofnode(dev, ap) introduces a TOCTOU race condition that can result in a Use-After-Free or NULL pointer dereference.
--

commit cc077598d05a5231fc7b5c24f7dca060535cfe72
Author: Phil Pemberton <philpem@philpem.me.uk>

ata: libata-scsi: probe additional LUNs for multi-LUN ATAPI devices

This commit updates libata-scsi to check the BLIST_FORCELUN flag after LUN 0
is added for an ATAPI device. If set, it temporarily bumps dev->nr_luns to
the host's max_lun and triggers a sequential SCSI scan to probe additional
LUNs.

> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> --- 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)) {

[ ... ]

> +				if (dev->class == ATA_DEV_ATAPI &&
> +				    sdev->sdev_bflags & BLIST_FORCELUN) {
> +					dev->nr_luns = ap->scsi_host->max_lun;
> +					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);

Can this sequence cause a use-after-free or a NULL pointer dereference?

By moving ata_scsi_assign_ofnode() outside of the !IS_ERR(sdev) block and
after scsi_device_put(), the local reference to sdev is dropped before it
is accessed.

Because ata_scsi_scan_host() does not hold shost->scan_mutex, a concurrent
device removal via sysfs could execute as soon as the local reference is
dropped.

If scsi_remove_device() asynchronously clears dev->sdev[0] and frees the
sdev structure:

CPU1
ata_scsi_scan_host()
    scsi_device_put(sdev);

CPU2
scsi_remove_device()
    dev->sdev[0] = NULL;
    // sdev is freed

CPU1
    if (dev->sdev[0])
        ata_scsi_assign_ofnode(dev, ap);

Could CPU1 evaluate the if statement before the pointer is cleared, but
execute ata_scsi_assign_ofnode() after it is freed, resulting in a write
to freed memory? 

Alternatively, if it reads dev->sdev[0] after the pointer is cleared,
could this result in a NULL pointer dereference?

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