Re: [PATCH v7 6/6] scsi: scsi_devinfo: add COMPAQ PD-1 multi-LUN ATAPI device quirk

Re: [PATCH v7 6/6] scsi: scsi_devinfo: add COMPAQ PD-1 multi-LUN ATAPI device quirk

[sashiko-bot]

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

Pre-existing issues:
- [High] Max-length vendor (8 characters) and model (16 characters) static quirks are silently ignored due to forced null-termination by `strscpy_pad`.
--

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

scsi: scsi_devinfo: add COMPAQ PD-1 multi-LUN ATAPI device quirk

The Compaq PD-1 (and equivalent Panasonic LF-1195C) is a combination
PD/CD-ROM drive that exposes two LUNs: LUN 0 is the CD-ROM and LUN 1
is the PD (Phase-change rewritable) drive.

Add a scsi_devinfo entry with BLIST_FORCELUN to enable multi-LUN
scanning, BLIST_SINGLELUN to prevent issuing LUN-aware commands
simultaneously, and BLIST_NO_LUN_1F to suppress spurious "No Device"
entries for unpopulated LUNs (which respond with PQ=0/PDT=0x1f).

> diff --git a/drivers/scsi/scsi_devinfo.c b/drivers/scsi/scsi_devinfo.c
> index 68a992494b121..bfc2cbd438975 100644
> --- a/drivers/scsi/scsi_devinfo.c
> +++ b/drivers/scsi/scsi_devinfo.c
> @@ -150,6 +150,8 @@ static struct {
>  	{"COMPAQ", "MSA1000", NULL, BLIST_SPARSELUN | BLIST_NOSTARTONADD},
>  	{"COMPAQ", "MSA1000 VOLUME", NULL, BLIST_SPARSELUN | BLIST_NOSTARTONADD},
>  	{"COMPAQ", "HSV110", NULL, BLIST_REPORTLUN2 | BLIST_NOSTARTONADD},
> +	{"COMPAQ", "PD-1", NULL, BLIST_FORCELUN | BLIST_SINGLELUN |
> +				 BLIST_NO_LUN_1F},
[Severity: High]
This isn't a bug introduced by this patch, but max-length vendor (8
characters) and model (16 characters) static quirks in this list are
silently ignored due to forced null-termination by strscpy_pad().

While this specific COMPAQ quirk is short enough to avoid the issue, the use
of strscpy_pad() in scsi_strcpy_devinfo() forces a null-terminator for
string lengths exactly equal to the maximum array size:

drivers/scsi/scsi_devinfo.c:scsi_strcpy_devinfo() {
	...
	strscpy_pad(to, from, to_length);
	...
}

If a vendor string is 8 characters long (e.g. MATSHITA, transtec) or a model
is 16 characters long (e.g. ARRAY CONTROLLER, ImageMate CF-SD1),
strscpy_pad() truncates the string to leave room for the null byte.

In scsi_dev_info_list_find(), the matching logic compares the length of the
device's INQUIRY data (vmax) against strnlen(devinfo->vendor, 8):

