Re: [PATCH] scsi: mpi3mr: make SAS port PHY masks 64-bit safe

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Incorrect bit iteration over a `u64` variable using `for_each_set_bit` in `mpi3mr_update_mr_sas_port`.
- [High] Out-of-bounds heap write in `mpi3mr_refresh_sas_ports` when a controller firmware reports more than 64 unique PHY connections.
--

--- Patch [1]: [PATCH] scsi: mpi3mr: make SAS port PHY masks 64-bit safe ---
commit f7b3b6c83f03cd4dace1e13e5700d80724e506b0
Author: Ilya Khomyakov <[email protected]>

    scsi: mpi3mr: make SAS port PHY masks 64-bit safe
    
    This patch fixes 64-bit PHY-mask handling in the Broadcom MPI3 Storage
    Controller driver under drivers/scsi/mpi3mr/.

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_transport.c b/drivers/scsi/mpi3mr/mpi3mr_transport.c
> index 240f67a8e2e3b..d6492dddf7352 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_transport.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_transport.c

[ ... ]

> @@ -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);

[Severity: High]
This isn't a bug introduced by this patch, but can the h_port array overflow
when iterating over the PHYs in mpi3mr_refresh_sas_ports()?

The array is allocated earlier in the function with a fixed size of 64:

	h_port = kzalloc_objs(struct host_port, 64);

Since the loop iterates up to mrioc->sas_hba.num_phys times (which is an
8-bit integer allowing up to 255), and host_port_count is incremented for
each unique attached handle without bounds checking, will this write out of
bounds if the firmware reports more than 64 unique PHY connections?

[Severity: High]
This is also a pre-existing issue, but since we are making PHY masks 64-bit
safe, is the bit iteration over the u64 mask in mpi3mr_update_mr_sas_port()
working correctly on 32-bit big-endian architectures?

mpi3mr_update_mr_sas_port() uses for_each_set_bit with a cast to ulong *:

	u64 phys_to_be_added, phys_to_be_removed;
	...
	for_each_set_bit(i, (ulong *) &phys_to_be_added, BITS_PER_TYPE(u64)) {
		mr_sas_phy = &mrioc->sas_hba.phy[i];

By casting a local u64 pointer to unsigned long * on 32-bit big-endian
systems, won't the bit halves be swapped? This could result in out-of-bounds
array accesses on mrioc->sas_hba.phy[i] and incorrect topology changes since
bits 0-31 become 32-63 and vice versa.

>  		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;

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