Re: [PATCH v7 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 5 potential issue(s) to consider:

New issues:
- [High] Deadlock caused by calling scsi_device_put() while holding an IRQ-disabled spinlock.
- [High] Infinite retry loop and LUN starvation due to early loop exit in ata_scsi_dev_rescan().
- [Medium] Denial of Service via WARN_ON_ONCE triggered by malicious device LUN.

Pre-existing issues:
- [High] Use-After-Free in ata_port_detach due to unreferenced scsi_device.
- [High] Use-After-Free in ZPODD wake callbacks racing with concurrent sysfs device deletion.
--

commit 49dbde19170ff410a516820c897120bfd39b44b3
Author: Phil Pemberton <philpem@philpem.me.uk>
Subject: ata: libata-scsi: convert dev->sdev to per-LUN array

Multi-LUN ATAPI devices share a single ata_device but expose multiple
scsi_devices. This patch replaces the scalar struct scsi_device *sdev
with a fixed-size array dev->sdev[ATAPI_MAX_LUN] indexed by LUN number,
allowing callers to operate on all populated LUNs.

> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index d39ac4292f818..11f8a341252db 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -6275,11 +6275,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);
>  				spin_lock_irqsave(ap->lock, flags);
> -				dev->sdev = NULL;
>  			}
>  		}
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but does dropping ap->lock without
taking a reference to sdev open a race condition in ata_port_detach() in
drivers/ata/libata-core.c?

If a concurrent sysfs device deletion drops the last reference right after
the lock is released, could scsi_remove_device(sdev) trigger a use-after-free
when it accesses sdev->host to lock the scan mutex?

Would it be safer to use scsi_device_get() before dropping the lock, and
scsi_device_put() after scsi_remove_device()?

> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 32c6a0e497cf9..b65358955cf17 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -1131,7 +1131,9 @@ int ata_scsi_dev_config(struct scsi_device *sdev, struct queue_limits *lim,
>  	if (dev->flags & ATA_DFLAG_TRUSTED)
>  		sdev->security_supported = 1;
>  
> -	dev->sdev = sdev;
> +	if (WARN_ON_ONCE(sdev->lun >= ATAPI_MAX_LUN))
> +		return -EINVAL;
> +	dev->sdev[sdev->lun] = sdev;
>  	return 0;
>  }

[Severity: Medium]
In ata_scsi_dev_config() in drivers/ata/libata-scsi.c, can an external device
provide an untrusted LUN (for example, via a crafted REPORT LUNS response over
USB)?

If so, would this WARN_ON_ONCE crash the machine when panic_on_warn is
enabled?

Should this use ata_dev_warn() or ata_dev_err() instead of WARN_ON_ONCE?

[ ... ]

> @@ -5016,28 +5025,43 @@ void ata_scsi_dev_rescan(struct work_struct *work)
>  			if (ap->pflags & ATA_PFLAG_SUSPENDED)
>  				goto unlock_ap;
>  
> -			if (!sdev)
> -				continue;
> -			if (scsi_device_get(sdev))
> -				continue;
> +			for (lun = 0; lun < ATAPI_MAX_LUN; lun++) {
> +				if (dev->sdev[lun] &&
> +				    !scsi_device_get(dev->sdev[lun]))
> +					sdevs[lun] = dev->sdev[lun];
> +			}
>  
>  			do_resume = dev->flags & ATA_DFLAG_RESUMING;
>  
> -			spin_unlock_irqrestore(ap->lock, flags);
> -			if (do_resume) {
> -				ret = scsi_resume_device(sdev);
> -				if (ret == -EWOULDBLOCK) {
> -					scsi_device_put(sdev);
> -					goto unlock_scan;
> +			for (lun = 0; lun < ATAPI_MAX_LUN; lun++) {
> +				if (!sdevs[lun])
> +					continue;
> +
> +				spin_unlock_irqrestore(ap->lock, flags);
> +				if (do_resume) {
> +					ret = scsi_resume_device(sdevs[lun]);
> +					if (ret == -EWOULDBLOCK) {
> +						scsi_device_put(sdevs[lun]);
> +						while (++lun < ATAPI_MAX_LUN)
> +							if (sdevs[lun])
> +								scsi_device_put(sdevs[lun]);
> +						goto unlock_scan;
> +					}
> +				}
> +				ret = scsi_rescan_device(sdevs[lun]);
> +				scsi_device_put(sdevs[lun]);
> +				spin_lock_irqsave(ap->lock, flags);
> +
> +				if (ret) {
> +					while (++lun < ATAPI_MAX_LUN)
> +						if (sdevs[lun])
> +							scsi_device_put(sdevs[lun]);
> +					goto unlock_ap;
>  				}

[Severity: High]
In ata_scsi_dev_rescan() in drivers/ata/libata-scsi.c, could calling
scsi_device_put() here while holding the IRQ-disabled ap->lock cause a
deadlock?

If this drops the last reference to the device, the synchronous release
function ata_scsi_sdev_destroy() will execute. That function attempts to
acquire ap->lock again, which would cause a hard deadlock on the same CPU
since we just acquired it via spin_lock_irqsave(ap->lock, flags).

[Severity: High]
If scsi_rescan_device() returns an error for an early LUN (such as LUN 0), we
exit the loop early via goto unlock_ap.

Does this skip the dev->flags &= ~ATA_DFLAG_RESUMING clearance that happens
at the end of the loop?

Since the work item reschedules itself on error, would this cause an infinite
loop that repeatedly attempts to scan LUN 0 while permanently starving the
higher LUNs?

> -				dev->flags &= ~ATA_DFLAG_RESUMING;
>  			}
> -			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;
>  		}
>  	}

> diff --git a/drivers/ata/libata-zpodd.c b/drivers/ata/libata-zpodd.c
> index 414e7c63bd850..151ae5726aca1 100644
> --- a/drivers/ata/libata-zpodd.c
> +++ b/drivers/ata/libata-zpodd.c
[ ... ]
> @@ -233,18 +239,27 @@ void zpodd_post_poweron(struct ata_device *dev)
>  	zpodd->zp_sampled = false;
>  	zpodd->zp_ready = false;
>  
> -	sdev_enable_disk_events(dev->sdev);
> +	for (lun = 0; lun < ATAPI_MAX_LUN; lun++) {
> +		struct scsi_device *sdev = dev->sdev[lun];
> +		if (sdev)
> +			sdev_enable_disk_events(sdev);
> +	}
>  }
>  
>  static void zpodd_wake_dev(acpi_handle handle, u32 event, void *context)
>  {
>  	struct ata_device *ata_dev = context;
>  	struct zpodd *zpodd = ata_dev->zpodd;
> -	struct device *dev = &ata_dev->sdev->sdev_gendev;
> +	int lun;
>  
> -	if (event == ACPI_NOTIFY_DEVICE_WAKE && pm_runtime_suspended(dev)) {
> -		zpodd->from_notify = true;
> -		pm_runtime_resume(dev);
> +	if (event != ACPI_NOTIFY_DEVICE_WAKE)
> +		return;
> +	for (lun = 0; lun < ATAPI_MAX_LUN; lun++) {
> +		struct scsi_device *sdev = ata_dev->sdev[lun];
> +		if (sdev && pm_runtime_suspended(&sdev->sdev_gendev)) {

[Severity: High]
This isn't a bug introduced by this patch, but is it safe to iterate and
dereference ata_dev->sdev[lun] here in zpodd_wake_dev() in
drivers/ata/libata-zpodd.c without holding ap->lock or acquiring a reference
via scsi_device_get()?

If a SCSI device is concurrently removed (for example, via a sysfs delete
operation), could the scsi_device structure be freed while the ACPI notify
callback is executing?

Would this lead to a use-after-free when pm_runtime_suspended() is called?

> +			zpodd->from_notify = true;
> +			pm_runtime_resume(&sdev->sdev_gendev);
> +		}
>  	}
>  }

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