Re: [PATCH v2 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] Double-put on `tgtdev` causing a reference count underflow and potential use-after-free.
- [High] Bitwise shift `(1 << i)` is used to construct a `u64 phy_mask`, causing sign-extension and out-of-bounds array access for i >= 31.
- [High] Casting a `u64` bitmask to `(ulong *)` for `for_each_set_bit()` swaps bit mask halves on 32-bit big-endian architectures.
--

commit 0f7fb7e1d3f3dab2157cb8912f1ddaf0e9989416
Author: Chandrakanth Patil <[email protected]>

mpi3mr: Fix out-of-bounds phy array access on link change

This commit adds a bounds check in mpi3mr_update_links() to ensure the
phy_number from a link change event does not exceed the number of allocated
phys for the node. This prevents an out-of-bounds array access on the
node's phy array.

> 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
[ ... ]

[Severity: High]
This is a pre-existing issue, but looking at mpi3mr_sastopochg_evt_bh()
in drivers/scsi/mpi3mr/mpi3mr_os.c, when processing events with a target
not responding reason code:

drivers/scsi/mpi3mr/mpi3mr_os.c:mpi3mr_sastopochg_evt_bh() {
    ...
            mpi3mr_tgtdev_put(tgtdev);
            break;
    ...
        if (tgtdev)
            mpi3mr_tgtdev_put(tgtdev);
}

Does this result in a double decrement of the tgtdev reference count,
causing premature freeing of the target device structure? Similar logic
appears to exist in mpi3mr_pcietopochg_evt_bh() and
mpi3mr_devinfochg_evt_bh().

[Severity: High]
This is a pre-existing issue, but looking at how the phy mask is updated
in mpi3mr_sas_port_add() in drivers/scsi/mpi3mr/mpi3mr_transport.c:

drivers/scsi/mpi3mr/mpi3mr_transport.c:mpi3mr_sas_port_add() {
    ...
            mr_sas_port->phy_mask |= (1 << i);
    ...
}

Since 1 is a signed 32-bit integer, evaluating it up to num_phys (which can
be 32 or greater on enterprise HBAs) could produce a negative value that
sign-extends to 0xFFFFFFFF80000000ULL when implicitly cast to the u64
phy_mask.

Can this cause out-of-bounds array accesses on the phy arrays?

Another instance with the identical signed shift occurs in
mpi3mr_refresh_sas_ports():

drivers/scsi/mpi3mr/mpi3mr_transport.c:mpi3mr_refresh_sas_ports() {
    ...
        h_port[port_idx].phy_mask = (1 << i);
    ...
}

[Severity: High]
This is a pre-existing issue, but looking at how phys_to_be_added is
processed in mpi3mr_update_mr_sas_port():

drivers/scsi/mpi3mr/mpi3mr_transport.c:mpi3mr_update_mr_sas_port() {
    ...
    for_each_set_bit(i, (ulong *) &phys_to_be_added, BITS_PER_TYPE(u64)) {
    ...
}

The variable phys_to_be_added is allocated as a 64-bit u64. On 32-bit
big-endian platforms, unsigned long is 4 bytes.

Is it possible that casting the u64 bitmask to (ulong *) for
for_each_set_bit() causes it to process the most significant 32 bits first,
treating bits 32-63 as bits 0-31 and vice versa, leading to incorrect port
mappings?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=12