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

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Mixing goto-based error handling and scoped_guard in the same function violates the kernel cleanup guidelines.

Pre-existing issues:
- [High] The error path in `megasas_resume` fails to deregister IRQs while freeing critical control structures, leading to a Use-After-Free.
--

commit 29ed10a7540b21ede13952240b8d781f1bdf89ab
Author: Bart Van Assche <bvanassche@acm.org>

scsi: megaraid_sas: Protect megasas_get_ctrl_info() in megasas_resume()

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

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

[Severity: Medium]
Does this mix goto-based error handling and scoped_guard() in the same
function?

The cleanup guidelines explicitly state that goto and cleanup helpers should
never be mixed in the same function to avoid confusing ownership semantics.
Should the other resources in megasas_resume() using goto cleanup be
converted to scope-based cleanup, or should this scoped_guard() be changed
to standard mutex_lock() and mutex_unlock()?

> +			goto fail_init_mfi;

[Severity: High]
This isn't a bug introduced by this patch, but could this error path lead
to a use-after-free?

If megasas_resume() fails here, it jumps to the fail_init_mfi label:

fail_init_mfi:
	megasas_free_ctrl_dma_buffers(instance);
	megasas_free_ctrl_mem(instance);
	scsi_host_put(host);

Since IRQs are requested earlier in the function but megasas_destroy_irqs()
is not called in this error path, could an interrupt fire and execute the
handler while structures like instance->reply_queue (freed in
megasas_free_ctrl_mem()) are accessed?

>  
>  	tasklet_init(&instance->isr_tasklet, instance->instancet->tasklet,
>  		     (unsigned long)instance);

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