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

On Tue, Aug 18, 2026 at 11:53 PM <[email protected]> wrote:
>
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
>
> New issues:
> - [High] The newly added bounds check reads `topo_evt->num_entries` directly from a DMA-mapped buffer, and the subsequent loop condition re-reads it, creating a Time-Of-Check to Time-Of-Use (TOCTOU) race condition.
>
> Pre-existing issues:
> - [High] The bounds check for PCIe and SAS topology change events fails to protect the bottom-half event handlers from out-of-bounds reads because early returns in the top-half do not prevent the malformed event from being queued.
> - [High] The bounds check uses an untrusted `event_data_length` without verifying it against the actual physical dimensions of the hardware reply buffer, allowing an out-of-bounds read in the top-half handler.
> - [High] Missing `dma_rmb()` after polling the phase bit in DMA-mapped reply descriptors, which can lead to processing stale descriptor data on weakly ordered architectures.
> --
>
> --- Patch [14]: [PATCH 14/17] mpi3mr: Fix out-of-bounds read in PCIe topology change events ---
> commit 8553af34c1ff3c0cff0462ca66842979af8cb4d4
> Author: Chandrakanth Patil <[email protected]>
>
> mpi3mr: Fix out-of-bounds read in PCIe topology change events
>
> The number of entries in a PCIe topology change event is used to walk
> the entry array without being compared against the amount of event
> data that was received. Bound the entry count to the received event data
> before use.
>
> > diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
> > --- a/drivers/scsi/mpi3mr/mpi3mr_os.c
> > +++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
> > @@ -2753,11 +2753,26 @@ 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;
> >       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 isn't a bug introduced by this patch, but could this calculation
> exceed the actual physical dimensions of the hardware reply buffer?
>
> Because avail_len is derived strictly from the untrusted event_data_length,
> a compromised or malicious device could provide a value larger than the
> allocated DMA reply buffer (mrioc->reply_sz).
>
> Should we verify event_data_length against mrioc->reply_sz to prevent
> the bounds check from being artificially inflated, which would lead to an
> out-of-bounds read?
>
> The SAS topology handler mpi3mr_sastopochg_evt_th() appears to share this
> exact pattern.
>
> > +     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);
> > +     if (topo_evt->num_entries > max_entries) {
> > +             ioc_err(mrioc, "PCIe topology event: num_entries(%d) exceeds max(%d)\n",
> > +                     topo_evt->num_entries, max_entries);
> > +             return;
> > +     }
>
> [Severity: High]
> This is a pre-existing issue, but does this early return actually prevent
> the malformed event from being processed by the bottom-half handler?
>
> When mpi3mr_pcietopochg_evt_th() returns early here, the process_evt_bh flag
> remains set in mpi3mr_os_handle_events():
>
> drivers/scsi/mpi3mr/mpi3mr_os.c:mpi3mr_os_handle_events() {
>     ...
>         case MPI3_EVENT_PCIE_TOPOLOGY_CHANGE_LIST:
>         {
>                 process_evt_bh = 1;
>                 mpi3mr_pcietopochg_evt_th(mrioc, event_reply);
>                 break;
>         }
>     ...
> }
>
> Since process_evt_bh remains 1, the event is queued to the worker thread.
> The bottom-half handler mpi3mr_pcietopochg_evt_bh() will then blindly
> iterate up to the unvalidated event_data->num_entries, causing an
> out-of-bounds read past the end of the allocated fwevt heap buffer.
>
> Would it be safer to clear process_evt_bh or return a status from the
> top-half handler to prevent queuing malformed events? The SAS topology
> handler mpi3mr_sastopochg_evt_bh() is likely vulnerable to the exact
> same incomplete fix flaw.
>
> >
> >       for (i = 0; i < topo_evt->num_entries; i++) {
> >               handle = le16_to_cpu(topo_evt->port_entry[i].attached_dev_handle);
> >               if (!handle)
>
> [Severity: High]
> Is this loop vulnerable to a Time-Of-Check to Time-Of-Use (TOCTOU) race
> condition?
>
> The newly added bounds check reads topo_evt->num_entries directly from a
> DMA-mapped buffer, and this loop condition continually re-evaluates it.
>
> Because the loop body contains functions that acquire spinlocks
> (e.g., mpi3mr_get_tgtdev_by_handle()), which act as full compiler memory
> barriers, the compiler is forced to re-fetch num_entries from DMA memory
> on every iteration.
>
> If a malicious device alters num_entries in DMA memory after the initial
> max_entries check, could it extend the loop beyond bounds?
>
> Should we cache num_entries in a local variable before the bounds check
> and use the local variable for the loop condition? The SAS topology handler
> mpi3mr_sastopochg_evt_th() appears to have the identical issue.
>
> [Severity: High]
> This isn't a bug introduced by this patch, but is a dma_rmb() barrier needed
> after polling the phase bit in the DMA-mapped reply descriptors?
>
> Looking at mpi3mr_process_op_reply_q() in drivers/scsi/mpi3mr/mpi3mr_fw.c:
>
> drivers/scsi/mpi3mr/mpi3mr_fw.c:mpi3mr_process_op_reply_q() {
>     ...
>         if ((le16_to_cpu(reply_desc->reply_flags) &
>             MPI3_REPLY_DESCRIPT_FLAGS_PHASE_MASK) != exp_phase) {
>                 atomic_dec(&op_reply_q->in_use);
>                 return 0;
>         }
>
>         do {
>                 if (mrioc->unrecoverable || mrioc->io_admin_reset_sync)
>                         break;
>
>                 req_q_idx = le16_to_cpu(reply_desc->request_queue_id) - 1;
>     ...
> }
>
> The driver polls the hardware DMA ring by checking the reply_flags phase mask.
> Without a subsequent dma_rmb(), weakly ordered CPU architectures (such as
> ARM64) can speculatively execute out-of-order reads and fetch the rest of
> the descriptor payload (like request_queue_id) before the hardware has
> actually committed the updated descriptor.
>
> Could this result in processing corrupted or stale descriptor payloads?
> There appears to be a similar polling loop for the admin reply queue in
> mpi3mr_process_admin_reply_q() lacking a dma_rmb() before accessing
> request_queue_ci.
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=14

Thanks for the review.

New issues:
- The TOCTOU observation is valid and will be fixed in the v2
submission. num_entries is now cached in a local variable before the
bounds check and used for the
  loop condition to prevent re-fetching from DMA memory.

Pre-existing issues:
- The event_data_length check against reply_sz is addressed in
"mpi3mr: Fix out-of-bounds read of event data" in this same series.
- The top-half early return behavior and missing dma_rmb() memory
barriers are pre-existing in the driver and will be addressed in a
separate follow-up patch series.