Re: [PATCH 16/37] scsi: mpt3sas: Fix page fault from stale ioc->chip after PCI error recovery

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: mpt3sas: NULL ioc->chip dereference in teardown and reset paths
--

commit 118d4e051f29076a6276971b9fb64bd8927def06
Author: Shivasharan S <shivasharan.srikanteshwara@broadcom.com>

scsi: mpt3sas: Fix page fault from stale ioc->chip after PCI error recovery

This commit sets ioc->chip to NULL when unmapping controller resources in
mpt3sas_base_unmap_resources(), guards mpt3sas_base_mask_interrupts() and
mpt3sas_base_unmask_interrupts() against a NULL ioc->chip pointer, and skips
zero-length memory BARs in mpt3sas_base_map_resources().

> diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c
> index 060482ba20aef..f0ed1bea64bbc 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_base.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
> @@ -1645,6 +1645,8 @@ mpt3sas_base_mask_interrupts(struct MPT3SAS_ADAPTER *ioc)
>  	u32 him_register;
>  
>  	ioc->mask_interrupts = 1;
> +	if (!ioc->chip)
> +		return;
>  	him_register = ioc->base_readl(&ioc->chip->HostInterruptMask);
>  	him_register |= MPI2_HIM_DIM + MPI2_HIM_RIM + MPI2_HIM_RESET_IRQ_MASK;
>  	writel(him_register, &ioc->chip->HostInterruptMask);
> @@ -1662,6 +1664,8 @@ mpt3sas_base_unmask_interrupts(struct MPT3SAS_ADAPTER *ioc)
>  {
>  	u32 him_register;
>  
> +	if (!ioc->chip)
> +		return;
>  	him_register = ioc->base_readl(&ioc->chip->HostInterruptMask);
>  	him_register &= ~MPI2_HIM_RIM;
>  	writel(him_register, &ioc->chip->HostInterruptMask);
> @@ -3579,6 +3583,7 @@ mpt3sas_base_unmap_resources(struct MPT3SAS_ADAPTER *ioc)
>  
>  	if (ioc->chip_phys) {
>  		iounmap(ioc->chip);
> +		ioc->chip = NULL;
>  		ioc->chip_phys = 0;
>  	}

[Severity: High]
When mpt3sas_base_unmap_resources() sets ioc->chip = NULL (such as during
scsih_pci_error_detected() on pci_channel_io_frozen, scsih_suspend(), or when
mpt3sas_base_map_resources() fails in scsih_pci_slot_reset() or
scsih_resume()), can the surrounding teardown and reset paths still
dereference a NULL ioc->chip pointer?

In drivers/scsi/mpt3sas/mpt3sas_scsih.c, scsih_shutdown() and scsih_remove()
run several helpers right around the newly guarded
mpt3sas_base_mask_interrupts() call without checking if ioc->chip is NULL:

drivers/scsi/mpt3sas/mpt3sas_scsih.c:scsih_shutdown() {
	...
	if (ioc->is_aero_ioc)
		mpt3sas_config_set_ioc_pg1(ioc, &mpi_reply,
				&ioc->ioc_pg1_copy);

	_scsih_ir_shutdown(ioc);
	_scsih_nvme_shutdown(ioc);
	mpt3sas_base_mask_interrupts(ioc);
	mpt3sas_base_stop_watchdog(ioc);
	ioc->shost_recovery = 1;
	mpt3sas_base_make_ioc_ready(ioc, SOFT_RESET);
	...
}

drivers/scsi/mpt3sas/mpt3sas_scsih.c:scsih_remove() {
	...
	if (ioc->is_aero_ioc)
		mpt3sas_config_set_ioc_pg1(ioc, &mpi_reply,
				&ioc->ioc_pg1_copy);
	/* release all the volumes */
	_scsih_ir_shutdown(ioc);
	...
}

Specifically:

1. mpt3sas_config_set_ioc_pg1() (when ioc->is_aero_ioc is set) calls
_config_request() -> mpt3sas_wait_for_ioc() -> mpt3sas_base_get_iocstate(),
which reads &ioc->chip->Doorbell in drivers/scsi/mpt3sas/mpt3sas_base.c:

	s = ioc->base_readl_ext_retry(&ioc->chip->Doorbell);

2. _scsih_ir_shutdown() (when ioc->ir_firmware is set and raid_device_list is
non-empty) and _scsih_nvme_shutdown() (when pcie_device_list is non-empty)
both call ioc->put_smid_default(ioc, smid), which writes to
&ioc->chip->RequestDescriptorPostLow or
&ioc->chip->AtomicRequestDescriptorPost without checking ioc->chip or
ioc->pci_error_recovery.

3. In scsih_shutdown(), mpt3sas_base_make_ioc_ready(ioc, SOFT_RESET) is
called unconditionally without the if (ioc->chip_phys && ioc->chip) check used
in mpt3sas_base_free_resources(). Because mpt3sas_base_make_ioc_ready() only
checks ioc->pci_error_recovery and not ioc->chip, any path where ioc->chip is
NULL and ioc->pci_error_recovery is 0 (for example, after
scsih_pci_slot_reset() clears ioc->pci_error_recovery = 0 and fails
mpt3sas_base_map_resources(), or after scsih_suspend() / failed
scsih_resume()) calls mpt3sas_base_get_iocstate(ioc, 0) and dereferences
&ioc->chip->Doorbell.

4. Similarly, in mpt3sas_base_hard_reset_handler() in
drivers/scsi/mpt3sas/mpt3sas_base.c, mpt3sas_wait_for_commands_to_complete()
and mpt3sas_base_make_ioc_ready() immediately precede and follow
mpt3sas_base_mask_interrupts() and both call mpt3sas_base_get_iocstate(),
dereferencing &ioc->chip->Doorbell when ioc->chip is NULL.

Should these callers and mpt3sas_base_get_iocstate() /
mpt3sas_base_make_ioc_ready() also guard against !ioc->chip?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/1793925772-1303-1-git-send-email-shivasharan.srikanteshwara@broadcom.com?part=16