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 <[email protected]> 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/[email protected]?part=2
