Re: [PATCH 14/17] mpi3mr: Fix out-of-bounds read in PCIe topology change events

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

New issues:
- [High] The newly added bounds check reads `topo_evt->num_entries` directly from a DMA-mapped buffer, and the subsequent loop condition re-reads it, creating a Time-Of-Check to Time-Of-Use (TOCTOU) race condition.

Pre-existing issues:
- [High] The bounds check for PCIe and SAS topology change events fails to protect the bottom-half event handlers from out-of-bounds reads because early returns in the top-half do not prevent the malformed event from being queued.
- [High] The bounds check uses an untrusted `event_data_length` without verifying it against the actual physical dimensions of the hardware reply buffer, allowing an out-of-bounds read in the top-half handler.
- [High] Missing `dma_rmb()` after polling the phase bit in DMA-mapped reply descriptors, which can lead to processing stale descriptor data on weakly ordered architectures.
--

--- Patch [14]: [PATCH 14/17] mpi3mr: Fix out-of-bounds read in PCIe topology change events ---
commit 8553af34c1ff3c0cff0462ca66842979af8cb4d4
Author: Chandrakanth Patil <[email protected]>

mpi3mr: Fix out-of-bounds read in PCIe topology change events
    
The number of entries in a PCIe topology change event is used to walk
the entry array without being compared against the amount of event
data that was received. Bound the entry count to the received event data
before use.

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
> --- a/drivers/scsi/mpi3mr/mpi3mr_os.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
> @@ -2753,11 +2753,26 @@ static void mpi3mr_pcietopochg_evt_th(struct mpi3mr_ioc *mrioc,
>  	struct mpi3_event_data_pcie_topology_change_list *topo_evt =
>  	    (struct mpi3_event_data_pcie_topology_change_list *)event_reply->event_data;
>  	int i;
> -	u16 handle;
> -	u8 reason_code;
> +	u16 handle, avail_len;
> +	u8 reason_code, max_entries;
>  	struct mpi3mr_tgt_dev *tgtdev = NULL;
>  	struct mpi3mr_stgt_priv_data *scsi_tgt_priv_data = NULL;
>  
> +	avail_len = event_reply->event_data_length * 4;

[Severity: High]
This isn't a bug introduced by this patch, but could this calculation
exceed the actual physical dimensions of the hardware reply buffer?

Because avail_len is derived strictly from the untrusted event_data_length,
a compromised or malicious device could provide a value larger than the
allocated DMA reply buffer (mrioc->reply_sz).

Should we verify event_data_length against mrioc->reply_sz to prevent
the bounds check from being artificially inflated, which would lead to an
out-of-bounds read? 

The SAS topology handler mpi3mr_sastopochg_evt_th() appears to share this
exact pattern.

> +	if (avail_len < offsetof(struct mpi3_event_data_pcie_topology_change_list, port_entry)) {
> +		ioc_err(mrioc, "PCIe topology event: event data too small (%u bytes)\n",
> +			avail_len);
> +		return;
> +	}
> +	max_entries = (avail_len -
> +		      offsetof(struct mpi3_event_data_pcie_topology_change_list, port_entry)) /
> +		      sizeof(struct mpi3_event_pcie_topo_port_entry);
> +	if (topo_evt->num_entries > max_entries) {
> +		ioc_err(mrioc, "PCIe topology event: num_entries(%d) exceeds max(%d)\n",
> +			topo_evt->num_entries, max_entries);
> +		return;
> +	}

[Severity: High]
This is a pre-existing issue, but does this early return actually prevent
the malformed event from being processed by the bottom-half handler?

When mpi3mr_pcietopochg_evt_th() returns early here, the process_evt_bh flag
remains set in mpi3mr_os_handle_events():

drivers/scsi/mpi3mr/mpi3mr_os.c:mpi3mr_os_handle_events() {
    ...
	case MPI3_EVENT_PCIE_TOPOLOGY_CHANGE_LIST:
	{
		process_evt_bh = 1;
		mpi3mr_pcietopochg_evt_th(mrioc, event_reply);
		break;
	}
    ...
}

Since process_evt_bh remains 1, the event is queued to the worker thread.
The bottom-half handler mpi3mr_pcietopochg_evt_bh() will then blindly
iterate up to the unvalidated event_data->num_entries, causing an
out-of-bounds read past the end of the allocated fwevt heap buffer.

Would it be safer to clear process_evt_bh or return a status from the
top-half handler to prevent queuing malformed events? The SAS topology
handler mpi3mr_sastopochg_evt_bh() is likely vulnerable to the exact
same incomplete fix flaw.

>  
>  	for (i = 0; i < topo_evt->num_entries; i++) {
>  		handle = le16_to_cpu(topo_evt->port_entry[i].attached_dev_handle);
>  		if (!handle)

[Severity: High]
Is this loop vulnerable to a Time-Of-Check to Time-Of-Use (TOCTOU) race
condition?

The newly added bounds check reads topo_evt->num_entries directly from a
DMA-mapped buffer, and this loop condition continually re-evaluates it. 

Because the loop body contains functions that acquire spinlocks
(e.g., mpi3mr_get_tgtdev_by_handle()), which act as full compiler memory
barriers, the compiler is forced to re-fetch num_entries from DMA memory
on every iteration.

If a malicious device alters num_entries in DMA memory after the initial
max_entries check, could it extend the loop beyond bounds? 

Should we cache num_entries in a local variable before the bounds check
and use the local variable for the loop condition? The SAS topology handler
mpi3mr_sastopochg_evt_th() appears to have the identical issue.

[Severity: High]
This isn't a bug introduced by this patch, but is a dma_rmb() barrier needed
after polling the phase bit in the DMA-mapped reply descriptors?

Looking at mpi3mr_process_op_reply_q() in drivers/scsi/mpi3mr/mpi3mr_fw.c:

drivers/scsi/mpi3mr/mpi3mr_fw.c:mpi3mr_process_op_reply_q() {
    ...
	if ((le16_to_cpu(reply_desc->reply_flags) &
	    MPI3_REPLY_DESCRIPT_FLAGS_PHASE_MASK) != exp_phase) {
		atomic_dec(&op_reply_q->in_use);
		return 0;
	}

	do {
		if (mrioc->unrecoverable || mrioc->io_admin_reset_sync)
			break;

		req_q_idx = le16_to_cpu(reply_desc->request_queue_id) - 1;
    ...
}

The driver polls the hardware DMA ring by checking the reply_flags phase mask.
Without a subsequent dma_rmb(), weakly ordered CPU architectures (such as
ARM64) can speculatively execute out-of-order reads and fetch the rest of
the descriptor payload (like request_queue_id) before the hardware has
actually committed the updated descriptor.

Could this result in processing corrupted or stale descriptor payloads?
There appears to be a similar polling loop for the admin reply queue in
mpi3mr_process_admin_reply_q() lacking a dma_rmb() before accessing
request_queue_ci.

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