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

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

Thanks for the review.

New issues:
- None. This patch specifically bounds the num_entries iteration count
in SAS topology change events against the received event data payload
length.

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 PCIe topology change event bounds check is addressed in "mpi3mr:
Fix out-of-bounds read in PCIe topology change events" in this same
series.
- The top-half early return behavior and bottom-half event handling
structure are pre-existing in the driver design and will be addressed
in a
  separate follow-up patch series.