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_O=
NCE` 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_e=
ach_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 over=
flow in `mpi3mr_refresh_sas_ports()` by providing a `num_phys` value greate=
r 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
   =20
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 s=
hift
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 @@
> =20
>  #include "mpi3mr.h"
> =20
> +/**
> + * 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 t=
he
> + * shift to avoid undefined behavior.
> + *
> + * Return: BIT_ULL(@phy_id) for a representable identifier, otherwise ze=
ro.
> + */
> +static u64 mpi3mr_sas_phy_bit(u8 phy_id)
> +{
> +	if (WARN_ON_ONCE(phy_id >=3D 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 >=3D 64, the driver iterates through these and p=
asses
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 =3D host_port_count;
>  		h_port[port_idx].sas_address =3D le64_to_cpu(sasinf->sas_address);
>  		h_port[port_idx].handle =3D attached_handle;
> -		h_port[port_idx].phy_mask =3D (1 << i);
> +		h_port[port_idx].phy_mask =3D mpi3mr_sas_phy_bit(i);
>  		h_port[port_idx].iounit_port_id =3D sas_io_unit_pg0->phy_data[i].io_un=
it_port;
>  		h_port[port_idx].lowest_phy =3D sasinf->phy_num;
>  		h_port[port_idx].used =3D 0;
>  		host_port_count++;

[Severity: High]
This isn't a bug introduced by this patch, but can a malicious or compromis=
ed
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 =3D 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 =3D &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 >=3D 32, which causes out-of-bounds access
to dynamically allocated arrays like mrioc->sas_hba.phy.

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260804142831.4365=
[email protected]?part=3D1