Re: [PATCH 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] Bottom-half event handlers perform out-of-bounds heap reads on the `fwevt` buffer because they trust unvalidated loop bounds against the newly clamped allocation size.

Pre-existing issues:
- [High] Top-half event handlers perform out-of-bounds DMA reads on the reply buffer due to missing or flawed bounds checking for variable-length arrays.
- [High] `MPI3_EVENT_DEVICE_INFO_CHANGED` incorrectly falls through to `MPI3_EVENT_LOG_DATA`, silently dropping events and bypassing its required bottom-half processing.
- [High] Accessing a potentially freed work item during event cleanup via `cancel_work_sync`.
--

--- Patch [11]: [PATCH 11/17] mpi3mr: Fix out-of-bounds read of event data ---
commit 4872e2f7b284da8f062de528f26e6b6482b6a169
Author: Chandrakanth Patil <[email protected]>
Subject: mpi3mr: Fix out-of-bounds read of event data

The commit clamps the event data length read from the reply to what the
frame can hold. This prevents out-of-bounds reads when caching log data
and when sizing the buffer allocated for the bottom-half event handlers.

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
> index 1291c60b4d98c..68b13c2ad2244 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_os.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
> @@ -3170,7 +3170,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 does MPI3_EVENT_DEVICE_INFO_CHANGED
intentionally fall through to MPI3_EVENT_LOG_DATA without setting
process_evt_bh = 1?

Because process_evt_bh is not set, the bottom-half handler is completely
bypassed, leaving mpi3mr_devinfochg_evt_bh() as effectively dead code:

mpi3mr_fwevt_bh() {
    ...
    case MPI3_EVENT_DEVICE_INFO_CHANGED:
    {
        dev_pg0 = (struct mpi3_device_page0 *)fwevt->event_data;
        perst_id = le16_to_cpu(dev_pg0->persistent_id);
        if (perst_id != MPI3_DEVICE0_PERSISTENTID_INVALID)
            mpi3mr_devinfochg_evt_bh(mrioc, dev_pg0);
    ...
}

Will this cause the driver to fail to process queue depth changes for target
devices?

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

[ ... ]

> @@ -3204,7 +3209,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);
>  		fwevt = mpi3mr_alloc_fwevt(sz);

[Severity: High]
Does clamping the allocation size here introduce an out-of-bounds heap read in
the bottom-half handlers?

The patch correctly limits the allocation of fwevt to sz, but the payload
copied into it can still contain an unmodified, untrusted num_entries from
the firmware.

When the bottom-half worker executes, won't it iterate up to num_entries,
reading past the end of the strictly clamped fwevt allocation?

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

Can this lead to an out-of-bounds read on the kernel heap?

[Severity: High]
This is a pre-existing issue, but does mpi3mr_pcietopochg_evt_th() also
lack proper bounds checking before looping over num_entries?

It iterates directly based on the untrusted num_entries:

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);
    ...
}

Similarly, in mpi3mr_sastopochg_evt_th(), the bounds check relies on
the unclamped event_data_length instead of mrioc->reply_sz:

mpi3mr_sastopochg_evt_th() {
    avail_len = event_reply->event_data_length * 4;
    ...
    max_entries = (avail_len - offsetof(...)) / sizeof(...);
    if (topo_evt->num_entries > max_entries) return;
}

Will this result in out-of-bounds DMA reads during top-half event processing if
the firmware sends an erroneously large event_data_length?

[Severity: High]
This is a pre-existing issue, but is there a use-after-free risk during
driver cleanup?

In mpi3mr_cleanup_fwevt_list(), current_event is read locklessly and passed to
mpi3mr_cancel_work():

mpi3mr_cleanup_fwevt_list() {
    ...
    if (mrioc->current_event) {
        fwevt = mrioc->current_event;
        ...
        mpi3mr_cancel_work(fwevt);
    ...
}

Concurrently, the worker thread can drop the reference count and free fwevt
before clearing mrioc->current_event:

mpi3mr_fwevt_bh() {
    ...
out:
    mpi3mr_fwevt_put(fwevt);
    mrioc->current_event = NULL;
}

Could mpi3mr_cleanup_fwevt_list() pass a freed fwevt pointer to
cancel_work_sync(&fwevt->work)?

>  		if (!fwevt) {
>  			dprint_event_th(mrioc,

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