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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unvalidated `driver_data` from PCI dynamic IDs can cause an out-of-bounds array access on `nsp32_model`.
- [High] Global structure `nsp32_data_base` is used for passing probe state, causing data races on parallel probing.
- [High] A failure in `nsp32_detect()` returns 1, causing `nsp32_probe()` to report success. This leads to a NULL pointer dereference in `nsp32_remove()` and memory leaks.
--

commit 96fc8cc24426c19e6b0431d668e3217eed8887c7
Author: Gary Guo <gary@garyguo.net>

nsp32: don't store pci_device_id

This commit removes the storage of pci_device_id in the nsp32 hardware
data structure, as it may not outlive the probe function when dynamic IDs
are used. Instead, it directly stores the driver_data which is the only
information needed later.

> 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 lead to an out-of-bounds read?

If a privileged user writes a dynamic ID via the new_id sysfs interface
and passes a driver_data value greater than or equal to 7, it seems this
could read outside the bounds of nsp32_model[] when printing further down
in nsp32_show_info() or nsp32_probe():

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

[ ... ]
> @@ -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 isn't a bug introduced by this patch, but does writing to data here
cause a data race if multiple devices matching the driver are probed
concurrently?

Looking at nsp32_probe(), data points to the static global
nsp32_data_base structure:

    nsp32_hw_data *data = &nsp32_data_base;

Since multiple probes will write to this shared structure before it is copied
to the per-instance allocation in nsp32_detect(), could this corrupt
device-specific state like MMIO addresses, IRQs, and PCI device pointers?

[Severity: High]
This isn't a bug introduced by this patch, but could this probe function
return a false success on failure, leading to a NULL pointer dereference
on removal?

If nsp32_detect() fails and returns 1, nsp32_probe() propagates this
positive value. The PCI driver core treats positive returns as success and
binds the device without setting the driver data.

Later, when the device is unbound, nsp32_remove() calls pci_get_drvdata(),
receives NULL, and passes it to scsi_remove_host(host). Because
scsi_remove_host() dereferences its argument unconditionally, wouldn't
this cause a kernel panic?

>  	data->IrqNumber   = pdev->irq;
>  	data->BaseAddress = pci_resource_start(pdev, 0);
>  	data->NumAddress  = pci_resource_len  (pdev, 0);

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706-pci_id_fix-v3-0-2d48fc025acc@garyguo.net?part=2