[PATCH] scsi: mpi3mr: make SAS port PHY masks 64-bit safe
Ilya Khomyakov <[email protected]> Tue, 4 Aug 2026 17:28:31 +0300
| Newsgroups | gmane.linux.scsi,gmane.linux.kernel |
|---|---|
| Message-ID | <[email protected]> |
This patch fixes 64-bit PHY-mask handling in the Broadcom MPI3 Storage Controller driver under drivers/scsi/mpi3mr/. struct mpi3mr_sas_port stores phy_mask as u64, but several paths construct the mask with the signed-int expression 1 << phy_id or 1 << i. The shift is evaluated as int before the result is converted to u64. The operation therefore has undefined behavior when the PHY identifier reaches the sign bit or width of int. The same code uses ffs() to find the lowest set bit, but ffs() accepts int and truncates bits 32 through 63. The issue was reproduced with UBSAN during SAS port creation: UBSAN: shift-out-of-bounds in mpi3mr_transport.c Workqueue: mpi3mr0_fwevt_wrkr mpi3mr_fwevt_worker mpi3mr_sas_port_add mpi3mr_update_links mpi3mr_report_tgtdev_to_sas_transport The reproduced topology contains a controller host node with 39 PHYs and an expander with 46 PHYs. Such a topology is sufficient to exercise PHY identifiers above 31 during normal discovery. Add a helper that validates the firmware PHY identifier and constructs the mask bit with BIT_ULL(). Add a separate helper that handles an empty mask and otherwise finds the lowest bit with __ffs64(). Use the helpers in the PHY add and remove paths, initial port construction, and reset-refresh port grouping. Also initialize lowest_phy when the first PHY is dynamically added to an empty port. The patch was tested in an out-of-tree mpi3mr 8.17.1.0.0 build. The driver successfully discovered a 39-PHY host node and a 46-PHY expander, created expander PHY objects through PHY 45, and completed device discovery without a shift-out-of-bounds or other UBSAN report. The boot test directly exercised initial high-PHY port construction. The same helpers are used in the add, remove, and reset-refresh paths to remove the identical 32-bit operations from those paths as well. Signed-off-by: Ilya Khomyakov <[email protected]> --- drivers/scsi/mpi3mr/mpi3mr_transport.c | 58 ++++++++++++++++++++++---- 1 file changed, 49 insertions(+), 9 deletions(-) diff --git a/drivers/scsi/mpi3mr/mpi3mr_transport.c b/drivers/scsi/mpi3mr/mpi3mr_transport.c index 240f67a..d6492dd 100644 --- a/drivers/scsi/mpi3mr/mpi3mr_transport.c +++ b/drivers/scsi/mpi3mr/mpi3mr_transport.c @@ -11,6 +11,42 @@ #include "mpi3mr.h" +/** + * mpi3mr_sas_phy_bit - build a bit for a firmware PHY identifier + * @phy_id: Firmware PHY identifier to represent in a 64-bit port mask + * + * The port mask is a u64, so every shift must be performed in a 64-bit + * unsigned type. Reject identifiers that cannot be represented before the + * shift to avoid undefined behavior. + * + * Return: BIT_ULL(@phy_id) for a representable identifier, otherwise zero. + */ +static u64 mpi3mr_sas_phy_bit(u8 phy_id) +{ + if (WARN_ON_ONCE(phy_id >= sizeof(u64) * 8)) + return 0; + + return BIT_ULL(phy_id); +} + +/** + * mpi3mr_sas_port_lowest_phy - find the lowest PHY in a port mask + * @phy_mask: 64-bit bitmap of PHY identifiers assigned to the port + * + * Use a 64-bit find-first-set operation so PHY identifiers 32 through 63 + * are not truncated to int. Keep -1 as the empty-mask sentinel used by the + * surrounding port bookkeeping. + * + * Return: lowest set PHY identifier, or -1 when the mask is empty. + */ +static int mpi3mr_sas_port_lowest_phy(u64 phy_mask) +{ + if (!phy_mask) + return -1; + + return __ffs64(phy_mask); +} + /** * mpi3mr_post_transport_req - Issue transport requests and wait * @mrioc: Adapter instance reference @@ -610,10 +646,11 @@ static void mpi3mr_delete_sas_phy(struct mpi3mr_ioc *mrioc, mr_sas_port->num_phys--; if (host_node) { - mr_sas_port->phy_mask &= ~(1 << mr_sas_phy->phy_id); + mr_sas_port->phy_mask &= ~mpi3mr_sas_phy_bit(mr_sas_phy->phy_id); if (mr_sas_port->lowest_phy == mr_sas_phy->phy_id) - mr_sas_port->lowest_phy = ffs(mr_sas_port->phy_mask) - 1; + mr_sas_port->lowest_phy = + mpi3mr_sas_port_lowest_phy(mr_sas_port->phy_mask); } sas_port_delete_phy(mr_sas_port->port, mr_sas_phy->phy); mr_sas_phy->phy_belongs_to_port = 0; @@ -641,10 +678,12 @@ static void mpi3mr_add_sas_phy(struct mpi3mr_ioc *mrioc, list_add_tail(&mr_sas_phy->port_siblings, &mr_sas_port->phy_list); mr_sas_port->num_phys++; if (host_node) { - mr_sas_port->phy_mask |= (1 << mr_sas_phy->phy_id); + mr_sas_port->phy_mask |= mpi3mr_sas_phy_bit(mr_sas_phy->phy_id); - if (mr_sas_phy->phy_id < mr_sas_port->lowest_phy) - mr_sas_port->lowest_phy = ffs(mr_sas_port->phy_mask) - 1; + if (mr_sas_port->lowest_phy < 0 || + mr_sas_phy->phy_id < mr_sas_port->lowest_phy) + mr_sas_port->lowest_phy = + mpi3mr_sas_port_lowest_phy(mr_sas_port->phy_mask); } sas_port_add_phy(mr_sas_port->port, mr_sas_phy->phy); mr_sas_phy->phy_belongs_to_port = 1; @@ -1396,7 +1435,7 @@ static struct mpi3mr_sas_port *mpi3mr_sas_port_add(struct mpi3mr_ioc *mrioc, &mr_sas_port->phy_list); mr_sas_port->num_phys++; if (mr_sas_node->host_node) - mr_sas_port->phy_mask |= (1 << i); + mr_sas_port->phy_mask |= mpi3mr_sas_phy_bit(i); } if (!mr_sas_port->num_phys) { @@ -1406,7 +1445,8 @@ static struct mpi3mr_sas_port *mpi3mr_sas_port_add(struct mpi3mr_ioc *mrioc, } if (mr_sas_node->host_node) - mr_sas_port->lowest_phy = ffs(mr_sas_port->phy_mask) - 1; + mr_sas_port->lowest_phy = + mpi3mr_sas_port_lowest_phy(mr_sas_port->phy_mask); if (mr_sas_port->remote_identify.device_type == SAS_END_DEVICE) { tgtdev = mpi3mr_get_tgtdev_by_addr(mrioc, @@ -1738,7 +1778,7 @@ mpi3mr_refresh_sas_ports(struct mpi3mr_ioc *mrioc) found = 0; for (j = 0; j < host_port_count; j++) { if (h_port[j].handle == attached_handle) { - h_port[j].phy_mask |= (1 << i); + h_port[j].phy_mask |= mpi3mr_sas_phy_bit(i); found = 1; break; } @@ -1765,7 +1805,7 @@ mpi3mr_refresh_sas_ports(struct mpi3mr_ioc *mrioc) port_idx = host_port_count; h_port[port_idx].sas_address = le64_to_cpu(sasinf->sas_address); h_port[port_idx].handle = attached_handle; - h_port[port_idx].phy_mask = (1 << i); + h_port[port_idx].phy_mask = mpi3mr_sas_phy_bit(i); h_port[port_idx].iounit_port_id = sas_io_unit_pg0->phy_data[i].io_unit_port; h_port[port_idx].lowest_phy = sasinf->phy_num; h_port[port_idx].used = 0;