drivers/scsi/scsi_devinfo.c:scsi_dev_info_list_find() {
	...
	if (vmax != strnlen(devinfo->vendor,
			    sizeof(devinfo->vendor)) ||
	...
}

Since vmax is 8 but strnlen() returns 7 for truncated strings, the length
comparison fails. This silently breaks critical SCSI quirks. Without these
quirks, devices like the Compaq ARRAY CONTROLLER fail to report LUNs
correctly (leading to missing disks or unbootable systems), or devices could
lock up the SCSI bus.

Could the truncation logic be revised to safely handle exact length strings
without null-termination?

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

Re: [PATCH v7 6/6] scsi: scsi_devinfo: add COMPAQ PD-1 multi-LUN ATAPI device quirk

[Hannes Reinecke]

On 6/11/26 04:43, Phil Pemberton wrote:
> The Compaq PD-1 (and equivalent Panasonic LF-1195C) is a combination
> PD/CD-ROM drive that exposes two LUNs: LUN 0 is the CD-ROM and LUN 1
> is the PD (Phase-change rewritable) drive.
> 
> Add a scsi_devinfo entry with BLIST_FORCELUN to enable multi-LUN
> scanning, BLIST_SINGLELUN to prevent issuing LUN-aware commands
> simultaneously, and BLIST_NO_LUN_1F to suppress spurious "No Device"
> entries for unpopulated LUNs (which respond with PQ=0/PDT=0x1f).
> 
> Signed-off-by: Phil Pemberton <philpem@philpem.me.uk>
> ---
>   drivers/scsi/scsi_devinfo.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/scsi/scsi_devinfo.c b/drivers/scsi/scsi_devinfo.c
> index 68a992494b12..bfc2cbd43897 100644
> --- a/drivers/scsi/scsi_devinfo.c
> +++ b/drivers/scsi/scsi_devinfo.c
> @@ -150,6 +150,8 @@ static struct {
>   	{"COMPAQ", "MSA1000", NULL, BLIST_SPARSELUN | BLIST_NOSTARTONADD},
>   	{"COMPAQ", "MSA1000 VOLUME", NULL, BLIST_SPARSELUN | BLIST_NOSTARTONADD},
>   	{"COMPAQ", "HSV110", NULL, BLIST_REPORTLUN2 | BLIST_NOSTARTONADD},
> +	{"COMPAQ", "PD-1", NULL, BLIST_FORCELUN | BLIST_SINGLELUN |
> +				 BLIST_NO_LUN_1F},
>   	{"DDN", "SAN DataDirector", "*", BLIST_SPARSELUN},
>   	{"DEC", "HSG80", NULL, BLIST_REPORTLUN2 | BLIST_NOSTARTONADD},
>   	{"DELL", "PV660F", NULL, BLIST_SPARSELUN},

Reviewed-by: Hannes Reinecke <hare@kernel.org>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

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

[sashiko-bot]

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

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

[Hannes Reinecke]

On 6/11/26 04:43, Phil Pemberton wrote:
> Multi-LUN ATAPI devices (PD/CD combos, CD changers) share a single
> ata_device but expose multiple scsi_devices.  The previous single
> dev->sdev pointer could only track one LUN, making all other LUNs
> invisible to code that operates on sdevs: port detach, suspend/resume,
> ACPI uevent, ZPODD, media change notification, and EH teardown.
> 
> Replace the scalar struct scsi_device *sdev with a fixed-size array
> dev->sdev[ATAPI_MAX_LUN] indexed by LUN number, where ATAPI_MAX_LUN
> is 8 (the SCSI-2 ceiling, LUN values 0..7).  All callers are updated
> to iterate the full array and skip NULL slots; only populated LUN slots
> are ever non-NULL so single-LUN devices (the vast majority) see no
> change in behaviour.
> 
> Add an inline helper ata_dev_scsi_device(dev, lun) that returns
> dev->sdev[lun] guarded by a WARN_ON_ONCE(lun >= ATAPI_MAX_LUN) bounds
> check.  Use it for the hardcoded LUN-0 references in libata-acpi
> (uevent kobj), libata-zpodd (disk events, wake notify for all LUNs),
> and the door-lock and OF-node paths in libata-scsi.
> 
> Key changes per call site:
>    - ata_scsi_dev_config:    bounds-check lun, assign sdev to dev->sdev[sdev->lun]
>    - ata_scsi_sdev_destroy:  clear per-LUN slot; trigger ATA detach only
>                              when the last populated LUN is destroyed
>    - ata_port_detach:        iterate all ATAPI_MAX_LUN slots descending;
>                              clear dev->sdev[lun] before unlock to close
>                              the UAF window (Hannes Reinecke)
>    - ata_scsi_offline_dev:   iterate all slots
>    - ata_scsi_remove_dev:    snapshot all LUN slots then remove outside lock
>    - ata_scsi_media_change_notify: send event to all populated LUNs
>    - ata_scsi_dev_rescan:    snapshot all LUNs under lock, then resume and
>                              rescan each; release remaining refs on early exit
>    - ACPI, ZPODD, door-lock: use ata_dev_scsi_device(dev, 0)
>    - ZPODD disk-events:      iterate all LUNs for enable/disable and wake
> 
> Signed-off-by: Phil Pemberton <philpem@philpem.me.uk>
> ---
>   drivers/ata/libata-acpi.c  |   9 +-
>   drivers/ata/libata-core.c  |  11 ++-
>   drivers/ata/libata-scsi.c  | 164 +++++++++++++++++++++----------------
>   drivers/ata/libata-zpodd.c |  27 ++++--
>   include/linux/libata.h     |  10 ++-
>   5 files changed, 138 insertions(+), 83 deletions(-)
> 

Reviewed-by: Hannes Reinecke <hare@kernel.org>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich