Re: [PATCH v8 2/6] ata: libata-scsi: convert dev->sdev to per-LUN array

[sashiko-bot@kernel.org]

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

New issues:
- [High] Sleeping in atomic context due to `scsi_device_put()` under spinlock in `ata_scsi_dev_rescan` error path

Pre-existing issues:
- [High] Double-free or reference count underflow of OF device node in `ata_scsi_assign_ofnode`
- [High] Use-After-Free in `ata_port_detach` due to missing reference count before `scsi_remove_device`
--

--- Patch [2]: [PATCH v8 2/6] ata: libata-scsi: convert dev->sdev to per-LUN array ---
commit 576f81319ae7ef4943e02f2cfa1da008a5f4256c
Author: Phil Pemberton <philpem@philpem.me.uk>
Subject: ata: libata-scsi: convert dev->sdev to per-LUN array

This commit replaces the scalar dev->sdev pointer with an array of
scsi_device pointers to support multi-LUN ATAPI devices. Code paths
are updated to iterate over all ATAPI_MAX_LUN slots instead of assuming
a single LUN.

> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 15ee44cf5bf23..43d5221dc347c 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -6381,11 +6381,16 @@ static void ata_port_detach(struct ata_port *ap)
>  	/* Remove scsi devices */
>  	ata_for_each_link(link, ap, HOST_FIRST) {
>  		ata_for_each_dev(dev, link, ALL) {
> -			if (dev->sdev) {
> +			int lun;
> +
> +			for (lun = ATAPI_MAX_LUN - 1; lun >= 0; lun--) {
> +				struct scsi_device *sdev = dev->sdev[lun];
> +				if (!sdev)
> +					continue;
> +				dev->sdev[lun] = NULL;
>  				spin_unlock_irqrestore(ap->lock, flags);
> -				scsi_remove_device(dev->sdev);
> +				scsi_remove_device(sdev);

[Severity: High]
This is a pre-existing issue, but does dropping the lock here without
holding a reference to sdev expose us to a use-after-free?

The commit message mentions that clearing the array slot closes the UAF
window, but if a concurrent sysfs removal drops the final reference
while the lock is released, could scsi_remove_device(sdev) end up
dereferencing freed memory?

Would it be safer to call scsi_device_get(sdev) before dropping the lock?

>  				spin_lock_irqsave(ap->lock, flags);
> -				dev->sdev = NULL;
>  			}
>  		}
>  	}

> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 04c96f3fd8652..808368b952b51 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
[ ... ]
> @@ -5185,7 +5196,7 @@ int ata_scsi_add_hosts(struct ata_host *host, const struct scsi_host_template *s
>  #ifdef CONFIG_OF
>  static void ata_scsi_assign_ofnode(struct ata_device *dev, struct ata_port *ap)
>  {
> -	struct scsi_device *sdev = dev->sdev;
> +	struct scsi_device *sdev = ata_dev_scsi_device(dev, 0);
>  	struct device *d = ap->host->dev;
>  	struct device_node *np = d->of_node;
>  	struct device_node *child;

[Severity: High]
This isn't a bug introduced by this patch, but does
ata_scsi_assign_ofnode() cause a reference count underflow or
double-free on the device node?

If for_each_available_child_of_node() assigns the child pointer to both
dev->tdev.of_node and sdev->sdev_gendev.of_node, we have two pointers
sharing a single acquired reference. When both devices are destroyed,
they will each call of_node_put(), potentially leading to a double-free.

Should we add an of_node_get() for the second assignment?

[ ... ]
> @@ -5534,7 +5542,8 @@ void ata_scsi_dev_rescan(struct work_struct *work)
[ ... ]
>  				ret = scsi_rescan_device(sdevs[lun]);
>  				scsi_device_put(sdevs[lun]);
>  				spin_lock_irqsave(ap->lock, flags);
>  
> -			if (ret)
> -				goto unlock_ap;
> +				if (ret) {
> +					while (++lun < ATAPI_MAX_LUN)
> +						if (sdevs[lun])
> +							scsi_device_put(sdevs[lun]);
> +					goto unlock_ap;
>  				}

[Severity: High]
Can this call to scsi_device_put() sleep while holding the spinlock?

Since this block runs after spin_lock_irqsave(ap->lock, flags) is
called, interrupts are disabled. If a concurrent removal has dropped
other references, this scsi_device_put() call might drop the final
reference and invoke the release handler scsi_device_dev_release().

Since the release handler contains blocking calls like
cancel_work_sync(), could this cause a schedule-while-atomic BUG?

> -			ret = scsi_rescan_device(sdev);
> -			scsi_device_put(sdev);
> -			spin_lock_irqsave(ap->lock, flags);
>  
> -			if (ret)
> -				goto unlock_ap;
> +			if (do_resume)
> +				dev->flags &= ~ATA_DFLAG_RESUMING;
>  		}
>  	}

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