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

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

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