[PATCH] scsi: arcmsr: fix NULL deref on dma_alloc_coherent() failure in arcmsr_alloc_xor_buffer()

[PATCH] scsi: arcmsr: fix NULL deref on dma_alloc_coherent() failure in arcmsr_alloc_xor_buffer()

[Ivy Lopez]

arcmsr_alloc_xor_buffer() does not check the return value of the
initial dma_alloc_coherent() call before using it. If the allocation
fails, the code performs pointer arithmetic on the NULL base
(computing pXorPhys and pXorVirt) and later unconditionally
dereferences it through pRamBuf to write hrbSignature and other
fields, causing a NULL pointer dereference.

Additionally, acb->xor_mega is set unconditionally before the
allocation attempt, based only on firmware status bits. If the
allocation fails, acb->xor_mega remains nonzero, so the later
cleanup path in arcmsr_free_ccb_pool() and the message-config code
in arcmsr_iop_confirm() will still enter their "if (acb->xor_mega)"
branches and dereference the never-set acb->xorVirt/acb->xorPhys,
a second NULL pointer dereference on the allocation failure path.

Fix this by checking the initial dma_alloc_coherent() result and, on
failure, resetting acb->xor_mega to 0 before returning -ENOMEM, so
that no code path treats the XOR buffer as present when it was never
allocated.

Found by static analysis; no hardware reproducer.

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

diff --git a/drivers/scsi/arcmsr/arcmsr_hba.c b/drivers/scsi/arcmsr/arcmsr_hba.c
index 8aa948f06cac..40407b281839 100644
--- a/drivers/scsi/arcmsr/arcmsr_hba.c
+++ b/drivers/scsi/arcmsr/arcmsr_hba.c
@@ -771,6 +771,12 @@ static int arcmsr_alloc_xor_buffer(struct AdapterControlBlock *acb)
 		(sizeof(struct XorHandle) * acb->xor_mega);
 	dma_coherent = dma_alloc_coherent(&pdev->dev, acb->init2cfg_size,
 		&dma_coherent_handle, GFP_KERNEL);
+	if (!dma_coherent) {
+		pr_info("arcmsr%d: alloc init2cfg buffer failed\n",
+			acb->host->host_no);
+		acb->xor_mega = 0;
+		return -ENOMEM;
+	}
 	acb->xorVirt = dma_coherent;
 	acb->xorPhys = dma_coherent_handle;
 	pXorPhys = (struct Xor_sg *)((unsigned long)dma_coherent +
-- 
2.55.0

Re: [PATCH] scsi: arcmsr: fix NULL deref on dma_alloc_coherent() failure in arcmsr_alloc_xor_buffer()

[sashiko-bot]

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

New issues:
- [Medium] The commit message contains a false claim about secondary NULL pointer dereferences, leading to dead code logic (`acb->xor_mega = 0;`).

Pre-existing issues:
- [High] Pre-existing massive memory leak of the main CCB pool and IO queues on `arcmsr_alloc_xor_buffer()` failure.
--

--- Patch [1]: [PATCH] scsi: arcmsr: fix NULL deref on dma_alloc_coherent() failure in arcmsr_alloc_xor_buffer() ---
commit afa04c2998e47d9ff0b55725d2cfb10245105508
Author: Ivy Lopez <skunkolee@gmail.com>

scsi: arcmsr: fix NULL deref on dma_alloc_coherent() failure in arcmsr_alloc_xor_buffer()

This patch checks the return value of dma_alloc_coherent() in
arcmsr_alloc_xor_buffer() to prevent a NULL pointer dereference. On failure,
it resets acb->xor_mega to 0 and returns -ENOMEM to fail gracefully.

> Additionally, acb->xor_mega is set unconditionally before the
> allocation attempt, based only on firmware status bits. If the
> allocation fails, acb->xor_mega remains nonzero, so the later
> cleanup path in arcmsr_free_ccb_pool() and the message-config code
> in arcmsr_iop_confirm() will still enter their "if (acb->xor_mega)"
> branches and dereference the never-set acb->xorVirt/acb->xorPhys,
> a second NULL pointer dereference on the allocation failure path.

[Severity: Medium]
Is this analysis of the failure path accurate?

If this allocation fails, arcmsr_alloc_ccb_pool() propagates the -ENOMEM
up to arcmsr_probe(). In arcmsr_probe(), an error from arcmsr_alloc_ccb_pool()
triggers a jump to the unmap_pci_region label, which completely bypasses the
free_ccb_pool label.

Because of this bypass, arcmsr_free_ccb_pool() and arcmsr_iop_confirm()
are never reached on this failure path, meaning those secondary NULL pointer
dereferences are structurally impossible.

> diff --git a/drivers/scsi/arcmsr/arcmsr_hba.c b/drivers/scsi/arcmsr/arcmsr_hba.c
> index 8aa948f06cacf..40407b2818396 100644
> --- a/drivers/scsi/arcmsr/arcmsr_hba.c
> +++ b/drivers/scsi/arcmsr/arcmsr_hba.c
> @@ -771,6 +771,12 @@ static int arcmsr_alloc_xor_buffer(struct AdapterControlBlock *acb)
>  		(sizeof(struct XorHandle) * acb->xor_mega);
>  	dma_coherent = dma_alloc_coherent(&pdev->dev, acb->init2cfg_size,
>  		&dma_coherent_handle, GFP_KERNEL);
> +	if (!dma_coherent) {
> +		pr_info("arcmsr%d: alloc init2cfg buffer failed\n",
> +			acb->host->host_no);
> +		acb->xor_mega = 0;

[Severity: Medium]
Since arcmsr_probe() jumps to unmap_pci_region and skips arcmsr_free_ccb_pool()
on failure, is this assignment dead logic? The cleanup path where this flag
would matter cannot be reached on this error path.

> +		return -ENOMEM;

[Severity: High]
This is a pre-existing issue, but does returning here cause a massive DMA
memory leak?

When this returns -ENOMEM, the caller arcmsr_alloc_ccb_pool() propagates
the error to arcmsr_probe() without cleaning up the earlier massive DMA
allocation for acb->dma_coherent (the CCB pool).

arcmsr_probe() then jumps directly to unmap_pci_region:

arcmsr_probe() {
    ...
    error = arcmsr_alloc_ccb_pool(acb);
    if(error){
        goto unmap_pci_region;
    }
    ...
}

This bypasses the free_ccb_pool label entirely, so arcmsr_free_ccb_pool()
is never called, leaking up to 12MB of DMA coherent memory.

> +	}
>  	acb->xorVirt = dma_coherent;
>  	acb->xorPhys = dma_coherent_handle;
>  	pXorPhys = (struct Xor_sg *)((unsigned long)dma_coherent +

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

Re: [PATCH] scsi: arcmsr: fix NULL deref on dma_alloc_coherent() failure in arcmsr_alloc_xor_buffer()

[Ivy Lopez]

Thanks for the review.

Now that I look at it, the commit message's claim about a reachable
secondary NULL deref via arcmsr_free_ccb_pool()/arcmsr_iop_confirm()
doesn't hold up: arcmsr_alloc_ccb_pool() failing sends arcmsr_probe()
straight to unmap_pci_region, bypassing the free_ccb_pool label
entirely, so that path isn't reachable here. The acb->xor_mega = 0
line is still harmless defensively, but my stated justification for
it was wrong. My apologies for the inaccuracy!

Separately, thanks for flagging the pre-existing DMA leak of
acb->dma_coherent (and the IO queue) when arcmsr_alloc_xor_buffer()
fails inside arcmsr_alloc_ccb_pool()! I'll follow up with a separate
patch addressing that.

ivy