Re: [PATCH v2 16/17] mpi3mr: Fix use-after-free of the firmware event workqueue

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

Pre-existing issues:
- [High] Use-after-free in `mpi3mr_cancel_work` when canceling concurrently executing firmware event work.
- [High] Data race leading to Use-After-Free of `mrioc->current_event` during firmware event cleanup.
--

commit c5c6dc4f18099cddf01a3785308a8b8fac21872d
Author: Chandrakanth Patil <[email protected]>

mpi3mr: Fix use-after-free of the firmware event workqueue
    
The enqueue path reads the workqueue pointer before taking the event
lock, while the remove path clears that pointer and destroys the
workqueue while holding it. The enqueue can therefore reach
queue_work() after the workqueue is gone.
    
Move the check inside the lock.

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
> index 543e9b8e3fc37..e97e343b14fa5 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_os.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
> @@ -233,10 +233,11 @@ static void mpi3mr_fwevt_add_to_list(struct mpi3mr_ioc *mrioc,
>  {
>  	unsigned long flags;
>  
> -	if (!mrioc->fwevt_worker_thread)
> -		return;
> -
>  	spin_lock_irqsave(&mrioc->fwevt_lock, flags);
> +	if (!mrioc->fwevt_worker_thread) {
> +		spin_unlock_irqrestore(&mrioc->fwevt_lock, flags);
> +		return;
> +	}
>  	/* get fwevt reference count while adding it to fwevt_list */
>  	mpi3mr_fwevt_get(fwevt);
>  	INIT_LIST_HEAD(&fwevt->list);

[Severity: High]
This isn't a bug introduced by this patch, but looking at how firmware events
are managed and cleaned up, is there a use-after-free risk during cancellation?

When mpi3mr_cleanup_fwevt_list() flushes pending events, it removes the event
from the list and immediately drops its list reference:

drivers/scsi/mpi3mr/mpi3mr_os.c:mpi3mr_dequeue_fwevt() {
    ...
    list_del_init(&fwevt->list);
    /*
     * Put fwevt reference count after
     * removing it from fwevt_list
     */
    mpi3mr_fwevt_put(fwevt);
    ...
}

It then calls mpi3mr_cancel_work(fwevt), which invokes
cancel_work_sync(&fwevt->work). If the work is already executing but hasn't yet
removed itself from the list in mpi3mr_fwevt_bh(), cancel_work_sync() will
block waiting for the worker thread.

When the worker thread completes, it drops all remaining references and frees
the event memory:

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

Does this leave cancel_work_sync() operating on a freed work_struct?


[Severity: High]
This is a pre-existing issue, but there appears to be a data race when accessing
mrioc->current_event in the cleanup path.

In mpi3mr_cleanup_fwevt_list(), after draining the list, the current_event
pointer is accessed without holding a lock or taking a reference:

drivers/scsi/mpi3mr/mpi3mr_os.c:mpi3mr_cleanup_fwevt_list() {
    ...
    if (mrioc->current_event) {
        fwevt = mrioc->current_event;
        ...
        if (current_work() == &fwevt->work || fwevt->pending_at_sml) {
    ...
}

Concurrently, if the worker thread is finishing execution in mpi3mr_fwevt_bh(),
it drops the reference and sets the pointer to NULL:

    mpi3mr_fwevt_put(fwevt);
    mrioc->current_event = NULL;

If the cleanup thread reads mrioc->current_event before it is set to NULL, and
the worker thread completes and drops its final reference (freeing the event),
could the cleanup thread dereference freed memory when evaluating
fwevt->pending_at_sml or calling mpi3mr_cancel_work(fwevt)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=16