Re: [PATCH v2 2/7] nsp32: don't keep pci_device_id

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

Pre-existing issues:
- [Critical] Probe failure incorrectly treated as success leading to NULL pointer dereferences.
- [High] Unvalidated `driver_data` from dynamic PCI IDs causes out-of-bounds reads on the `nsp32_model` array.
- [High] Race condition in `nsp32_probe` due to use of a shared global variable for instance data.
- [High] Double endianness swap in MMIO read/write functions.
--

commit 68e40f6c7308e7ebef3fc97b6095ad0f3e42988d
Author: Gary Guo <[email protected]>

nsp32: don't keep 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

[ ... ]

> @@ -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 I have a question about how driver_data
is used.

If a user with root privileges writes a dynamic PCI ID to
/sys/bus/pci/drivers/nsp32/new_id with an arbitrary, large driver_data
value, the driver unconditionally uses this value as an index into the
7-element nsp32_model string array here in nsp32_probe() via
id->driver_data, and later in nsp32_show_info().

Could this unvalidated driver_data from dynamic PCI IDs cause an
out-of-bounds read?


[Severity: High]
This isn't a bug introduced by this patch, but I have a concern about
the global nsp32_data_base variable.

If two nsp32 PCI devices are probed concurrently, for example via concurrent
sysfs bind operations, both threads will concurrently modify the single
global static variable nsp32_data_base at the start of nsp32_probe() before
it is copied into the per-device host structure in nsp32_detect().

Does using this shared global variable for instance data create a race
condition in nsp32_probe()?


[Severity: Critical]
This is a pre-existing issue, but there might be an issue with the error
handling path.

If nsp32_detect() fails, it returns 1, and nsp32_probe() blindly propagates
this positive value to the PCI core. The PCI core treats positive return
values from probe() as a success, meaning the device successfully binds to
the driver even though initialization completely failed.

Because pci_set_drvdata() is not called on the error path, subsequent driver
callbacks like nsp32_remove() will receive a NULL pointer from
pci_get_drvdata(), causing a NULL pointer dereference.

Does nsp32_probe() incorrectly treat probe failures as success?

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

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but I noticed something in the
MMIO accessors.

In drivers/scsi/nsp32_io.h, nsp32_mmio_write4() explicitly uses cpu_to_le32()
in combination with writel(), and nsp32_mmio_read4() uses le32_to_cpu() with
readl(). Because writel() and readl() intrinsically perform the correct
host-to-PCI byte swapping, manually adding the byte-swap macros results in a
double byte-swap.

Do the MMIO read and write functions perform a double endianness swap on
big-endian architectures?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2