[PATCH v2] scsi: megaraid_sas: Protect megasas_get_ctrl_info() in megasas_resume()

[PATCH v2] scsi: megaraid_sas: Protect megasas_get_ctrl_info() in megasas_resume()

[Bart Van Assche]

Protect the megasas_get_ctrl_info() call in megasas_resume() with
instance->reset_mutex using scoped_guard().

megasas_get_ctrl_info() may release and reacquire instance->reset_mutex.
Hence, calling this function without holding instance->reset_mutex is not
safe.

Fixes: c3b10a55abc9 ("scsi: megaraid_sas: Update controller info during resume")
Cc: Kashyap Desai <kashyap.desai@broadcom.com>
Cc: Sumit Saxena <sumit.saxena@broadcom.com>
Cc: Shivasharan S <shivasharan.srikanteshwara@broadcom.com>
Cc: Chandrakanth patil <chandrakanth.patil@broadcom.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---

Changes compared to v1: moved goto outside scoped_guard() { }

 drivers/scsi/megaraid/megaraid_sas_base.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c b/drivers/scsi/megaraid/megaraid_sas_base.c
index f0152b043e18..d4cbc0643eaf 100644
--- a/drivers/scsi/megaraid/megaraid_sas_base.c
+++ b/drivers/scsi/megaraid/megaraid_sas_base.c
@@ -7875,7 +7875,9 @@ megasas_resume(struct device *dev)
 			goto fail_init_mfi;
 	}
 
-	if (megasas_get_ctrl_info(instance) != DCMD_SUCCESS)
+	scoped_guard(mutex, &instance->reset_mutex)
+		rval = megasas_get_ctrl_info(instance);
+	if (rval != DCMD_SUCCESS)
 		goto fail_init_mfi;
 
 	tasklet_init(&instance->isr_tasklet, instance->instancet->tasklet,

Re: [PATCH v2] scsi: megaraid_sas: Protect megasas_get_ctrl_info() in megasas_resume()

[sashiko-bot]

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

New issues:
- [Medium] Mixing goto-based error handling with scoped_guard() in megasas_resume() violates subsystem cleanup guidelines.

Pre-existing issues:
- [High] Missing IRQ teardown in megasas_resume error path leads to ISR Use-After-Free and memory leak.
--

commit aca55e5d3d8f6827c1bf9b8345c147c29fda17ef
Author: Bart Van Assche <bvanassche@acm.org>
scsi: megaraid_sas: Protect megasas_get_ctrl_info() in megasas_resume()

This commit protects the megasas_get_ctrl_info() call within
megasas_resume() by wrapping it with scoped_guard() using
instance->reset_mutex. This ensures the function is called with the
mutex held, preventing unsafe releases and reacquisitions.

> diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c b/drivers/scsi/megaraid/megaraid_sas_base.c
> index f0152b043e18c..d4cbc0643eaf1 100644
> --- a/drivers/scsi/megaraid/megaraid_sas_base.c
> +++ b/drivers/scsi/megaraid/megaraid_sas_base.c
> @@ -7875,7 +7875,9 @@ megasas_resume(struct device *dev)
>  			goto fail_init_mfi;
>  	}
>  
> -	if (megasas_get_ctrl_info(instance) != DCMD_SUCCESS)
> +	scoped_guard(mutex, &instance->reset_mutex)
> +		rval = megasas_get_ctrl_info(instance);

[Severity: Medium]
Does mixing scoped_guard() with goto-based error handling in megasas_resume()
violate subsystem cleanup guidelines?

The subsystem documentation explicitly forbids mixing scope-based cleanup
helpers with goto-based error handling in the same function, requiring an
all-or-nothing conversion to avoid confusing ownership semantics.

Since megasas_resume() heavily relies on the goto fail_init_mfi style,
should this function be fully converted, or should a traditional
mutex_lock() and mutex_unlock() be used here instead?

> +	if (rval != DCMD_SUCCESS)
>  		goto fail_init_mfi;

[Severity: High]
This is a pre-existing issue, but does the fail_init_mfi error path lack
proper IRQ teardown, creating a potential Use-After-Free?

If a failure occurs later in megasas_resume() after interrupts are enabled
(for example, if megasas_fusion_start_watchdog() fails), the code jumps to
fail_init_mfi and calls megasas_free_ctrl_mem(instance). This frees
instance->reply_map and the fusion context, but it does not call
megasas_destroy_irqs(instance) or disable the interrupts.

Since instance->mask_interrupts would be 0 at that point, any subsequent
shared or spurious interrupt could invoke megasas_isr_fusion(), accessing
the freed reply_map and fusion context.

Does this error path need to unregister interrupts before freeing the memory?

>  
>  	tasklet_init(&instance->isr_tasklet, instance->instancet->tasklet,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/f06b5ee432b21cf293f0663e15b64f75a84b9fd5.1788204406.git.bvanassche@acm.org?part=1