Re: [PATCH v2 17/17] mpi3mr: Fix NULL pointer dereference on PCI error recovery

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

Pre-existing issues:
- [Critical] Concurrent execution between `mpi3mr_cleanup_fwevt_list()` and the firmware event worker thread results in a Use-After-Free of the `mrioc->current_event` pointer.
- [High] The patch fails to resolve the 'already in flight' race condition described in its commit message, leaving the driver vulnerable to NULL pointer dereferences via a Time-Of-Check to Time-Of-Use (TOCTOU) race.
--

commit 67ebeaa54b154ad043a90c5a2e98345f3782e9c3
Author: Chandrakanth Patil <[email protected]>

mpi3mr: Fix NULL pointer dereference on PCI error recovery

This patch attempts to fix a race condition on a frozen PCI channel by
preventing in-flight firmware event handlers from accessing unmapped
register windows. It adds a check for pci_err_recovery early in the
bottom-half handler to skip event processing during recovery.

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
> index e97e343b14fa5..a389f7f2c741e 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_os.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
> @@ -2130,9 +2130,11 @@ static void mpi3mr_fwevt_bh(struct mpi3mr_ioc *mrioc,
>  	mpi3mr_fwevt_del_from_list(mrioc, fwevt);
>  	mrioc->current_event = fwevt;

[Severity: Critical]
This is a pre-existing issue, but does assigning mrioc->current_event here
lead to a use-after-free during driver teardown or reset?

If mpi3mr_cleanup_fwevt_list() is executing concurrently, it performs a
lockless read of mrioc->current_event:

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

At the end of mpi3mr_fwevt_bh(), the worker thread drops the reference and
NULLs the pointer:

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

If mpi3mr_cleanup_fwevt_list() captures the pointer before the worker thread
NULLs it, but accesses it after mpi3mr_fwevt_put() frees the memory, could
this corrupt memory when interacting with the freed event's work struct?

>  
> -	if (mrioc->stop_drv_processing) {
> -		dprint_event_bh(mrioc, "ignoring event(0x%02x) in the bottom half handler\n"
> -				"due to stop_drv_processing\n", fwevt->event_id);
> +	if (mrioc->stop_drv_processing || mrioc->pci_err_recovery) {

[Severity: High]
This isn't a bug introduced by this patch, but does this lockless check fully
resolve the race condition described in the commit message?

Since this check is evaluated early in the handler, could an in-flight worker
pass this check, get preempted, and then attempt to access hardware registers
concurrently with mpi3mr_error_detected() unmapping and NULLing sysif_regs?

For example, if the worker proceeds to mpi3mr_admin_request_post():

drivers/scsi/mpi3mr/mpi3mr_fw.c:mpi3mr_admin_request_post() {
        ...
        writel(mrioc->admin_req_pi, &mrioc->sysif_regs->admin_request_queue_pi);
        ...
}

Would this still result in a NULL pointer dereference or unmapped memory
access if the PCI error recovery path clears mrioc->sysif_regs after the
boolean check in mpi3mr_fwevt_bh() has already passed?

> +		dprint_event_bh(mrioc,
> +				"ignoring event(0x%02x) in the bottom half handler\n"
> +				"due to stop_drv_processing or pci_err_recovery\n",
> +				fwevt->event_id);
>  		goto out;
>  	}

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