Re: [PATCH 37/37] scsi: mpt3sas: Update driver version to 59.100.00.00

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] scsi: mpt3sas: expander link updates broken in mpt3sas_transport_update_links()
- [High] scsi: mpt3sas: infinite retry loop on invalid handle in _scsih_add_device()
- [High] scsi: mpt3sas: out-of-bounds read on short events in mpt3sas_scsih_event_callback()
- [Medium] scsi: mpt3sas: u16 underflow of scsiio_depth in _base_allocate_memory_pools()
- [Medium] scsi: mpt3sas: signed count bypasses clamp in driver trigger page helpers
- [Medium] scsi: mpt3sas: shift and u32 multiply overflow from CurrentHostPageSize
--

commit 4e069d24b1cbcc20ec25652dd7863f765956c610
Author: Shivasharan S <shivasharan.srikanteshwara@broadcom.com>

scsi: mpt3sas: Update driver version to 59.100.00.00

This series hardens bounds, size, and handle validation across the mpt3sas
driver and updates the driver version to 59.100.00.00.

> diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c
> index 1af25a22611a1..7cc849dbf7060 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_base.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c

[ ... ]

> @@ -6434,6 +6477,13 @@ _base_allocate_memory_pools(struct MPT3SAS_ADAPTER *ioc)
>  				(ioc->reply_post_queue_depth % 16);
>  	}
>  
> +	if (facts->MaxReplyDescriptorPostQueueDepth <
> +	    MPT3SAS_MIN_REPLY_POST_QUEUE_DEPTH) {
> +		ioc_err(ioc, "MaxReplyDescriptorPostQueueDepth is too small (%u)\n",
> +			facts->MaxReplyDescriptorPostQueueDepth);
> +		return -EINVAL;
> +	}
> +
>  	if (ioc->reply_post_queue_depth >
>  	    facts->MaxReplyDescriptorPostQueueDepth) {
>  		ioc->reply_post_queue_depth =

[Severity: Medium]
Can this minimum check still allow an unsigned underflow of ioc->scsiio_depth
further down in _base_allocate_memory_pools()?

MPT3SAS_MIN_REPLY_POST_QUEUE_DEPTH is 80, which gives ioc->hba_queue_depth =
((80 - 64) / 2) - 1 = 7 when ioc->reply_post_queue_depth is clamped to
facts->MaxReplyDescriptorPostQueueDepth.

Earlier in _base_allocate_memory_pools(), ioc->internal_depth is set to at
least INTERNAL_CMDS_COUNT (10) and ioc->hi_priority_depth is set to
ioc->internal_depth - 5 (at least 5):

drivers/scsi/mpt3sas/mpt3sas_base.c:_base_allocate_memory_pools() {
	...
	if (ioc->internal_depth < INTERNAL_CMDS_COUNT) {
		...
		ioc->internal_depth = 10;
	}

	ioc->hi_priority_depth = ioc->internal_depth - (5);
	...
	ioc->scsiio_depth = ioc->hba_queue_depth -
	    ioc->hi_priority_depth - ioc->internal_depth;

	ioc->shost->can_queue = ioc->scsiio_depth - INTERNAL_SCSIIO_CMDS_COUNT;
}

When ioc->hba_queue_depth is less than ioc->hi_priority_depth +
ioc->internal_depth + INTERNAL_SCSIIO_CMDS_COUNT (at least 18), subtracting
ioc->hi_priority_depth and ioc->internal_depth wraps the u16 ioc->scsiio_depth
around to 65528 and sets ioc->shost->can_queue to 65525.

[ ... ]

> @@ -7507,8 +7557,15 @@ _base_get_ioc_facts(struct MPT3SAS_ADAPTER *ioc)
>  	facts->CurrentHostPageSize = mpi_reply.CurrentHostPageSize;
>  
>  	/*
> -	 * Get the Page Size from IOC Facts. If it's 0, default to 4k.
> +	 * Get the Page Size from IOC Facts. If it's 0 or too large to be a
> +	 * valid shift amount, default to 4k.
>  	 */
> +	if ((facts->CurrentHostPageSize >= 32) ||
> +	    (facts->CurrentHostPageSize != 0 && facts->CurrentHostPageSize < 12)) {
> +		ioc_warn(ioc, "CurrentHostPageSize(%d) is invalid: Setting default host page size to 4k\n",
> +			 facts->CurrentHostPageSize);
> +		facts->CurrentHostPageSize = MPT3SAS_HOST_PAGE_SIZE_4K;
> +	}
>  	ioc->page_size = 1 << facts->CurrentHostPageSize;
>  	if (ioc->page_size == 1) {
>  		ioc_info(ioc, "CurrentHostPageSize is 0: Setting default host page size to 4k\n");

[Severity: Medium]
When facts->CurrentHostPageSize is 31, doesn't 1 << facts->CurrentHostPageSize
overflow a signed 32-bit int?

Also, if facts->CurrentHostPageSize is allowed up to 31, can the 32-bit
multiplication ioc->shost->sg_tablesize * ioc->page_size in
_ctl_do_mpt_command() wrap around?

> diff --git a/drivers/scsi/mpt3sas/mpt3sas_config.c b/drivers/scsi/mpt3sas/mpt3sas_config.c
> index b0d5ef8936008..271d7dff7c079 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_config.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_config.c

[ ... ]

> @@ -2278,6 +2290,8 @@ mpt3sas_config_update_driver_trigger_pg2(struct MPT3SAS_ADAPTER *ioc,
>  
>  	if (set) {
>  		count = event_tg->ValidEntries;
> +		if (count > NUM_VALID_ENTRIES)
> +			count = NUM_VALID_ENTRIES;
>  		tg_pg2.NumMPIEventTrigger = cpu_to_le16(count);
>  		for (i = 0; i < count; i++) {
>  			tg_pg2.MPIEventTriggers[i].MPIEventCode =
> @@ -2437,6 +2451,8 @@ mpt3sas_config_update_driver_trigger_pg3(struct MPT3SAS_ADAPTER *ioc,
>  
>  	if (set) {
>  		count = scsi_tg->ValidEntries;
> +		if (count > NUM_VALID_ENTRIES)
> +			count = NUM_VALID_ENTRIES;
>  		tg_pg3.NumSCSISenseTrigger = cpu_to_le16(count);
>  		for (i = 0; i < count; i++) {
>  			tg_pg3.SCSISenseTriggers[i].ASCQ =
> @@ -2595,6 +2611,8 @@ mpt3sas_config_update_driver_trigger_pg4(struct MPT3SAS_ADAPTER *ioc,
>  
>  	if (set) {
>  		count = mpi_tg->ValidEntries;
> +		if (count > NUM_VALID_ENTRIES)
> +			count = NUM_VALID_ENTRIES;
>  		tg_pg4.NumIOCStatusLogInfoTrigger = cpu_to_le16(count);
>  		for (i = 0; i < count; i++) {
>  			tg_pg4.IOCStatusLoginfoTriggers[i].IOCStatus =

[Severity: Medium]
In mpt3sas_config_update_driver_trigger_pg2(),
mpt3sas_config_update_driver_trigger_pg3(), and
mpt3sas_config_update_driver_trigger_pg4(), count is declared as a signed int
while ValidEntries is a uint32_t.

If userspace passes ValidEntries >= 0x80000000 via the diag_trigger_event,
diag_trigger_scsi, or diag_trigger_mpi sysfs attributes, won't count become
negative, bypass the count > NUM_VALID_ENTRIES check, and write the truncated
16-bit value into NumMPIEventTrigger, NumSCSISenseTrigger, or
NumIOCStatusLogInfoTrigger?

> diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
> index 8bb947004885b..1087802368c5b 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c

[ ... ]

> @@ -862,6 +899,21 @@ _ctl_do_mpt_command(struct MPT3SAS_ADAPTER *ioc, struct mpt3_ioctl_command karg,
>  			ret = -EINVAL;
>  			goto out;
>  		}
> +		/*
> +		 * Reject an oversized transfer, since building the PRP list
> +		 * would overrun this command's fixed buffer, corrupting
> +		 * others sharing it.
> +		 */
> +		if (data_out_sz > (ioc->shost->sg_tablesize * ioc->page_size) ||
> +		    data_in_sz > (ioc->shost->sg_tablesize * ioc->page_size)) {
> +			dtmprintk(ioc, ioc_info(ioc,
> +				  "%s: data_in_size/data_out_size exceeds NVMe PRP buffer capacity\n",
> +				  __func__));
> +			mpt3sas_base_free_smid(ioc, smid);
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +

[Severity: Medium]
Since ioc->shost->sg_tablesize is an unsigned short and ioc->page_size is a
u32, this product is evaluated in 32-bit unsigned arithmetic. If
facts->CurrentHostPageSize is large (up to 31 in _base_get_ioc_facts()), can
ioc->shost->sg_tablesize * ioc->page_size overflow 32 bits?

> diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> index 271d7dff7c079..8bb947004885b 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c

[ ... ]

> @@ -8399,6 +8426,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;
> +	}
> +
>  	if ((mpt3sas_config_get_sas_device_pg0(ioc, &mpi_reply, &sas_device_pg0,
>  	    MPI2_SAS_DEVICE_PGAD_FORM_HANDLE, handle))) {
>  		ioc_err(ioc, "failure at %s:%d/%s()!\n",

[Severity: High]
Does returning -1 here cause callers to retry forever when handle >=
ioc->facts.MaxDevHandle?

Callers of _scsih_add_device() treat any non-zero return value as a request to
retry adding a busy device:

drivers/scsi/mpt3sas/mpt3sas_scsih.c:_scsih_scan_for_devices_after_reset() {
	...
			/* 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);
			}
}

and in _scsih_sas_topology_change_event():

drivers/scsi/mpt3sas/mpt3sas_scsih.c:_scsih_sas_topology_change_event() {
	...
			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;
			}
}

Because the handle >= ioc->facts.MaxDevHandle check in _scsih_add_device()
returns -1 on every call regardless of retry_count, wouldn't
_scsih_scan_for_devices_after_reset() loop indefinitely and
_scsih_sas_topology_change_event() requeue fw_event indefinitely instead of
dropping the invalid handle by returning 0?

[ ... ]

> @@ -12144,9 +12220,21 @@ mpt3sas_scsih_event_callback(struct MPT3SAS_ADAPTER *ioc, u8 msix_index,
>  	}
>  
>  	case MPI2_EVENT_SAS_TOPOLOGY_CHANGE_LIST:
> -		_scsih_check_topo_delete_events(ioc,
> +	{
> +		Mpi2EventDataSasTopologyChangeList_t *topo_data =
>  		    (Mpi2EventDataSasTopologyChangeList_t *)
> -		    mpi_reply->EventData);
> +		    mpi_reply->EventData;
> +		u32 data_len = le16_to_cpu(mpi_reply->EventDataLength) * 4;
> +		size_t hdr_size =
> +		    offsetof(Mpi2EventDataSasTopologyChangeList_t, PHY);
> +		u16 max_entries = 0;
> +
> +		if (data_len > hdr_size)
> +			max_entries = (data_len - hdr_size) /
> +			    sizeof(Mpi2EventSasTopoPhyEntry_t);
> +		if (topo_data->NumEntries > max_entries)
> +			topo_data->NumEntries = max_entries;
> +		_scsih_check_topo_delete_events(ioc, topo_data);
>  		/*
>  		 * No need to add the topology change list
>  		 * event to fw event work queue when
> @@ -12158,18 +12246,45 @@ mpt3sas_scsih_event_callback(struct MPT3SAS_ADAPTER *ioc, u8 msix_index,
>  		if (ioc->shost_recovery)
>  			return 1;
>  		break;
> +	}
>  	case MPI2_EVENT_PCIE_TOPOLOGY_CHANGE_LIST:
> -	_scsih_check_pcie_topo_remove_events(ioc,
> +	{
> +		Mpi26EventDataPCIeTopologyChangeList_t *pcie_topo_data =
>  		    (Mpi26EventDataPCIeTopologyChangeList_t *)
> -		    mpi_reply->EventData);
> +		    mpi_reply->EventData;
> +		u32 data_len = le16_to_cpu(mpi_reply->EventDataLength) * 4;
> +		size_t hdr_size =
> +		    offsetof(Mpi26EventDataPCIeTopologyChangeList_t, PortEntry);
> +		u16 max_entries = 0;
> +
> +		if (data_len > hdr_size)
> +			max_entries = (data_len - hdr_size) /
> +			    sizeof(MPI26_EVENT_PCIE_TOPO_PORT_ENTRY);
> +		if (pcie_topo_data->NumEntries > max_entries)
> +			pcie_topo_data->NumEntries = max_entries;
> +		_scsih_check_pcie_topo_remove_events(ioc, pcie_topo_data);
>  		if (ioc->shost_recovery)
>  			return 1;
>  		break;
> +	}
>  	case MPI2_EVENT_IR_CONFIGURATION_CHANGE_LIST:
> -		_scsih_check_ir_config_unhide_events(ioc,
> +	{
> +		Mpi2EventDataIrConfigChangeList_t *ir_config_data =
>  		    (Mpi2EventDataIrConfigChangeList_t *)
> -		    mpi_reply->EventData);
> +		    mpi_reply->EventData;
> +		u32 data_len = le16_to_cpu(mpi_reply->EventDataLength) * 4;
> +		size_t hdr_size =
> +		    offsetof(Mpi2EventDataIrConfigChangeList_t, ConfigElement);
> +		u16 max_elements = 0;
> +
> +		if (data_len > hdr_size)
> +			max_elements = (data_len - hdr_size) /
> +			    sizeof(MPI2_EVENT_IR_CONFIG_ELEMENT);
> +		if (ir_config_data->NumElements > max_elements)
> +			ir_config_data->NumElements = max_elements;
> +		_scsih_check_ir_config_unhide_events(ioc, ir_config_data);
>  		break;
> +	}

[Severity: High]
What happens here if data_len < hdr_size for MPI2_EVENT_SAS_TOPOLOGY_CHANGE_LIST,
MPI2_EVENT_PCIE_TOPOLOGY_CHANGE_LIST, or MPI2_EVENT_IR_CONFIGURATION_CHANGE_LIST?

When data_len < hdr_size, the event is not rejected, so execution continues to
alloc_fw_event_work(sz) where sz = le16_to_cpu(mpi_reply->EventDataLength) * 4:

drivers/scsi/mpt3sas/mpt3sas_scsih.c:mpt3sas_scsih_event_callback() {
	...
	sz = le16_to_cpu(mpi_reply->EventDataLength) * 4;
	fw_event = alloc_fw_event_work(sz);
	...
	memcpy(fw_event->event_data, mpi_reply->EventData,
	    mpi_reply->EventDataLength * 4);
}

Because fw_event->event_data is allocated with only sz bytes (smaller than
hdr_size), won't the workqueue handlers _scsih_sas_topology_change_event(),
_scsih_pcie_topology_change_event(), and _scsih_sas_ir_config_change_event()
(as well as _scsih_check_topo_delete_events() and
_scsih_check_pcie_topo_remove_events() when walking queued fw_event_list
entries) read header fields such as ExpanderDevHandle, SwitchDevHandle,
NumEntries, or NumElements past the end of fw_event->event_data?

> diff --git a/drivers/scsi/mpt3sas/mpt3sas_transport.c b/drivers/scsi/mpt3sas/mpt3sas_transport.c
> index e74a526efa8db..4ae11f2260143 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_transport.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_transport.c

[ ... ]

> @@ -1176,6 +1178,11 @@ mpt3sas_transport_update_links(struct MPT3SAS_ADAPTER *ioc,
>  		return;
>  	}
>  
> +	if (phy_number >= sas_node->nr_phys_allocated) {
> +		spin_unlock_irqrestore(&ioc->sas_node_lock, flags);
> +		return;
> +	}
> +
>  	mpt3sas_phy = &sas_node->phy[phy_number];
>  	mpt3sas_phy->attached_handle = handle;
>  	spin_unlock_irqrestore(&ioc->sas_node_lock, flags);

[Severity: High]
Doesn't checking phy_number >= sas_node->nr_phys_allocated break link updates
for all SAS expanders?

In mpt3sas_transport_update_links(), _transport_sas_node_find_by_sas_address()
returns either &ioc->sas_hba or a SAS expander node from
ioc->sas_expander_list. While _scsih_sas_host_add() and
_scsih_sas_host_refresh() set ioc->sas_hba.nr_phys_allocated,
_scsih_expander_add() only sets sas_expander->num_phys and leaves
sas_expander->nr_phys_allocated at 0:

drivers/scsi/mpt3sas/mpt3sas_scsih.c:_scsih_expander_add() {
	...
	sas_expander = kzalloc_obj(struct _sas_node);
	...
	sas_expander->handle = handle;
	sas_expander->num_phys = expander_pg0.NumPhys;
	...
	sas_expander->phy = kzalloc_objs(struct _sas_phy,
					 sas_expander->num_phys);
}

Whenever sas_node is an expander, sas_node->nr_phys_allocated is 0, so
phy_number >= sas_node->nr_phys_allocated is always true and
mpt3sas_transport_update_links() returns early without updating the expander
phy link state.

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