Re: [PATCH v2 11/17] mpi3mr: Fix out-of-bounds read of event data

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

New issues:
- [High] Heap out-of-bounds read in bottom-half topology handlers due to newly clamped allocation size without clamped logical array counts.

Pre-existing issues:
- [High] MPI3_EVENT_DEVICE_INFO_CHANGED events are erroneously processed as log data and entirely bypass bottom-half processing.
- [High] Incomplete fix leaves DMA buffer out-of-bounds reads unpatched in top-half topology event handlers.
- [High] Bottom-half event handlers risk out-of-bounds slab reads by blindly casting event data without checking length.
--

commit bbda3bacb81906126bd46f33bc2d5656197ac85e
Author: Chandrakanth Patil <[email protected]>

mpi3mr: Fix out-of-bounds read of event data

This commit restricts the size of event data read from the reply buffer to
prevent reading past the end of the frame when caching log data or allocating
the buffer for bottom-half event processing.

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
> index 6a80b784200b3..65a6192cf3960 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_os.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_os.c

[ ... ]

> @@ -3179,7 +3179,12 @@ void mpi3mr_os_handle_events(struct mpi3mr_ioc *mrioc,
>  	case MPI3_EVENT_DEVICE_INFO_CHANGED:
>  	case MPI3_EVENT_LOG_DATA:

[Severity: High]
This is a pre-existing issue, but is it intentional that
MPI3_EVENT_DEVICE_INFO_CHANGED falls through to MPI3_EVENT_LOG_DATA?

When this happens, the payload is passed to mpi3mr_app_save_logdata_th()
and process_evt_bh is not set to 1. This appears to cause the bottom-half
event to never be scheduled, and might overwrite firmware log buffers with
unrelated device information page data.

> -		sz = event_reply->event_data_length * 4;
> +		if (mrioc->reply_sz > offsetof(struct mpi3_event_notification_reply, event_data))
> +			avail_reply_room = mrioc->reply_sz -
> +			    offsetof(struct mpi3_event_notification_reply, event_data);
> +		else
> +			avail_reply_room = 0;
> +		sz = min_t(u16, event_reply->event_data_length * 4, avail_reply_room);
>  		mpi3mr_app_save_logdata_th(mrioc,
>  			(char *)event_reply->event_data, sz);
>  		break;

[Severity: High]
This is a pre-existing issue, but while this fixes the buffer copy size
here, does it leave out-of-bounds reads unpatched in the top-half topology
event handlers?

In mpi3mr_pcietopochg_evt_th(), the bounds checking still uses the
unclamped firmware-provided length directly:

avail_len = event_reply->event_data_length * 4;
...
max_entries = (avail_len - ...) / sizeof(...);

If the event_data_length provided by firmware is larger than the physical
DMA reply frame size, could this lead to reading out of bounds directly
from the DMA mapping during the top-half processing loop?

[ ... ]

> @@ -3213,7 +3218,12 @@ void mpi3mr_os_handle_events(struct mpi3mr_ioc *mrioc,
>  		dprint_event_th(mrioc,
>  		    "scheduling bottom half handler for event(0x%02x) - (0x%08x), ack_required=%d\n",
>  		    evt_type, le32_to_cpu(event_reply->event_context), ack_req);
> -		sz = event_reply->event_data_length * 4;
> +		if (mrioc->reply_sz > offsetof(struct mpi3_event_notification_reply, event_data))
> +			avail_reply_room = mrioc->reply_sz -
> +			    offsetof(struct mpi3_event_notification_reply, event_data);
> +		else
> +			avail_reply_room = 0;
> +		sz = min_t(u16, event_reply->event_data_length * 4, avail_reply_room);

[Severity: High]
Since this patch restricts the allocated size of the heap buffer fwevt by
clamping it to avail_reply_room, could this cause an out-of-bounds read in
the bottom-half handler?

For instance, in mpi3mr_sastopochg_evt_bh(), the loop iterates up to the
unclamped num_entries from the payload. If the allocation was truncated,
iterating up to the original unvalidated num_entries could read past the
end of the newly clamped fwevt heap allocation.

[Severity: High]
This is a pre-existing issue, but could truncating the event data size
cause out-of-bounds slab reads later if the buffer size is exceptionally
small?

If the clamped sz ends up being extremely small or zero, the bottom-half
handlers like mpi3mr_fwevt_bh() blindly cast fwevt->event_data to
fixed-size structures such as struct mpi3_device_page0 and access fields
like persistent_id. Should there be a minimum size check before accessing
these fields in the bottom half?

>  		fwevt = mpi3mr_alloc_fwevt(sz);
>  		if (!fwevt) {
>  			dprint_event_th(mrioc,

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