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

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 <[email protected]>

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/[email protected]?part=1