Re: [PATCH] scsi: megaraid_sas: fix hb_host_mem leak on init failure

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Moving hb_host_mem deallocation to megasas_free_ctrl_dma_buffers() introduces a Double Free vulnerability.
- [Critical] Moving hb_host_mem deallocation to megasas_free_ctrl_dma_buffers() introduces a critical DMA Use-After-Free (memory corruption) in the megasas_resume() error path.

Pre-existing issues:
- [High] Premature scsi_host_put() in the megasas_resume() error path causes an unbalanced refcount and Use-After-Free.
- [High] The background workqueue work_init is never explicitly canceled during driver teardown, leading to a Use-After-Free.
--

commit f28052d4c7a011d567f9039c9799bb5ecb631dd8
Author: Li Youhong <liyouhong@kylinos.cn>

scsi: megaraid_sas: fix hb_host_mem leak on init failure

hb_host_mem is allocated in megasas_sriov_start_heartbeat() but only
freed in megasas_detach_one(). megasas_init_fw() and megasas_resume()
failure paths clean up through megasas_free_ctrl_dma_buffers(), which
did not free this buffer.

Move the free from megasas_detach_one() into
megasas_free_ctrl_dma_buffers().

> diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c b/drivers/scsi/megaraid/megaraid_sas_base.c
> index 46aebe52c3da9..5ca2908ebfb10 100644
> --- a/drivers/scsi/megaraid/megaraid_sas_base.c
> +++ b/drivers/scsi/megaraid/megaraid_sas_base.c
> @@ -7415,6 +7415,12 @@ void megasas_free_ctrl_dma_buffers(struct megasas_instance *instance)
>  				  instance->host_device_list_buf,
>  				  instance->host_device_list_buf_h);
>  
> +	if (instance->hb_host_mem)
> +		dma_free_coherent(&pdev->dev,
> +				  sizeof(struct MR_CTRL_HB_HOST_MEM),
> +				  instance->hb_host_mem,
> +				  instance->hb_host_mem_h);
> +

[Severity: Critical]
Does moving this deallocation here introduce a double free of hb_host_mem?

If megasas_resume() fails, it cleans up by calling
megasas_free_ctrl_dma_buffers(), which frees instance->hb_host_mem but
leaves the pointer dangling because it is not set to NULL.

If the block layer holds a reference preventing immediate freeing of the
instance, subsequent unbinding of the driver will invoke
megasas_detach_one(), which unconditionally calls
megasas_free_ctrl_dma_buffers() again, triggering a double free.

[Severity: Critical]
Could freeing this memory here lead to DMA memory corruption when called
from the megasas_resume() error path?

In megasas_resume(), megasas_sriov_start_heartbeat() tells the active
firmware to continuously DMA to the physical address of hb_host_mem.

If a subsequent initialization step fails, the fail_init_mfi error path
calls megasas_free_ctrl_dma_buffers(). 

Because the error path does not send a shutdown command to the firmware
or clear the PCI Bus Master bit, the active firmware may silently
continue to perform periodic DMA writes to the freed memory, corrupting
whichever subsystem later allocates that physical page.

>  }
>  
>  /*
[ ... ]
> @@ -8092,11 +8098,6 @@ static void megasas_detach_one(struct pci_dev *pdev)
>  				    instance->vf_affiliation_111,
>  				    instance->vf_affiliation_111_h);
>  
> -	if (instance->hb_host_mem)
> -		dma_free_coherent(&pdev->dev, sizeof(struct MR_CTRL_HB_HOST_MEM),
> -				    instance->hb_host_mem,
> -				    instance->hb_host_mem_h);
> -
>  	megasas_free_ctrl_dma_buffers(instance);
>  
>  	megasas_free_ctrl_mem(instance);

[Severity: High]
This is a pre-existing issue, but does megasas_detach_one() leave
instance->work_init pending during driver teardown?

During driver teardown, megasas_detach_one() deletes the
sriov_heartbeat_timer via timer_delete_sync(), but it entirely fails to
call cancel_work_sync(&instance->work_init).

If the work is scheduled just before the timer is deleted, it will remain
pending and can execute asynchronously after scsi_host_put() frees the
instance structure, leading to a Use-After-Free.

[Severity: High]
This isn't a bug introduced by this patch, but does the megasas_resume()
error path drop a Scsi_Host reference that was never acquired?

If megasas_resume() encounters an initialization failure, the
fail_init_mfi error path executes scsi_host_put(host):

drivers/scsi/megaraid/megaraid_sas_base.c:megasas_resume() {
    ...
fail_init_mfi:
	megasas_free_ctrl_dma_buffers(instance);
	megasas_free_ctrl_mem(instance);
	scsi_host_put(host);
    ...
}

Because this reference was never taken during resume, dropping it causes
a refcount underflow, leading to the premature freeing of the host and
the embedded megasas_instance structure.

When the device is later unbound, megasas_detach_one() will retrieve the
already-freed instance pointer via pci_get_drvdata() and dereference it,
leading to a system crash.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904092740.2612033-1-dayou5941@163.com?part=1