Re: [PATCH v1 07/10] mpi3mr: Fix firmware event reference leak during cleanup

[sashiko-bot@kernel.org]

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

New issues:
- [High] The patch introduces a Use-After-Free and Double-Free vulnerability by prematurely decrementing the reference count of the currently executing firmware event in the early-exit path of `mpi3mr_cleanup_fwevt_list`.

Pre-existing issues:
- [High] Lockless read of `mrioc->current_event` in `mpi3mr_cleanup_fwevt_list` leads to a Time-of-Check to Time-of-Use (TOCTOU) use-after-free when racing with the firmware event worker thread.
--

commit 50f8b457978a02d12ee5baafb77e83f6f06d294f
Author: Ranjan Kumar <ranjan.kumar@broadcom.com>
Subject: mpi3mr: Fix firmware event reference leak during cleanup

This commit attempts to fix a firmware event reference leak during cleanup.
It adds a call to release the firmware event reference before returning from
the early-exit path when an event is currently executing or pending at the
SCSI mid-layer.

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
> index e361fbb8f7235..b60afaeef68d0 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_os.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
> @@ -399,6 +399,7 @@ void mpi3mr_cleanup_fwevt_list(struct mpi3mr_ioc *mrioc)
>  		 */

[Severity: High]
This is a pre-existing issue, but does the lockless read of current_event
earlier in this function create a race condition with the firmware event
worker thread?

If a soft reset thread executes mpi3mr_cleanup_fwevt_list() and reads the
event into the fwevt pointer, and the worker thread finishes and frees the
event before this thread reaches the pending_at_sml check below, could
dereferencing fwevt result in a use-after-free?

>  		if (current_work() == &fwevt->work || fwevt->pending_at_sml) {
>  			fwevt->discard = 1;
> +			mpi3mr_fwevt_put(fwevt);

[Severity: High]
Does this additional mpi3mr_fwevt_put() cause a double-free or use-after-free
when the active worker thread finishes its execution?

Looking at mpi3mr_fwevt_worker(), the worker thread retains ownership of the
event and appears to drop the reference itself after returning from the bottom
half handler:

mpi3mr_fwevt_worker() {
   ...
   mpi3mr_fwevt_bh(fwevt->mrioc, fwevt);
   /*
    * Put fwevt reference count after
    * dequeuing it from worker queue
    */
   mpi3mr_fwevt_put(fwevt);
   ...
}

Since the worker thread relies on its own reference, will stealing the
reference here cause the count to reach zero prematurely while the worker is
still using the object?

>  			return;
>  		}
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260626114109.43685-1-ranjan.kumar@broadcom.com?part=7