Re: [PATCH 2/4] scsi: pm8001: clean up resources on probe failure

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Hardware soft reset is initiated before tearing down tasklets and IRQs, allowing concurrent MMIO accesses during reset.
- [High] Workqueue flushed after freeing DMA memory and unmapping MMIO registers, leading to use-after-free and use-after-unmap.
--

commit 701715eef41b511a0a6a9cbd9e7c8e9908bfcfc9
Author: Peng Hao <flyingpenghao@gmail.com>
scsi: pm8001: clean up resources on probe failure

This commit improves resource cleanup on probe failures in the pm8001 driver.
It routes failures through the standard teardown chain and correctly resets
the chip. It also adds logic to release SAS arrays when initialization fails.

> diff --git a/drivers/scsi/pm8001/pm8001_init.c b/drivers/scsi/pm8001/pm8001_init.c
> index d0e91ed09bbe9..20b02e63be8c9 100644
> --- a/drivers/scsi/pm8001/pm8001_init.c
> +++ b/drivers/scsi/pm8001/pm8001_init.c
> [ ... ]
> @@ -1216,8 +1216,16 @@ static int pm8001_pci_probe(struct pci_dev *pdev,
>  
>  err_out_shost:
>  	scsi_remove_host(pm8001_ha->shost);
> +err_out_chip:
> +	PM8001_CHIP_DISP->interrupt_disable(pm8001_ha, 0xFF);
> +	PM8001_CHIP_DISP->chip_soft_rst(pm8001_ha);
>  err_out_ha_free:
> +	pm8001_free_irq(pm8001_ha);
> +	pm8001_kill_tasklet(pm8001_ha);

[Severity: High]
This is a pre-existing issue, but does this ordering allow concurrent MMIO
accesses during the hardware soft reset?

In the newly introduced err_out_chip error path of pm8001_pci_probe(),
chip_soft_rst() is called before pm8001_free_irq() and pm8001_kill_tasklet().
If an interrupt fires immediately before interrupt_disable(), a tasklet can
be scheduled.

This tasklet could then execute concurrently on another CPU during the lengthy
soft reset window, accessing chip MMIO registers via pm8001_chip_isr() and
triggering PCI bus errors like a PCI Master Abort.

Could we tear down the tasklets and IRQs before initiating the soft reset?

>  	pm8001_free(pm8001_ha);

[Severity: High]
This is also a pre-existing issue, but can this lead to a use-after-free
or use-after-unmap if a delayed work item executes?

When pm8001_free() is called here, it unmaps MMIO and frees memory before
flushing the workqueue:

pm8001_free() {
    ...
    dma_free_coherent(...)
    PM8001_CHIP_DISP->chip_iounmap(pm8001_ha);
    flush_workqueue(pm8001_wq);
}

Because interrupts are enabled earlier in pm8001_pci_probe() prior to
pm8001_init_sas_add(), any work item queued by an interrupt during this
window could execute concurrently with or after the unmapping.

The delayed work item could then write to the now-freed DMA memory and access
unmapped MMIO.

Should the workqueue be flushed before freeing DMA memory and unmapping
the MMIO registers?

> +err_out_free_sas_ha:
> +	kfree(sha->sas_phy);
> +	kfree(sha->sas_port);
>  err_out_free:
>  	kfree(sha);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911091011.94676-1-flyingpeng@tencent.com?part=2