Re: [PATCH] scsi: megaraid_sas: kill tasklet after free_irq in teardown paths

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Unprotected concurrent access to the global `megasas_mgmt_info` array during device probe and detach causes out-of-bounds writes.
- [High] The error paths in `megasas_resume` fail to disable/destroy IRQs, kill the tasklet, and free PCI IRQ vectors, causing a use-after-free.
- [High] Legacy INTx IRQ vectors are systematically leaked in teardown and error paths.
- [High] Incomplete tasklet cleanup in initialization error paths allows a pending tasklet to execute after the `instance` is freed.
- [High] The asynchronous work item `instance->work_init` is never cancelled during driver teardown, suspend, or probe error paths.
--

--- Patch [1]: [PATCH] scsi: megaraid_sas: kill tasklet after free_irq in teardown paths ---
commit 8d9eb16b77703617161e881af92f251c7fb898af
Author: Fan Wu <fanwu01@zju.edu.cn>

scsi: megaraid_sas: kill tasklet after free_irq in teardown paths

On MFI-series adapters the interrupt handler arms instance->isr_tasklet
for as long as the interrupt is registered, so the tasklet can be armed
again after tasklet_kill() has returned.  This patch moves tasklet_kill()
after megasas_destroy_irqs() to ensure tasklets are stopped after
the handler is drained.

Cc: stable@vger.kernel.org

> diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c b/drivers/scsi/megaraid/megaraid_sas_base.c
> index f0152b043e18c..e72009a587567 100644
> --- a/drivers/scsi/megaraid/megaraid_sas_base.c
> +++ b/drivers/scsi/megaraid/megaraid_sas_base.c
> @@ -6536,6 +6536,7 @@ static int megasas_init_fw(struct megasas_instance *instance)
>  fail_get_ld_pd_list:
>  	instance->instancet->disable_intr(instance);
>  	megasas_destroy_irqs(instance);
> +	tasklet_kill(&instance->isr_tasklet);
>  fail_init_adapter:

[Severity: High]
This isn't a bug introduced by this patch, but does the jump to
fail_init_adapter leave the tasklet pending if IRQ setup fails?

If megasas_setup_irqs_msix() partially succeeds but fails on a
subsequent vector, it returns an error and the code jumps to
fail_init_adapter, bypassing the tasklet_kill() added here.

Since the rollback loop in megasas_setup_irqs_msix() calls free_irq()
which doesn't kill scheduled tasklets, can the pending tasklet
execute and dereference the freed instance memory?

>  	if (instance->msix_vectors)
>  		pci_free_irq_vectors(instance->pdev);

[Severity: High]
This is a pre-existing issue, but does this conditional check leak
legacy INTx IRQ vectors?

When MSI-X is unavailable, the driver falls back to allocating a
legacy INTx interrupt, but instance->msix_vectors remains 0.
By wrapping pci_free_irq_vectors() in this condition across the
teardown and error paths, it appears the cleanup is skipped for
INTx vectors.

Does this permanently leak IRQ resources in the error paths?

[ ... ]

> @@ -7618,6 +7619,7 @@ static int megasas_probe_one(struct pci_dev *pdev,
>  
>  	instance->instancet->disable_intr(instance);
>  	megasas_destroy_irqs(instance);
> +	tasklet_kill(&instance->isr_tasklet);

[Severity: High]
This is a pre-existing issue, but do the error paths in megasas_probe_one()
properly clean up interrupts and tasklets before freeing the instance?

If a late step in the probe fails, such as megasas_fusion_start_watchdog(),
the code jumps to fail_init_mfi.  At this point, IRQs have already been
requested and the tasklet initialized.

However, the fail_init_mfi path skips disable_intr, megasas_destroy_irqs,
and tasklet_kill, but proceeds to free the coherent DMA buffers and the
host structure.

Can an active ISR or scheduled tasklet execute after these are freed,
resulting in a use-after-free?

[ ... ]

> @@ -8010,8 +8011,6 @@ static void megasas_detach_one(struct pci_dev *pdev)
>  	/* cancel all wait events */
>  	wake_up_all(&instance->int_cmd_wait_q);
>  

[Severity: High]
This is a pre-existing issue, but is the asynchronous work item
instance->work_init properly cancelled during detach?

The driver schedules instance->work_init on the global system
workqueue to handle firmware state changes and online controller
resets.  Neither the detach path nor the suspend and probe error
handlers appear to call cancel_work_sync(&instance->work_init).

If a controller fault triggers the work right before teardown,
will the asynchronous work dereference the freed instance memory?

> -	tasklet_kill(&instance->isr_tasklet);
> -
>  	/*
>  	 * Take the instance off the instance array. Note that we will not
>  	 * decrement the max_index. We let this array be sparse array

[Severity: Critical]
This is a pre-existing issue, but does this sparse array management
allow unbounded growth and out-of-bounds writes?

Because megasas_detach_one() intentionally does not decrement
max_index, repeated unbind and bind cycles via sysfs or hotplugs
will cause max_index to grow continuously.

Since there is no bounds checking in megasas_probe_one() when
inserting new instances:

megasas_probe_one() {
        ...
        megasas_mgmt_info.count++;
        megasas_mgmt_info.instance[megasas_mgmt_info.max_index] = instance;
        megasas_mgmt_info.max_index++;
        ...
}

Will max_index eventually exceed MAX_MGMT_ADAPTERS and overwrite
adjacent kernel memory?

> @@ -8028,6 +8027,7 @@ static void megasas_detach_one(struct pci_dev *pdev)
>  	instance->instancet->disable_intr(instance);
>  
>  	megasas_destroy_irqs(instance);
> +	tasklet_kill(&instance->isr_tasklet);
>  
>  	if (instance->msix_vectors)
>  		pci_free_irq_vectors(instance->pdev);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260819005043.439436-1-fanwu01@zju.edu.cn?part=1