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

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

Pre-existing issues:
- [High] Use-After-Free of firmware event (`fwevt`) in `mpi3mr_cleanup_fwevt_list` during driver teardown.
--

--- Patch [16]: [PATCH 16/17] mpi3mr: Fix use-after-free of the firmware event workqueue ---
commit d1068d82a1a0b06d1a9647481bac5d43ff5f1ebc
Author: Chandrakanth Patil <[email protected]>

mpi3mr: Fix use-after-free of the firmware event workqueue

This commit fixes a use-after-free issue regarding the firmware event
workqueue in the mpi3mr driver. It ensures the worker thread pointer is
checked while holding the event lock to prevent a race condition with the
workqueue being destroyed.

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
> index 510c990d42d1b..4a72b3d89c294 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 is a pre-existing issue, but while reviewing the firmware event
lifecycle, I noticed another potential use-after-free during driver teardown.

In mpi3mr_dequeue_fwevt(), the list reference is dropped before returning
the pointer:

drivers/scsi/mpi3mr/mpi3mr_os.c:mpi3mr_dequeue_fwevt() {
    ...
		fwevt = list_first_entry(&mrioc->fwevt_list,
		    struct mpi3mr_fwevt, list);
		list_del_init(&fwevt->list);
		/*
		 * Put fwevt reference count after
		 * removing it from fwevt_list
		 */
		mpi3mr_fwevt_put(fwevt);
	}
	spin_unlock_irqrestore(&mrioc->fwevt_lock, flags);

	return fwevt;
}

Then in mpi3mr_cleanup_fwevt_list(), we pass this unreferenced pointer to
cancel_work_sync():

drivers/scsi/mpi3mr/mpi3mr_os.c:mpi3mr_cleanup_fwevt_list() {
    ...
	while ((fwevt = mpi3mr_dequeue_fwevt(mrioc)))
		mpi3mr_cancel_work(fwevt);
    ...
}

Could the background worker thread (mpi3mr_fwevt_worker) concurrently finish
executing and free the fwevt structure, causing cancel_work_sync() inside
mpi3mr_cancel_work() to access freed memory?

Additionally, lockless access to mrioc->current_event in
mpi3mr_cleanup_fwevt_list() could similarly read a concurrently freed event if
the worker thread finishes in the middle of cleanup:

drivers/scsi/mpi3mr/mpi3mr_os.c:mpi3mr_cleanup_fwevt_list() {
    ...
	if (mrioc->current_event) {
		fwevt = mrioc->current_event;
        ...
		mpi3mr_cancel_work(fwevt);
	}
}

Does this code need to hold a reference to fwevt while calling
mpi3mr_cancel_work() to prevent the worker thread from freeing it?

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