Re: [PATCH 12/17] mpi3mr: Fix out-of-bounds phy array access on link change

On Wed, Aug 19, 2026 at 12:03 AM <[email protected]> wrote:
>
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] The firmware event handlers for SAS and PCIe topology changes iterate up to `num_entries` without ensuring it is strictly bounded by the event data length, leading to out-of-bounds reads.
> - [High] This problem wasn't introduced by this patch, but the entirety of `mpi3mr_update_links` continues to access `mr_sas_node` and performs lockless list traversals on its structures after releasing `mrioc->sas_node_lock` on the happy path.
> - [High] This problem wasn't introduced by this patch, but a shift-sign-extension bug exists when updating 64-bit `phy_mask` bitmasks using `1 << i`, leading to out-of-bounds array accesses when `i >= 31`.
> --
>
> --- Patch [12]: [PATCH 12/17] mpi3mr: Fix out-of-bounds phy array access on link change ---
> commit 5a98eebaf3b1e059c60e29aa1529199221440703
> Author: Chandrakanth Patil <[email protected]>
>
> mpi3mr: Fix out-of-bounds phy array access on link change
>
> This commit adds a bounds check to ensure the phy number associated with a
> link change does not exceed the allocated number of phys for the node. If it
> does, the event is logged and ignored.
>
> > diff --git a/drivers/scsi/mpi3mr/mpi3mr_transport.c b/drivers/scsi/mpi3mr/mpi3mr_transport.c
> > index 240f67a8e2e3b..2afff1a002ed9 100644
> > --- a/drivers/scsi/mpi3mr/mpi3mr_transport.c
> > +++ b/drivers/scsi/mpi3mr/mpi3mr_transport.c
> > @@ -1090,6 +1090,13 @@ void mpi3mr_update_links(struct mpi3mr_ioc *mrioc,
> >               return;
> >       }
> >
> > +     if (phy_number >= mr_sas_node->num_phys) {
> > +             spin_unlock_irqrestore(&mrioc->sas_node_lock, flags);
> > +             ioc_err(mrioc, "%s: phy_number(%d) >= num_phys(%d)\n",
> > +                     __func__, phy_number, mr_sas_node->num_phys);
> > +             return;
> > +     }
> > +
> >       mr_sas_phy = &mr_sas_node->phy[phy_number];
> >       mr_sas_phy->attached_handle = handle;
> >       spin_unlock_irqrestore(&mrioc->sas_node_lock, flags);
>
> [Severity: High]
> This is a pre-existing issue, but does dropping the sas_node_lock here leave
> mr_sas_node vulnerable to concurrent modification or use-after-free?
>
> Looking at the remainder of mpi3mr_update_links(), mr_sas_node and its members
> continue to be used after the lock is released:
>
>     spin_unlock_irqrestore(&mrioc->sas_node_lock, flags);
>     if (handle && (link_rate >= MPI3_SAS_NEG_LINK_RATE_1_5)) {
>         mpi3mr_set_identify(mrioc, handle, &mr_sas_phy->remote_identify);
>         mpi3mr_add_phy_to_an_existing_port(mrioc, mr_sas_node, mr_sas_phy, ...);
>
> When mpi3mr_add_phy_to_an_existing_port() is called, it iterates over the
> sas_port_list locklessly:
>
>     list_for_each_entry(mr_sas_port, &mr_sas_node->sas_port_list, port_list) {
>
> Since mrioc->sas_node_lock is no longer held, can other events concurrently add
> or remove ports, corrupting the traversal? Additionally, mr_sas_node is fetched
> without acquiring a reference, potentially allowing it to be concurrently freed.
>
> [Severity: High]
> This isn't a bug introduced by this patch, but could the firmware event
> handlers for SAS and PCIe topology changes iterate up to num_entries without
> bounds checking against the event data length?
>
> In mpi3mr_sastopochg_evt_bh() and mpi3mr_pcietopochg_evt_bh() in mpi3mr_os.c,
> the loop blindly iterates up to num_entries:
>
>     for (i = 0; i < event_data->num_entries; i++) {
>         handle = le16_to_cpu(event_data->phy_entry[i].attached_dev_handle);
>
> If the hardware or an attacker controlling the controller issues an event with
> a num_entries field larger than what fits in the reply buffer, could this
> lead to an out-of-bounds read? Even if the top-half handler attempts to bound
> num_entries, returning early without dropping the event still allows the
> bottom-half to iterate over the truncated buffer.
>
> Additionally, mpi3mr_pcietopochg_evt_th() appears to completely lack any
> bounds checking before iterating over topo_evt->port_entry.
>
> [Severity: High]
> This isn't a bug introduced by this patch, but is there a sign-extension
> regression when updating the 64-bit phy_mask bitmask in
> mpi3mr_add_phy_to_an_existing_port()?
>
>     mr_sas_port->phy_mask |= (1 << i);
>
> When i evaluates to 31, the expression 1 << 31 evaluates as a negative 32-bit
> integer (0x80000000). When this is bitwise OR'd into the 64-bit phy_mask, it
> sign-extends to 0xFFFFFFFF80000000ULL, erroneously setting bits 31 through 63.
>
> Later, in mpi3mr_update_mr_sas_port(), this corrupted mask seems to be used
> in a loop:
>
>     for_each_set_bit(i, (ulong *) &phys_to_be_added, BITS_PER_TYPE(u64)) {
>         mr_sas_phy = &mrioc->sas_hba.phy[i];
>
> If phys_to_be_added inherits the sign-extended mask, won't this loop exceed
> the allocated bounds of mrioc->sas_hba.phy (which may only have 32 elements)
> and corrupt memory?
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=12

Thanks for the review.

New issues:
- None. This patch specifically prevents an out-of-bounds array access
by checking phy_number against mr_sas_node->num_phys before indexing
mr_sas_node->phy[].

Pre-existing issues:
- The pre-existing issues noted (lock scoping in
mpi3mr_update_links(), bottom-half topology change event iterations,
and the 64-bit phy_mask shift sign-extension)
  are pre-existing in the driver transport layer and will be addressed
in a separate follow-up patch series.