Re: [PATCH 08/37] scsi: mpt3sas: Validate firmware/user supplied device handles against MaxDevHandle

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: mpt3sas: infinite retry loop when _scsih_add_device() returns -1
--

commit 366cf2a2a69371d1bf988949aa77181dfdc2bddd
Author: Shivasharan S <shivasharan.srikanteshwara@broadcom.com>

scsi: mpt3sas: Validate firmware/user supplied device handles against MaxDevHandle

This commit validates firmware-supplied and ioctl-supplied device handles
against ioc->facts.MaxDevHandle across event handling, device discovery,
post-reset scanning, and task management/control completion paths. It also
changes the passthrough handle boundary check in _ctl_do_mpt_command() from
> to >=.

> diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> index 8dcbe60db542e..a697200844078 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c

[ ... ]

> @@ -8399,6 +8418,12 @@ _scsih_add_device(struct MPT3SAS_ADAPTER *ioc, u16 handle, u8 retry_count,
>  	u8 connector_name[5];
>  	u8 port_id;
>  
> +	if (handle >= ioc->facts.MaxDevHandle) {
> +		ioc_err(ioc, "handle(0x%04x) out of bounds in %s\n",
> +			handle, __func__);
> +		return -1;
> +	}

[Severity: High]
Does returning -1 here cause an infinite retry loop in callers that treat
any non-zero return from _scsih_add_device() as a retryable condition?

In _scsih_scan_for_devices_after_reset(), the SAS end-device scan loop
reads handle = le16_to_cpu(sas_device_pg0.DevHandle) without checking it
against ioc->facts.MaxDevHandle before calling _scsih_add_device():

	retry_count = 0;
	/* This will retry adding the end device.
	 * _scsih_add_device() will decide on retries and
	 * return "1" when it should be retried
	 */
	while (_scsih_add_device(ioc, handle, retry_count++,
	    0)) {
		ssleep(1);
	}

Similarly, _scsih_sas_topology_change_event() checks the return value when
handling MPI2_EVENT_SAS_TOPO_RC_TARG_ADDED:

	rc = _scsih_add_device(ioc, handle,
	    fw_event->retries[i], 0);
	if (rc) {/* retry due to busy device */
		fw_event->retries[i]++;
		requeue_event = 1;
	} else {/* mark entry vacant */
		event_data->PHY[i].PhyStatus |=
	    MPI2_EVENT_SAS_TOPO_PHYSTATUS_VACANT;
	}

Because _scsih_add_device() returns -1 at the start of the function before
retry_count is checked in _scsih_wait_for_target_to_become_ready(),
wouldn't an out-of-bounds handle cause _scsih_scan_for_devices_after_reset()
to loop forever holding ioc->reset_in_progress_mutex, and cause
_scsih_sas_topology_change_event() to requeue fw_event indefinitely?

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