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

On Tue, Aug 18, 2026 at 11:46 PM <[email protected]> wrote:
>
> 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

Thanks for the review.

New issues:
- The bottom-half event handlers trusting num_entries without
validating against the copied fwevt size is a pre-existing structural
issue in the bottom-half event processing logic.
  This patch specifically fixes the top-half out-of-bounds read by
clamping event data size to avail_reply_room, preventing memcpy() and
mpi3mr_app_save_logdata_th() from reading past
  the end of the reply buffer. Bottom-half payload validation will be
addressed in a separate follow-up patch series.

Pre-existing issues:
- The MPI3_EVENT_DEVICE_INFO_CHANGED fallthrough, top-half DMA bounds
checks, and cleanup race condition in mpi3mr_cleanup_fwevt_list() are
all pre-existing issues in the driver and
  will be addressed in separate follow-up patch series.