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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The lock-based TOCTOU fix is incomplete and fails to prevent a Use-After-Free because the worker thread modifies `current_event` locklessly.
- [High] A Use-After-Free write exists during event cleanup because `mpi3mr_dequeue_fwevt()` drops the event's reference before it is passed to `mpi3mr_cancel_work()`.
- [High] A race window during event dispatch allows `mpi3mr_cleanup_fwevt_list()` to return early and bypass cancelling an active firmware event.
--

--- Patch [7]: [PATCH v2 07/10] mpi3mr: Fix firmware event reference leak during cleanup ---
commit 594f8db6a0f63b3b0acbdd43108522c6a9ed9f0b
Author: Ranjan Kumar <ranjan.kumar@broadcom.com>

mpi3mr: Fix firmware event reference leak during cleanup

This commit addresses a firmware event reference leak in the cleanup routine
by properly releasing the reference before returning from the early exit and
normal cancel paths. It also adds a lock around the read of the current
event to prevent a race condition with the firmware event worker thread.

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

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
> index df7365d19b44a..273512ab25ba0 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_os.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
> @@ -379,6 +379,7 @@ static void mpi3mr_cancel_work(struct mpi3mr_fwevt *fwevt)
>  void mpi3mr_cleanup_fwevt_list(struct mpi3mr_ioc *mrioc)
>  {
>  	struct mpi3mr_fwevt *fwevt = NULL;
> +	unsigned long flags;
>  
>  	if ((list_empty(&mrioc->fwevt_list) && !mrioc->current_event) ||
>  	    !mrioc->fwevt_worker_thread)
>  		return;

[Severity: High]
This is a pre-existing issue, but does this early exit condition allow a race
where we bypass cancelling an active firmware event?

During event dispatch, the event is removed from the list and assigned to
current_event sequentially without atomicity:

drivers/scsi/mpi3mr/mpi3mr_os.c:mpi3mr_fwevt_bh() {
    ...
    mpi3mr_fwevt_del_from_list(mrioc, fwevt);
    mrioc->current_event = fwevt;
    ...
}

This creates a race window where the list is empty, but current_event is
still NULL. If mpi3mr_cleanup_fwevt_list() executes during this window,
could its lockless check evaluate to true and return early without waiting
for the work to complete?

>  
>  	while ((fwevt = mpi3mr_dequeue_fwevt(mrioc)))
>  		mpi3mr_cancel_work(fwevt);

[Severity: High]
This is a pre-existing issue, but does passing this pointer to
mpi3mr_cancel_work() result in a use-after-free?

The mpi3mr_dequeue_fwevt() function drops the list reference before returning
the pointer:

drivers/scsi/mpi3mr/mpi3mr_os.c:mpi3mr_dequeue_fwevt() {
    ...
    list_del_init(&fwevt->list);
    mpi3mr_fwevt_put(fwevt);
    ...
}

When we pass this unreferenced pointer to mpi3mr_cancel_work(), it calls
cancel_work_sync(&fwevt->work). If the worker thread finishes executing
concurrently and drops its remaining references, the structure could be freed.
Would cancel_work_sync() then write to freed memory when it calls
clear_work_data() on the embedded work structure?

>  
> -	if (mrioc->current_event) {
> -		fwevt = mrioc->current_event;
> +	/*
> +	 * Safely read current_event under lock to prevent TOCTOU race
> +	 * with the firmware event worker thread.
> +	 */
> +	spin_lock_irqsave(&mrioc->fwevt_lock, flags);
> +	fwevt = mrioc->current_event;
> +	if (fwevt)
> +		mpi3mr_fwevt_get(fwevt);
> +	spin_unlock_irqrestore(&mrioc->fwevt_lock, flags);

[Severity: High]
This is a pre-existing issue, but does the lock-based fix still leave
a use-after-free window because the worker thread modifies current_event
locklessly?

The read-side acquires the lock here, but the worker thread locklessly clears
current_event and drops the final reference:

drivers/scsi/mpi3mr/mpi3mr_os.c:mpi3mr_fwevt_bh() {
    ...
    mpi3mr_fwevt_put(fwevt);
    mrioc->current_event = NULL;
    ...
}

If mpi3mr_cleanup_fwevt_list() reads a non-NULL current_event, but gets
preempted before calling mpi3mr_fwevt_get(), could the worker thread
concurrently clear the pointer and free the memory? When the cleanup routine
resumes, it would call mpi3mr_fwevt_get() on a freed structure.

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