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

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

Pre-existing issues:
- [High] Missing bounds checking in PCIe topology change event handlers.
- [High] Incomplete fix: early return in SAS top-half bypasses bottom-half cancellation.
- [High] Event data length is unbounded by host DMA buffer size.
--

--- Patch [10]: [PATCH 10/17] mpi3mr: Fix out-of-bounds read in SAS topology change events ---
commit bab975c5553e402733c8341f728b4c3f336df543
Author: Chandrakanth Patil <[email protected]>

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

The number of entries in a SAS topology change event is used to walk
the entry array without being compared against the amount of event data
that was received, so the walk can run past the end of the buffer.

Work out how many entries the payload can hold and skip the event if it
claims more.

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
> index b18e7ea873030..1291c60b4d98c 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_os.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_os.c

[ ... ]

> @@ -2819,11 +2819,26 @@ static void mpi3mr_sastopochg_evt_th(struct mpi3mr_ioc *mrioc,
>  	struct mpi3_event_data_sas_topology_change_list *topo_evt =
>  	    (struct mpi3_event_data_sas_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 is a pre-existing issue, but does event_reply->event_data_length
need to be validated against the host DMA buffer size before being used?

Since event_data_length is provided by the firmware, if it exceeds the
hardware DMA reply frame size (mrioc->reply_sz), could this calculation
artificially inflate avail_len and defeat the bounds check?

Also, looking at mpi3mr_os_handle_events(), this unvalidated length
appears to be used for a memory copy:

mpi3mr_os_handle_events() {
    ...
    sz = event_reply->event_data_length * 4;
    fwevt = mpi3mr_alloc_fwevt(sz);
    ...
    memcpy(fwevt->event_data, event_reply->event_data, sz);
}

Could this result in an out-of-bounds read from the DMA reply frame pool?

> +	if (avail_len < offsetof(struct mpi3_event_data_sas_topology_change_list, phy_entry)) {
> +		ioc_err(mrioc, "SAS topology event: event data too small (%u bytes)\n",
> +			avail_len);
> +		return;

[Severity: High]
This isn't a bug introduced by this patch, but does returning early here
leave the bottom-half handler scheduled with unvalidated data?

In mpi3mr_os_handle_events(), the bottom-half scheduling flag is set
before the top-half runs:

mpi3mr_os_handle_events() {
    ...
    process_evt_bh = 1;
    mpi3mr_sastopochg_evt_th(mrioc, event_reply);
}

If we hit this early return (or the one below), process_evt_bh remains 1.
When mpi3mr_sastopochg_evt_bh() subsequently runs, it will iterate over the
unvalidated event_data->num_entries, potentially reading out-of-bounds.

Should this function clear the bottom-half scheduling flag or return an error
code when validation fails?

> +	}
> +	max_entries = (avail_len -
> +		      offsetof(struct mpi3_event_data_sas_topology_change_list, phy_entry)) /
> +		      sizeof(struct mpi3_event_sas_topo_phy_entry);
> +	if (topo_evt->num_entries > max_entries) {
> +		ioc_err(mrioc, "SAS topology event: num_entries(%d) exceeds max(%d)\n",
> +			topo_evt->num_entries, max_entries);
> +		return;
> +	}
> +
>  	for (i = 0; i < topo_evt->num_entries; i++) {
>  		handle = le16_to_cpu(topo_evt->phy_entry[i].attached_dev_handle);
>  		if (!handle)

[Severity: High]
This is a pre-existing issue, but should a similar bounds check be applied
to the PCIe topology change events?

Looking at mpi3mr_pcietopochg_evt_th() and mpi3mr_pcietopochg_evt_bh(),
both handlers blindly iterate over the variable-length port_entry array
using the unvalidated num_entries provided by firmware:

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

Could a maliciously large num_entries provided by a device cause an
out-of-bounds memory read here as well?

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