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

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