[PATCH] scsi: arcmsr: fix DMA memory and IO queue leak on arcmsr_alloc_ccb_pool() failure

[PATCH] scsi: arcmsr: fix DMA memory and IO queue leak on arcmsr_alloc_ccb_pool() failure

[Ivy Lopez]

If arcmsr_alloc_ccb_pool() fails in arcmsr_probe(), the error path
jumps directly to unmap_pci_region, bypassing the free_ccb_pool
label entirely. This leaks the CCB pool DMA allocation
(acb->dma_coherent) whenever arcmsr_alloc_ccb_pool() fails after
successfully allocating it, e.g. when the later
arcmsr_alloc_xor_buffer() call fails.

For ACB_ADAPTER_TYPE_F adapters specifically, the IO queue allocated
earlier by arcmsr_alloc_io_queue() is also never freed on this path:
other adapter types free their IO queue before calling
arcmsr_alloc_ccb_pool() (since its space is folded into the CCB
pool's own allocation), but type F does not, so it remains allocated
and leaked if arcmsr_alloc_ccb_pool() subsequently fails.

Fix this by routing arcmsr_alloc_ccb_pool() failure through the
existing free_ccb_pool label, and explicitly freeing the IO queue
for type F adapters beforehand. arcmsr_free_ccb_pool() is safe to
call here regardless of how far arcmsr_alloc_ccb_pool() got:
dma_free_coherent() is a no-op on a NULL address, and acb is
zero-initialized via scsi_host_alloc()'s kzalloc(), so
acb->dma_coherent and acb->xor_mega are both safely zero if
arcmsr_alloc_ccb_pool() failed before setting them.

Found via review feedback on a related fix (commit afa04c2998e4,
"scsi: arcmsr: fix NULL deref on dma_alloc_coherent() failure in
arcmsr_alloc_xor_buffer()").

Signed-off-by: Ivy Lopez <skunkolee@gmail.com>
Cc: ching Huang <ching2048@areca.com.tw>
---
 drivers/scsi/arcmsr/arcmsr_hba.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/arcmsr/arcmsr_hba.c b/drivers/scsi/arcmsr/arcmsr_hba.c
index 40407b281839..d8e56d7b1906 100644
--- a/drivers/scsi/arcmsr/arcmsr_hba.c
+++ b/drivers/scsi/arcmsr/arcmsr_hba.c
@@ -1150,7 +1150,9 @@ static int arcmsr_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		arcmsr_free_io_queue(acb);
 	error = arcmsr_alloc_ccb_pool(acb);
 	if(error){
-		goto unmap_pci_region;
+		if (acb->adapter_type == ACB_ADAPTER_TYPE_F)
+			arcmsr_free_io_queue(acb);
+		goto free_ccb_pool;
 	}
 	error = scsi_add_host(host, &pdev->dev);
 	if(error){
-- 
2.55.0

Re: [PATCH] scsi: arcmsr: fix DMA memory and IO queue leak on arcmsr_alloc_ccb_pool() failure

[sashiko-bot]

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

New issues:
- [High] The patch introduces a kernel panic (NULL pointer dereference) in `arcmsr_free_ccb_pool()` when cleaning up after an XOR buffer allocation failure.

Pre-existing issues:
- [High] A race condition during device removal and probe error teardown leaves work queued on the system workqueue after the adapter struct has been freed, causing a use-after-free.
--

--- Patch [1]: [PATCH] scsi: arcmsr: fix DMA memory and IO queue leak on arcmsr_alloc_ccb_pool() failure ---
commit 54e963f4d0ddf2b670ba41920062308c1b96d75e
Author: Ivy Lopez <skunkolee@gmail.com>

scsi: arcmsr: fix DMA memory and IO queue leak on arcmsr_alloc_ccb_pool() failure

This commit fixes a DMA memory and IO queue leak that occurs when
arcmsr_alloc_ccb_pool() fails. It routes the error path through the
free_ccb_pool label and explicitly frees the IO queue for type F adapters.

> diff --git a/drivers/scsi/arcmsr/arcmsr_hba.c b/drivers/scsi/arcmsr/arcmsr_hba.c
> index 8aa948f06cacf..3ffd160a6a550 100644
> --- a/drivers/scsi/arcmsr/arcmsr_hba.c
> +++ b/drivers/scsi/arcmsr/arcmsr_hba.c
> @@ -1144,7 +1144,9 @@ static int arcmsr_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  		arcmsr_free_io_queue(acb);
>  	error = arcmsr_alloc_ccb_pool(acb);
>  	if(error){
> -		goto unmap_pci_region;
> +		if (acb->adapter_type == ACB_ADAPTER_TYPE_F)
> +			arcmsr_free_io_queue(acb);
> +		goto free_ccb_pool;

[Severity: High]
Does this routing of the error path to free_ccb_pool introduce a kernel
panic if arcmsr_alloc_ccb_pool() fails during XOR buffer allocation?

If arcmsr_alloc_xor_buffer() fails, acb->xor_mega is already set to a
non-zero value, but acb->xorVirt might be NULL.

When arcmsr_free_ccb_pool() is called from this new error path, it checks
acb->xor_mega and proceeds to calculate a pointer using the NULL
acb->xorVirt:

drivers/scsi/arcmsr/arcmsr_hba.c:arcmsr_free_ccb_pool() {
    ...
    if (acb->xor_mega) {
        struct Xor_sg *pXorPhys;
        ...
        pXorPhys = (struct Xor_sg *)(acb->xorVirt + sizeof(struct HostRamBuf));
        ...
        if (pXorPhys->xorPhys) {
    ...
}

Will this unconditionally dereference the invalid pXorPhys pointer
(address 16) and cause a panic?

[Severity: High]
This is a pre-existing issue, but does the error teardown sequence leave a
window for a use-after-free?

In the arcmsr_probe() error path, flush_work() is called before the hardware
interrupt is freed:

drivers/scsi/arcmsr/arcmsr_hba.c:arcmsr_probe() {
out_free_sysfs:
    ...
    flush_work(&acb->arcmsr_do_message_isr_bh);
    arcmsr_stop_adapter_bgrb(acb);
    arcmsr_flush_adapter_cache(acb);
    arcmsr_free_irq(pdev, acb);
    ...
}

Because the interrupt is still active, could it fire and re-queue
arcmsr_do_message_isr_bh after the flush? When scsi_host_put() subsequently
frees the acb structure, would the system workqueue then dereference the
freed memory?

>  	}
>  	error = scsi_add_host(host, &pdev->dev);
>  	if(error){

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260807185018.80661-1-skunkolee@gmail.com?part=1