Re: [PATCH] scsi: pm8001: unwind the probe on ccb tag allocation failure

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] scsi: pm8001: use-after-free via active IRQs in pm8001_pci_probe()
- [High] scsi: pm8001: DMA regions freed while MPI is active in pm8001_pci_probe()
- [Medium] scsi: pm8001: memory leaks on ccb tag failure unwind in pm8001_pci_probe()
--

commit 06b680982a72c843fb06984b05c4494852d0cf98
Author: Wentao Liang <vulab@iscas.ac.cn>

scsi: pm8001: unwind the probe on ccb tag allocation failure

This commit changes the error handling path in pm8001_pci_probe() when
pm8001_init_ccb_tag() fails, jumping to err_out_ha_free instead of
err_out_enable to release previously allocated resources.

> diff --git a/drivers/scsi/pm8001/pm8001_init.c b/drivers/scsi/pm8001/pm8001_init.c
> index 5af81c73a8f87..389649b36aabd 100644
> --- a/drivers/scsi/pm8001/pm8001_init.c
> +++ b/drivers/scsi/pm8001/pm8001_init.c
> @@ -1178,7 +1178,7 @@ static int pm8001_pci_probe(struct pci_dev *pdev,
>  
>  	rc = pm8001_init_ccb_tag(pm8001_ha);
>  	if (rc)
> -		goto err_out_enable;
> +		goto err_out_ha_free;

[Severity: High]
Can jumping to err_out_ha_free here leave active interrupt handlers
registered with dangling pointers into freed pm8001_ha and sha memory?

Before pm8001_init_ccb_tag() runs, pm8001_pci_probe() calls
pm8001_pci_alloc() -> pm8001_init_tasklet() and pm8001_alloc() ->
pm8001_request_irq().

In MSI-X mode, pm8001_request_msix() registers IRQ handlers pointing into
pm8001_ha:

drivers/scsi/pm8001/pm8001_init.c:pm8001_request_msix() {
	...
		rc = request_irq(pci_irq_vector(pm8001_ha->pdev, i),
			pm8001_interrupt_handler_msix, flag,
			pm8001_ha->intr_drvname[i],
			&(pm8001_ha->irq_vector[i]));
	...
}

In INT-X mode, pm8001_request_irq() registers a shared IRQ handler with
pm8001_ha->name and sha:

drivers/scsi/pm8001/pm8001_init.c:pm8001_request_irq() {
	...
	return request_irq(pdev->irq, pm8001_interrupt_handler_intx,
			   IRQF_SHARED, pm8001_ha->name,
			   SHOST_TO_SAS_HA(pm8001_ha->shost));
}

When pm8001_init_ccb_tag() fails, the err_out_ha_free path calls
pm8001_free(pm8001_ha) and kfree(sha) without calling
pm8001_free_irq(pm8001_ha) or pm8001_kill_tasklet(pm8001_ha) (unlike
pm8001_pci_remove()):

drivers/scsi/pm8001/pm8001_init.c:pm8001_pci_probe() {
	...
err_out_ha_free:
	pm8001_free(pm8001_ha);
err_out_free:
	kfree(sha);
	...
}

If a shared INT-X interrupt fires afterwards, pm8001_interrupt_handler_intx()
will dereference the freed sha and pm8001_ha structures and read unmapped MMIO
registers, and reading /proc/interrupts will dereference the freed devname
strings in pm8001_ha->intr_drvname[i] or pm8001_ha->name.

[Severity: High]
Also on the err_out_ha_free path, does pm8001_free() release coherent DMA
memory while the controller MPI engine is still active?

In pm8001_pci_probe(), PM8001_CHIP_DISP->chip_init(pm8001_ha) succeeds right
before pm8001_init_ccb_tag() is called, which programs the physical DMA
addresses of pm8001_ha->memoryMap.region[] (inbound/outbound queues,
producer/consumer indices, and firmware event logs) into the controller and
enables the firmware MPI state machine via mpi_init_check().

When pm8001_init_ccb_tag() fails and jumps to err_out_ha_free, pm8001_free()
immediately frees those DMA regions without first calling
PM8001_CHIP_DISP->chip_soft_rst(pm8001_ha) to uninitialize the MPI engine and
reset the chip (as pm8001_pci_remove() does before calling pm8001_free()):

drivers/scsi/pm8001/pm8001_init.c:pm8001_free() {
	...
	for (i = 0; i < USI_MAX_MEMCNT; i++) {
		if (pm8001_ha->memoryMap.region[i].virt_ptr != NULL) {
			dma_free_coherent(&pm8001_ha->pdev->dev,
				(pm8001_ha->memoryMap.region[i].total_len +
				pm8001_ha->memoryMap.region[i].alignment),
				pm8001_ha->memoryMap.region[i].virt_ptr,
				pm8001_ha->memoryMap.region[i].phys_addr);
			}
	}
	PM8001_CHIP_DISP->chip_iounmap(pm8001_ha);
	...
}

Should chip_soft_rst() be called before pm8001_free() so the controller cannot
DMA into the freed memoryMap.region[] buffers?

[Severity: Medium]
Finally, does unwinding to err_out_ha_free on pm8001_init_ccb_tag() failure
still leak sha->sas_phy, sha->sas_port, pm8001_ha->devices,
pm8001_ha->ccb_info, and partially allocated pm8001_ha->ccb_info[i].buf_prd
DMA buffers?

Inside pm8001_init_ccb_tag(), if bitmap_zalloc() for pm8001_ha->rsvd_tags
fails, or if dma_alloc_coherent() for pm8001_ha->ccb_info[i].buf_prd fails at
iteration i, it jumps to err_out:

drivers/scsi/pm8001/pm8001_init.c:pm8001_init_ccb_tag() {
	...
	pm8001_ha->rsvd_tags = bitmap_zalloc(PM8001_RESERVE_SLOT, GFP_KERNEL);
	if (!pm8001_ha->rsvd_tags)
		goto err_out;
	...
	for (i = 0; i < ccb_count; i++) {
		pm8001_ha->ccb_info[i].buf_prd = dma_alloc_coherent(dev,
				sizeof(struct pm8001_prd) * PM8001_MAX_DMA_SG,
				&pm8001_ha->ccb_info[i].ccb_dma_handle,
				GFP_KERNEL);
		if (!pm8001_ha->ccb_info[i].buf_prd) {
			pm8001_dbg(pm8001_ha, FAIL,
				   "ccb prd memory allocation error\n");
			goto err_out;
		}
		...
	}

	return 0;

err_out_noccb:
	kfree(pm8001_ha->devices);
err_out:
	return -ENOMEM;
}

Jumping to err_out bypasses kfree(pm8001_ha->devices) (allocated in
pm8001_alloc()) and leaves pm8001_ha->ccb_info and any already allocated
pm8001_ha->ccb_info[0..i-1].buf_prd buffers unfreed. Neither pm8001_free() nor
err_out_free frees those allocations, nor do they free sha->sas_phy and
sha->sas_port (allocated in pm8001_prep_sas_ha_init()) before kfree(sha) is
called.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104021411.1467970-1-vulab@iscas.ac.cn?part=1