Re: [PATCH v2 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:

Pre-existing issues:
- [High] The bounds check blindly trusts `event_data_length` provided by the hardware without clamping it to the physical size of the DMA reply buffer, failing to prevent out-of-bounds reads in the top-half handlers.
- [High] The patch adds bounds checking to the top-half handler, but fails to prevent the bottom-half handler from being scheduled and iterating out-of-bounds over the same malformed event data.
- [High] The compiler-inserted bounds checks from the `__counted_by` attribute bypass the local variable cache, re-introducing a TOCTOU vulnerability.
- [High] Lockless access to `tgtdev->starget` and `starget->hostdata` leads to a Use-After-Free vulnerability.
--

commit 63bdf6fdca71d0debbf3d9facb074c2cc5e861c5
Author: Chandrakanth Patil <[email protected]>

mpi3mr: Fix out-of-bounds read in PCIe topology change events

This commit bounds the entry count to the received event data before use to
prevent out-of-bounds reads when processing PCIe topology change events.

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
> index 65a6192cf3960..543e9b8e3fc37 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_os.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
> @@ -2761,12 +2761,28 @@ 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, num_entries;
>  	struct mpi3mr_tgt_dev *tgtdev = NULL;
>  	struct mpi3mr_stgt_priv_data *scsi_tgt_priv_data = NULL;
>  
> -	for (i = 0; i < topo_evt->num_entries; i++) {
> +	avail_len = event_reply->event_data_length * 4;

[Severity: High]
This is a pre-existing issue, but the patch provides an incomplete fix. Does
this code blindly trust event_data_length provided by the hardware without
clamping it to the physical size of the DMA reply buffer?

If a malicious or buggy hardware controller sends a PCIe topology change
event with a large event_data_length (e.g., 255) and a large num_entries,
avail_len could become up to 1020, exceeding the actual physical buffer size
(mrioc->reply_sz, typically 128 bytes).

Since avail_len is not clamped, max_entries would become artificially large,
bypassing the loop bounds check. Could this result in an out-of-bounds read
of kernel memory adjacent to the DMA reply buffer?

> +	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);
> +	num_entries = topo_evt->num_entries;
> +	if (num_entries > max_entries) {
> +		ioc_err(mrioc, "PCIe topology event: num_entries(%d) exceeds max(%d)\n",
> +			num_entries, max_entries);
> +		return;

[Severity: High]
This isn't a bug introduced by this patch, but the patch's attempt to fix the
bounds check might be bypassed in the bottom half. If the top-half handler
returns early here, what happens to process_evt_bh which was set to 1 prior
to this call by mpi3mr_os_handle_events()?

If process_evt_bh is not cleared, wouldn't the bottom-half handler
mpi3mr_pcietopochg_evt_bh() still be scheduled and iterate out-of-bounds over
the same malformed event data?

mpi3mr_pcietopochg_evt_bh():
	for (i = 0; i < event_data->num_entries; i++) {

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

[Severity: High]
This isn't a bug introduced by this patch, but the patch's attempt to fix the
TOCTOU vulnerability might be nullified. Even though num_entries is cached in
a local variable, doesn't accessing topo_evt->port_entry[i] trigger a
compiler-inserted bounds check because of the __counted_by(num_entries)
attribute in the structure definition?

Under CONFIG_UBSAN_BOUNDS, this would force the compiler to re-read
topo_evt->num_entries directly from DMA memory to perform the bounds check.

If a malicious device modifies this value post-validation, could it trigger
a bounds check failure and panic the kernel?

>  		if (!handle)
>  			continue;
>  		reason_code = topo_evt->port_entry[i].port_status;
>  		scsi_tgt_priv_data =  NULL;
>  		tgtdev = mpi3mr_get_tgtdev_by_handle(mrioc, handle);
>  		if (tgtdev && tgtdev->starget && tgtdev->starget->hostdata)

[Severity: High]
This is a pre-existing issue that was not addressed, but is it safe to access
tgtdev->starget and starget->hostdata locklessly here?

If mpi3mr_target_destroy() executes concurrently, it can take the lock, set
tgtdev->starget = NULL, release the lock, and call kfree(starget->hostdata).

Could the top-half handler race with this destruction and lead to a
Use-After-Free when dereferencing scsi_tgt_priv_data immediately afterwards?

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