Re: [PATCH v4 2/9] scsi: nsp32: don't store pci_device_id

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unvalidated `driver_data` from dynamic PCI IDs causes an out-of-bounds read of the `nsp32_model` array.
- [High] Missing cleanup of PCI resources and MMIO mapping on probe error path, and missing disable on removal.
- [High] Unprotected global `nsp32_data_base` causes race conditions during concurrent device probes.
- [High] Time-of-Check to Time-of-Use (TOCTOU) race fetching `CurrentSC` locklessly in the ISR.
- [High] Returning a positive value instead of a negative error code on probe failure tricks the PCI core into assuming success, causing a subsequent NULL pointer dereference.
--

--- Patch [2]: [PATCH v4 2/9] scsi: nsp32: don't store pci_device_id ---
commit d3fbc37c7e8ddff13dd3c674aa1a8a1d1f511010
Author: Gary Guo <gary@garyguo.net>

scsi: nsp32: don't store pci_device_id

pci_device_id is not guaranteed to live longer than probe due to presence
of dynamic ID. All information apart from driver_data can be easily
retrieved from pci_dev, so just store driver_data.

> diff --git a/drivers/scsi/nsp32.c b/drivers/scsi/nsp32.c
> index e893d56772417..9c9281222a0ac 100644
> --- a/drivers/scsi/nsp32.c
> +++ b/drivers/scsi/nsp32.c
> @@ -1470,7 +1470,7 @@ static int nsp32_show_info(struct seq_file *m, struct Scsi_Host *host)
>  		   (nsp32_read2(base, INDEX_REG) >> 8) & 0xff);
>  
>  	mode_reg = nsp32_index_read1(base, CHIP_MODE);
> -	model    = data->pci_devid->driver_data;
> +	model    = data->model;

[Severity: High]
This isn't a bug introduced by this patch, but does this unvalidated model
value (derived from id->driver_data via sysfs new_id) cause an out-of-bounds
read of the fixed-size nsp32_model array?

If a large value for driver_data is supplied, this happens just below when
reading the procfs file:

nsp32_show_info():
    seq_printf(m, "OEM:                   %ld, %s\n",
           (mode_reg & (OEM0|OEM1)), nsp32_model[model]);

The same out-of-bounds read also appears in nsp32_probe() during logging.

[ ... ]

> @@ -3340,7 +3340,7 @@ static int nsp32_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	}
>  
>  	data->Pci	  = pdev;
> -	data->pci_devid   = id;
> +	data->model       = id->driver_data;

[Severity: High]
This is a pre-existing issue, but does the lockless use of the global
nsp32_data_base struct cause race conditions during concurrent device probes?

nsp32_probe() writes device-specific resources to this shared global struct:

nsp32_probe():
    nsp32_hw_data *data = &nsp32_data_base;
    ...
    data->Pci = pdev;
    data->model = id->driver_data;
    data->MmioAddress = pci_ioremap_bar(pdev, 1);

nsp32_detect() then copies it to the per-device hostdata. If multiple devices
are probed concurrently (e.g., async probe or hotplug), threads might race to
overwrite these fields, causing devices to steal each other's MMIO mappings.

[Severity: High]
This is another pre-existing issue, but doesn't the error path of
nsp32_probe() fail to clean up PCI resources?

If nsp32_detect() fails, nsp32_probe() returns the error without unmapping
data->MmioAddress or calling pci_disable_device(). Also, on the normal
removal path, nsp32_release() unmaps the IO but entirely skips
pci_disable_device().

[Severity: High]
This is also a pre-existing issue, but does nsp32_detect() returning 1
instead of a negative error code on failure cause issues with the PCI core?

nsp32_probe() propagates this positive value, which the PCI core treats as
success in local_pci_probe(), binding the device even though pci_set_drvdata()
was skipped. When the device is later unbound, nsp32_remove() retrieves a
NULL driver data pointer and passes it to scsi_remove_host(), triggering a
NULL pointer dereference.

>  	data->IrqNumber   = pdev->irq;
>  	data->BaseAddress = pci_resource_start(pdev, 0);
>  	data->NumAddress  = pci_resource_len  (pdev, 0);
> diff --git a/drivers/scsi/nsp32.h b/drivers/scsi/nsp32.h
> index 924889f8bd37e..9e65771cb5928 100644
> --- a/drivers/scsi/nsp32.h
> +++ b/drivers/scsi/nsp32.h
> @@ -564,10 +564,10 @@ typedef struct _nsp32_hw_data {
>  
>  	struct scsi_cmnd *CurrentSC;
>  
> -	struct pci_dev             *Pci;
> -	const struct pci_device_id *pci_devid;
> -	struct Scsi_Host           *Host;
> -	spinlock_t                  Lock;
> +	struct pci_dev    *Pci;
> +	int                model;
> +	struct Scsi_Host  *Host;
> +	spinlock_t         Lock;

[Severity: High]
This isn't a bug introduced by this patch, but is there a Time-of-Check to
Time-of-Use (TOCTOU) race condition when fetching CurrentSC in the ISR?

In do_nsp32_isr(), SCpnt = data->CurrentSC is fetched locklessly before
acquiring host->host_lock. 

If the EH thread completes the command concurrently, it frees the DMA mapping,
calls scsi_done(), and sets data->CurrentSC = NULL. The ISR thread might
subsequently acquire the lock and use the stale pre-fetched SCpnt pointer
to modify its result and call nsp32_scsi_done() again, potentially causing a
use-after-free and double completion.

>  
>  	char info_str[100];

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260723-pci_id_fix-v4-0-3580726844e1@garyguo.net?part=2