Re: [PATCH 09/37] scsi: mpt3sas: Clamp topology/IR event entry counts to EventDataLength

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] scsi: mpt3sas: u16 sz truncation bypasses u32 data_len clamp in mpt3sas_scsih_event_callback()
- [High] scsi: mpt3sas: out-of-bounds access when EventDataLength is smaller than hdr_size
--

commit 6f4d73dbcac7a66faf3928944f8f083832f7bc3b
Author: Shivasharan S <shivasharan.srikanteshwara@broadcom.com>

scsi: mpt3sas: Clamp topology/IR event entry counts to EventDataLength

This commit clamps NumEntries and NumElements in SAS topology, PCIe topology,
and IR configuration change events to the maximum number of entries that fit
within EventDataLength.

> diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> index a697200844078..4730e2d344f00 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> @@ -12212,9 +12212,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;

[Severity: High]
Can a 16-bit truncation of sz later in mpt3sas_scsih_event_callback() bypass
this u32 data_len clamp and still overflow fw_event->event_data[]?

Here and in the MPI2_EVENT_PCIE_TOPOLOGY_CHANGE_LIST and
MPI2_EVENT_IR_CONFIGURATION_CHANGE_LIST cases, data_len is computed as a u32,
whereas sz used for allocating and populating fw_event->event_data is declared
as a u16 in mpt3sas_scsih_event_callback():

	u16 event;
	u16 sz;
	...
	sz = le16_to_cpu(mpi_reply->EventDataLength) * 4;
	fw_event = alloc_fw_event_work(sz);
	...
	memcpy(fw_event->event_data, mpi_reply->EventData, sz);

If EventDataLength is >= 0x4000 (for example, 0x4003), data_len is 65548, so
max_entries is 8192 and an 8-bit NumEntries or NumElements value (up to 255)
is not clamped. However, sz truncates modulo 65536 to 12 bytes, so
alloc_fw_event_work(sz) allocates only 12 bytes for fw_event->event_data and
copies the header containing the unclamped NumEntries or NumElements. When
_mpt3sas_fw_work() later runs _scsih_sas_topology_change_event(),
_scsih_pcie_topology_change_event(), or _scsih_sas_ir_config_change_event(),
won't the loops over PHY[], PortEntry[], or ConfigElement[] read and write
past the end of fw_event->event_data?

> +		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);

[Severity: High]
What happens if EventDataLength is smaller than hdr_size (for example,
EventDataLength is 0 or 1, so data_len < 12 for SAS/PCIe topology events or
data_len < 8 for IR configuration change events)?

When data_len < hdr_size, max_entries (or max_elements) is 0, but the event is
not rejected before calling _scsih_check_topo_delete_events(),
_scsih_check_pcie_topo_remove_events(), or
_scsih_check_ir_config_unhide_events(), and execution continues to allocate
and queue fw_event with sz = data_len:

	sz = le16_to_cpu(mpi_reply->EventDataLength) * 4;
	fw_event = alloc_fw_event_work(sz);
	...
	memcpy(fw_event->event_data, mpi_reply->EventData, sz);

Because memcpy() only copies sz bytes into fw_event->event_data, fixed header
fields such as ExpStatus/SwitchStatus (at offset 10), NumEntries (at offset 8),
or Flags (at offset 4) lie beyond the copied payload and, when sz is 0 or 4,
beyond the end of the fw_event slab allocation. When
_scsih_sas_topology_change_event(), _scsih_pcie_topology_change_event(), or
_scsih_sas_ir_config_change_event() (as well as the fw_event_list walks in
_scsih_check_topo_delete_events() and _scsih_check_pcie_topo_remove_events())
access those header fields in fw_event->event_data, can this read out of
bounds past fw_event->event_data?

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