Re: [PATCH] scsi: mptfusion: Avoid NULL alt_ioc reload in reset work

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] The patch resolves a TOCTOU NULL pointer dereference but leaves a severe Use-After-Free (UAF) vulnerability. If the alternate controller is detached and freed concurrently, the worker thread will dereference a freed pointer when acquiring the task management lock.
- [High] Detaching an alternate IOC permanently stops fault polling for the remaining primary IOC, silently leaving it unmonitored.
--

--- Patch [1]: [PATCH] scsi: mptfusion: Avoid NULL alt_ioc reload in reset work ---
commit 62bf3402df60ac4d2e56595bb2e871b578a2c9e5
Author: Ruoyu Wang <ruoyuw560@gmail.com>

scsi: mptfusion: Avoid NULL alt_ioc reload in reset work

mpt_fault_reset_work() alternates delayed fault polling between bound IOC
ports by switching to ioc->alt_ioc before rearming the timer.

> diff --git a/drivers/message/fusion/mptbase.c b/drivers/message/fusion/mptbase.c
> index 3a431ffd3e2eb..29bcf44356d23 100644
> --- a/drivers/message/fusion/mptbase.c
> +++ b/drivers/message/fusion/mptbase.c
> @@ -424,8 +424,7 @@ mpt_fault_reset_work(struct work_struct *work)
>  	/*
>  	 * Take turns polling alternate controller
>  	 */
> -	if (ioc->alt_ioc)
> -		ioc = ioc->alt_ioc;
> +	ioc = READ_ONCE(ioc->alt_ioc) ?: ioc;

[Severity: Critical]
This is a pre-existing issue, but can this code cause a use-after-free
when the alternate controller is detached concurrently?

If the primary adapter (A) runs mpt_fault_reset_work() and reads A->alt_ioc
(pointing to B), and concurrently mpt_detach() runs for B,
mpt_adapter_dispose() clears A->alt_ioc and frees B:

drivers/message/fusion/mptbase.c:mpt_adapter_dispose() {
	if (ioc->alt_ioc)
		ioc->alt_ioc->alt_ioc = NULL;

	kfree(ioc);
}

If A's work reads A->alt_ioc just before it is cleared, will A attempt to
acquire the freed B->taskmgmt_lock below?

>  
>  	/* rearm the timer */
>  	spin_lock_irqsave(&ioc->taskmgmt_lock, flags);
>  	if (ioc->reset_work_q)
>  		queue_delayed_work(ioc->reset_work_q, &ioc->fault_reset_work,
>  			msecs_to_jiffies(MPT_POLLING_INTERVAL));

[Severity: High]
This is also a pre-existing issue, but does detaching an alternate IOC
break fault polling for the remaining primary IOC?

If the primary adapter (A) and alternate adapter (B) alternate queueing each
other's reset work, what happens when B is detached?

drivers/message/fusion/mptbase.c:mpt_detach() {
	wq = ioc->reset_work_q;
	ioc->reset_work_q = NULL;
	spin_unlock_irqrestore(&ioc->taskmgmt_lock, flags);
	cancel_delayed_work(&ioc->fault_reset_work);
}

If B's work was the currently scheduled one, it is cancelled, and A's work is
never queued again. Alternatively, if A's work evaluates the now-NULL
B->reset_work_q here, will it skip rearming the timer and leave A
without polling?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707151344.2335768-1-ruoyuw560@gmail.com?part=1