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

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

New issues:
- [High] The newly introduced `mpi3mr_sas_phy_bit` function uses `WARN_ON_ONCE` to validate firmware-provided PHY identifiers, which creates a kernel panic vector if `panic_on_warn` is enabled.

Pre-existing issues:
- [High] Incorrect casting of `u64` bitmask to `unsigned long *` for `for_each_set_bit()` causes incorrect PHY IDs on 32-bit big-endian architectures, leading to array out-of-bounds access.
- [High] A malicious or compromised firmware can trigger a heap buffer overflow in `mpi3mr_refresh_sas_ports()` by providing a `num_phys` value greater than 64.
--

--- Patch [1]: [PATCH] scsi: mpi3mr: make SAS port PHY masks 64-bit safe ---
commit ceebecd676e904ef112074dffb72b70397933c87
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/. It introduces helpers to avoid
undefined behavior when constructing 64-bit PHY masks using signed 32-bit shift
operations.

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

[Severity: High]
Can this allow malicious or buggy firmware to trigger a kernel panic?

The phy_id originates from the device firmware. If a compromised or buggy
device provides a num_phys >= 64, the driver iterates through these and passes
them to mpi3mr_sas_phy_bit(), triggering WARN_ON_ONCE().

Kernel guidelines prohibit using WARN_ON macros to validate external or
hardware-provided data because it allows an external entity to crash the
system via a Denial of Service if panic_on_warn is enabled.

[ ... ]
> @@ -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;
>  		host_port_count++;

[Severity: High]
This isn't a bug introduced by this patch, but can a malicious or compromised
firmware trigger a heap buffer overflow here by providing a num_phys value
greater than 64?

Earlier in mpi3mr_refresh_sas_ports(), h_port is allocated with a hardcoded
size of 64 elements on the heap:

    h_port = kzalloc_objs(struct host_port, 64);

The loop that populates it iterates up to mrioc->sas_hba.num_phys, which is
parsed directly from the firmware's SAS IO Unit Page 0 (as an 8-bit value,
allowing up to 255).

If the firmware reports more than 64 phys and provides distinct attached
handles for them, host_port_count (tracked via port_idx) will exceed 63.
This directly results in unconstrained out-of-bounds heap writes when
initializing fields like h_port[port_idx].sas_address.

[Severity: High]
This is a pre-existing issue, but does the incorrect casting in
mpi3mr_update_mr_sas_port() cause array out-of-bounds access on 32-bit
big-endian architectures?

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)) {
        mr_sas_phy = &mrioc->sas_hba.phy[i];
    ...
}

In mpi3mr_update_mr_sas_port(), phys_to_be_added is a u64 bitmask. On 32-bit
big-endian systems, unsigned long is 32 bits, and the cast points to the
most significant 32 bits (bits 32-63) of the u64.

for_each_set_bit() processes this as bits 0-31, effectively swapping the
lower and upper halves of the 64-bit mask. If a PHY ID < 32 is set, it
will be interpreted as PHY ID >= 32, which causes out-of-bounds access
to dynamically allocated arrays like mrioc->sas_hba.phy.

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