Re: [PATCH v5 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:
- [Critical] Out-of-bounds array write in `ata_scsi_dev_config()` and `ata_scsi_sdev_destroy()` via unvalidated `sdev->lun` index.
- [High] Semantic error in `atapi_qc_complete()` clears `locked` flag for LUN 0 on failures originating from other LUNs.
- [High] TOCTOU race condition in `ata_port_detach()` leading to NULL pointer dereference or Use-After-Free.
--

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

This commit converts the single dev->sdev pointer into an array to support
multi-LUN ATAPI devices. It replaces hardcoded single-LUN logic with loops
over valid LUN slots and adds helpers to safely access the per-LUN SCSI
devices.

> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 4408b1fb48c77..1cb159d9dbc75 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
[ ... ]
> @@ -6275,11 +6276,15 @@ 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 = dev->nr_luns - 1; lun >= 0; lun--) {
> +				if (!dev->sdev[lun])
> +					continue;
>  				spin_unlock_irqrestore(ap->lock, flags);
> -				scsi_remove_device(dev->sdev);
> +				scsi_remove_device(dev->sdev[lun]);

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

The argument dev->sdev[lun] is evaluated after the lock is released. If a
concurrent sysfs device removal invokes ata_scsi_sdev_destroy(), it will
acquire the lock and set dev->sdev[lun] = NULL.

When the original thread resumes, it might pass NULL to scsi_remove_device(),
which unconditionally dereferences sdev->host. Even if it reads a non-NULL
pointer, no reference is held via scsi_device_get(), so the device could be
freed concurrently.

Should the pointer be cached and refcounted under the lock, similarly to how
it is done in ata_scsi_remove_dev()?

>  				spin_lock_irqsave(ap->lock, flags);
> -				dev->sdev = NULL;
> +				dev->sdev[lun] = NULL;
>  			}
>  		}
>  	}
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 32c6a0e497cf9..7c3d31dc49a17 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
[ ... ]
> @@ -1131,7 +1131,7 @@ 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;
> +	dev->sdev[sdev->lun] = sdev;

Could this cause an out-of-bounds write?

The dev->sdev array has a fixed size of ATAPI_MAX_LUN (8), so valid indices
are 0 to 7. The SCSI layer's shost->max_lun is 8, and scsi_report_lun_scan()
checks if (lun > sdev->host->max_lun), which allows lun == 8.

If an ATAPI device returns LUN 8, could dev->sdev[8] = sdev overwrite
dev->nr_luns, leading to further stack buffer overflows during teardown paths
that use dev->nr_luns?

>  	return 0;
>  }
>  
[ ... ]
> @@ -1220,11 +1220,12 @@ void ata_scsi_sdev_destroy(struct scsi_device *sdev)
>  
>  	spin_lock_irqsave(ap->lock, flags);
>  	dev = __ata_scsi_find_dev(ap, sdev);
> -	if (dev && dev->sdev) {
> -		/* SCSI device already in CANCEL state, no need to offline it */
> -		dev->sdev = NULL;
> -		dev->flags |= ATA_DFLAG_DETACH;
> -		ata_port_schedule_eh(ap);
> +	if (dev && dev->sdev[sdev->lun] == sdev) {

Could this also cause an out-of-bounds access if sdev->lun is 8?

> +		dev->sdev[sdev->lun] = NULL;
> +		if (sdev->lun == 0) {
> +			dev->flags |= ATA_DFLAG_DETACH;
> +			ata_port_schedule_eh(ap);
> +		}
>  	}
>  	spin_unlock_irqrestore(ap->lock, flags);
>  
[ ... ]
> @@ -2911,10 +2912,15 @@ static void atapi_qc_complete(struct ata_queued_cmd *qc)
>  		 * avoid this infinite loop.
>  		 *
>  		 * This may happen before SCSI scan is complete.  Make
> -		 * sure qc->dev->sdev isn't NULL before dereferencing.
> +		 * sure the LUN-0 sdev isn't NULL before dereferencing.
>  		 */
> -		if (qc->cdb[0] == ALLOW_MEDIUM_REMOVAL && qc->dev->sdev)
> -			qc->dev->sdev->locked = 0;
> +		if (qc->cdb[0] == ALLOW_MEDIUM_REMOVAL) {
> +			struct scsi_device *sdev =
> +				ata_dev_scsi_device(qc->dev, 0);
> +
> +			if (sdev)
> +				sdev->locked = 0;
> +		}

Does this clear the locked flag for LUN 0 even if the failed command was
issued to a different LUN?

If the command was issued to LUN 1, its locked flag would remain set, which
could cause an infinite retry loop in SCSI EH for multi-LUN devices.

Should this use qc->scsicmd->device->locked = 0 instead to clear the flag on
the actual target device?

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