Linux SCSI subsystem development
 help / color / mirror / Atom feed
* [PATCH] scsi: mpi3mr: make SAS port PHY masks 64-bit safe
@ 2026-08-04 14:00 Ilya Khomyakov
  2026-08-04 14:39 ` sashiko-bot
  0 siblings, 1 reply; 4+ messages in thread
From: Ilya Khomyakov @ 2026-08-04 14:00 UTC (permalink / raw)
  To: linux-scsi
  Cc: Sathya Prakash Veerichetty, Kashyap Desai, Sumit Saxena,
	Sreekanth Reddy, mpi3mr-linuxdrv.pdl, Martin K . Petersen,
	James E . J . Bottomley, Ilya Khomyakov

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 <khomyakovilya@gmail.com>

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

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* [PATCH] scsi: mpi3mr: make SAS port PHY masks 64-bit safe
@ 2026-08-04 14:28 Ilya Khomyakov
  2026-08-04 15:29 ` sashiko-bot
  0 siblings, 1 reply; 4+ messages in thread
From: Ilya Khomyakov @ 2026-08-04 14:28 UTC (permalink / raw)
  To: Martin K . Petersen
  Cc: James E . J . Bottomley, Sathya Prakash Veerichetty,
	Kashyap Desai, Sumit Saxena, Sreekanth Reddy, mpi3mr-linuxdrv.pdl,
	linux-scsi, linux-kernel, Ilya Khomyakov

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 <khomyakovilya@gmail.com>

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

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH] scsi: mpi3mr: make SAS port PHY masks 64-bit safe
  2026-08-04 14:00 Ilya Khomyakov
@ 2026-08-04 14:39 ` sashiko-bot
  0 siblings, 0 replies; 4+ messages in thread
From: sashiko-bot @ 2026-08-04 14:39 UTC (permalink / raw)
  To: Ilya Khomyakov; +Cc: linux-scsi

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 <khomyakovilya@gmail.com>

    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/20260804140004.4004-1-khomyakovilya@gmail.com?part=1

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] scsi: mpi3mr: make SAS port PHY masks 64-bit safe
  2026-08-04 14:28 [PATCH] scsi: mpi3mr: make SAS port PHY masks 64-bit safe Ilya Khomyakov
@ 2026-08-04 15:29 ` sashiko-bot
  0 siblings, 0 replies; 4+ messages in thread
From: sashiko-bot @ 2026-08-04 15:29 UTC (permalink / raw)
  To: Ilya Khomyakov; +Cc: linux-scsi

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 <khomyakovilya@gmail.com>

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/20260804142831.4365-1-khomyakovilya@gmail.com?part=1

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2026-08-04 15:29 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-08-04 14:28 [PATCH] scsi: mpi3mr: make SAS port PHY masks 64-bit safe Ilya Khomyakov
2026-08-04 15:29 ` sashiko-bot
  -- strict thread matches above, loose matches on Subject: below --
2026-08-04 14:00 Ilya Khomyakov
2026-08-04 14:39 ` sashiko-bot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox