Re: [PATCH v9 07/11] ata: libata-scsi: convert dev->sdev to per-LUN array

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] ata: UAF in ata_acpi_uevent() accessing freed scsi_device kobject
- [Medium] of: memory leak in ata_scsi_assign_ofnode() due to missing of_node_put()
- [Medium] ata: libata-zpodd: stale from_notify flag in zpodd_wake_dev() causes unintended tray ejection
- [Medium] ata: redundant scsi_resume_device() calls on already-resumed LUNs due to shared device flag
--

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

ata: libata-scsi: convert dev->sdev to per-LUN array

Multi-LUN ATAPI devices share one ata_device and expose several SCSI
devices. Replace dev->sdev with an eight-slot array indexed by LUN and
update its callers. Single-LUN devices use slot 0; unused slots are NULL.

> diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
> index 4433f626246ba..2d1662f6f0642 100644
> --- a/drivers/ata/libata-acpi.c
> +++ b/drivers/ata/libata-acpi.c
> @@ -153,10 +153,13 @@ static void ata_acpi_uevent(struct ata_port *ap, struct ata_device *dev,
>  	char *envp[] = { event_string, NULL };
>  
>  	if (dev) {
> -		if (dev->sdev)
> -			kobj = &dev->sdev->sdev_gendev.kobj;
> -	} else
> +		struct scsi_device *sdev = ata_dev_scsi_device(dev, 0);
> +
> +		if (sdev)
> +			kobj = &sdev->sdev_gendev.kobj;

[Severity: High]
Is this scsi_device kobject safe from being freed during this call?

The scsi_device is retrieved via ata_dev_scsi_device() without taking
ap->lock or acquiring a reference with scsi_device_get(). A concurrent
unplug could drop the reference count and free the device object before
kobject_uevent_env() executes, leading to a use-after-free.

> +	} else {
>  		kobj = &ap->dev->kobj;
> +	}
>  
>  	if (kobj) {

[ ... ]

> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 3131d84204ed7..68d6ca1f791dd 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c

[ ... ]

> @@ -5259,9 +5270,9 @@ 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)
> +static void ata_scsi_assign_ofnode(struct scsi_device *sdev,
> +				   struct ata_device *dev, struct ata_port *ap)
>  {

[Severity: Medium]
Does ata_scsi_assign_ofnode() leak a device tree node reference?

When for_each_available_child_of_node() inside this function finds a
matching port, it assigns the child to sdev->sdev_gendev.of_node and
returns early. The iterator implicitly acquires a reference to the child
node on each iteration. Since the SCSI and driver cores do not
automatically call of_node_put() on the device of_node during teardown,
this leaves a persistent reference leak.

> -	struct scsi_device *sdev = dev->sdev;
>  	struct device *d = ap->host->dev;
>  	struct device_node *np = d->of_node;
>  	struct device_node *child;

[ ... ]

> @@ -5610,7 +5619,9 @@ void ata_scsi_dev_rescan(struct work_struct *work)
>  
>  	ata_for_each_link(link, ap, EDGE) {
>  		ata_for_each_dev(dev, link, ENABLED) {
> -			struct scsi_device *sdev = dev->sdev;
> +			struct scsi_device *sdevs[ATAPI_MAX_LUN] = {};
> +			bool retry = false;
> +			int lun, err;
>  
>  			/*
>  			 * If the port was suspended before this was scheduled,

[ ... ]

> -			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) {
> +					err = scsi_resume_device(sdevs[lun]);
> +					if (err == -EWOULDBLOCK)
> +						goto put_sdev;
>  				}
> -				dev->flags &= ~ATA_DFLAG_RESUMING;
> +				err = scsi_rescan_device(sdevs[lun]);

[Severity: Medium]
Will this cause redundant resume attempts on already-resumed LUNs?

When do_resume is true, both scsi_resume_device() and scsi_rescan_device()
execute for every valid LUN without checking if that specific LUN was
already successfully resumed in a previous pass.

> +put_sdev:
> +				/* A deferred LUN must not prevent scanning its siblings. */
> +				if (err) {
> +					ret = err;
> +					retry = true;
> +				}
> +				scsi_device_put(sdevs[lun]);
> +				spin_lock_irqsave(ap->lock, flags);
>  			}
> -			ret = scsi_rescan_device(sdev);
> -			scsi_device_put(sdev);
> -			spin_lock_irqsave(ap->lock, flags);
>  
> -			if (ret)
> -				goto unlock_ap;
> +			if (do_resume && !retry)
> +				dev->flags &= ~ATA_DFLAG_RESUMING;

[Severity: Medium]
If any LUN sets retry to true by returning -EWOULDBLOCK, the per-device
ATA_DFLAG_RESUMING flag is kept set. Consequently, the next execution of
the workqueue will see do_resume as true and unconditionally re-resume all
sibling LUNs, even the ones that previously succeeded.

>  		}
>  	}

[ ... ]

> diff --git a/drivers/ata/libata-zpodd.c b/drivers/ata/libata-zpodd.c
> index 581eb41ea0f3c..bbdcaaa357980 100644
> --- a/drivers/ata/libata-zpodd.c
> +++ b/drivers/ata/libata-zpodd.c

[ ... ]

> @@ -265,18 +272,21 @@ 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 scsi_device *sdev;
> +	int lun;
>  
>  	if (event != ACPI_NOTIFY_DEVICE_WAKE)
>  		return;
> -	sdev = zpodd_get_sdev(ata_dev);
> -	if (!sdev)
> -		return;
> -	if (pm_runtime_suspended(&sdev->sdev_gendev)) {
> -		zpodd->from_notify = true;
> -		pm_runtime_resume(&sdev->sdev_gendev);
> +	for (lun = 0; lun < ATAPI_MAX_LUN; lun++) {
> +		struct scsi_device *sdev = zpodd_get_sdev(ata_dev, lun);
> +
> +		if (!sdev)
> +			continue;
> +		if (pm_runtime_suspended(&sdev->sdev_gendev)) {
> +			zpodd->from_notify = true;
> +			pm_runtime_resume(&sdev->sdev_gendev);
> +		}

[Severity: Medium]
Can the from_notify flag become stuck in a true state here?

Setting this per-ATA-device flag inside a per-LUN loop allows the flag to
be incorrectly left true. If the first suspended LUN causes an actual
resume, from_notify is set and then cleared in zpodd_post_poweron() when
EH runs. For subsequent suspended LUNs, from_notify is set to true again,
but pm_runtime_resume() finds the parent already active and does not
trigger EH. This leaves from_notify set to true, which will cause an
unintended physical tray ejection during the next power-on.

> +		scsi_device_put(sdev);
>  	}
> -	scsi_device_put(sdev);
>  }

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