[PATCH v2 00/17] scsi: mpi3mr: Fix out-of-bounds accesses and reference leaks

[PATCH v2 00/17] scsi: mpi3mr: Fix out-of-bounds accesses and reference leaks

[Chandrakanth Patil]

This series contains a set of fixes for the mpi3mr driver:

 - out-of-bounds accesses where values reported by the controller
   (device handles, phy numbers, topology event entry counts, reply and
   sense buffer addresses, event data lengths) are used to index arrays,
   derive pointers or size copies without being checked first

 - out-of-bounds accesses in the BSG passthrough paths, from a request
   size held in too narrow a variable and from a copy made without
   checking the payload holds that much data

 - target device reference leaks and an I/O block counter leak on error
   and teardown paths, the latter leaving a device blocked for I/O

 - a response buffer copied back to user space without being zeroed
   first, so its unwritten fields carry whatever the allocation held

 - a use-after-free and a NULL dereference around the firmware event
   workqueue during driver removal and PCI error recovery

Changes in v2:
 - Patch 6: Switched to do_div() for alignment check to fix 32-bit
   build issue.
 - Patch 7: Switched to do_div() for alignment check to fix 32-bit
   build issue.
 - Patch 8: Relocated handle bounds check to the entry of
   mpi3mr_dev_rmhs_send_tm() so out-of-bounds handles are rejected
   immediately without polluting delayed_rmhs_list.
 - Patch 10: Cached num_entries in a local variable before bounds
   checking to eliminate the TOCTOU re-fetch race from DMA memory.
 - Patch 14: Cached num_entries in a local variable before bounds
   checking to eliminate the TOCTOU re-fetch race from DMA memory.
 - Patch 17: Removed stop_drv_processing and workqueue cleanup from
   pci_channel_io_frozen to prevent I/O breakage (DID_NO_CONNECT) and
   workqueue deadlocks. Added pci_err_recovery check in
   mpi3mr_fwevt_bh() to safely skip event processing.
 - Patches 1-5, 9, 11-13, 15-16: Unchanged from v1.

Chandrakanth Patil (17):
  mpi3mr: Fix buffer overflow in BSG passthrough request copy
  mpi3mr: Fix out-of-bounds read when copying BSG MPI requests
  mpi3mr: Fix I/O block counter leak on admin request post failure
  mpi3mr: Fix target device reference leak in BSG task management
  mpi3mr: Fix buffer overflow when caching log data
  mpi3mr: Fix out-of-bounds reply frame access
  mpi3mr: Fix out-of-bounds sense buffer access
  mpi3mr: Fix out-of-bounds bitmap access during device removal
  mpi3mr: Fix target device reference leak in device removal handshake
  mpi3mr: Fix out-of-bounds read in SAS topology change events
  mpi3mr: Fix out-of-bounds read of event data
  mpi3mr: Fix out-of-bounds phy array access on link change
  mpi3mr: Fix buffer overflow in the BSG target device map
  mpi3mr: Fix out-of-bounds read in PCIe topology change events
  mpi3mr: zero out diagnostic buffer status memory
  mpi3mr: Fix use-after-free of the firmware event workqueue
  mpi3mr: Fix NULL pointer dereference on PCI error recovery

 drivers/scsi/mpi3mr/mpi3mr_app.c       | 43 ++++++++----
 drivers/scsi/mpi3mr/mpi3mr_fw.c        | 17 ++++-
 drivers/scsi/mpi3mr/mpi3mr_os.c        | 95 +++++++++++++++++++++-----
 drivers/scsi/mpi3mr/mpi3mr_transport.c |  7 ++
 4 files changed, 133 insertions(+), 29 deletions(-)

-- 
2.52.0

[PATCH v2 01/17] mpi3mr: Fix buffer overflow in BSG passthrough request copy

[Chandrakanth Patil]

The size of an incoming BSG request is checked using a variable that is
narrower than the field it is read from, so large values wrap and pass
the check. The copy that follows then uses the full value and writes
past the request buffer.

Widen the variable and copy only the amount that was checked.

Fixes: 506bc1a0d6ba ("scsi: mpi3mr: Add support for MPT commands")
Signed-off-by: Chandrakanth Patil <chandrakanth.patil@broadcom.com>
---
v2:
 - No changes from v1.
 drivers/scsi/mpi3mr/mpi3mr_app.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/mpi3mr/mpi3mr_app.c b/drivers/scsi/mpi3mr/mpi3mr_app.c
index 1353a8ff9c85..8e5d24793efd 100644
--- a/drivers/scsi/mpi3mr/mpi3mr_app.c
+++ b/drivers/scsi/mpi3mr/mpi3mr_app.c
@@ -2384,7 +2384,7 @@ static long mpi3mr_bsg_process_mpt_cmds(struct bsg_job *job)
 	long rval = -EINVAL;
 	struct mpi3mr_ioc *mrioc = NULL;
 	u8 *mpi_req = NULL, *sense_buff_k = NULL;
-	u8 mpi_msg_size = 0;
+	u32 mpi_msg_size = 0;
 	struct mpi3mr_bsg_packet *bsg_req = NULL;
 	struct mpi3mr_bsg_mptcmd *karg;
 	struct mpi3mr_buf_entry *buf_entries = NULL;
@@ -2538,7 +2538,7 @@ static long mpi3mr_bsg_process_mpt_cmds(struct bsg_job *job)
 				rval = -EINVAL;
 				goto out;
 			}
-			memcpy(mpi_req, sgl_iter, buf_entries->buf_len);
+			memcpy(mpi_req, sgl_iter, mpi_msg_size);
 			break;
 		default:
 			invalid_be = 1;
-- 
2.52.0

Re: [PATCH v2 01/17] mpi3mr: Fix buffer overflow in BSG passthrough request copy

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing validation of `job->request_len` allows out-of-bounds slab memory reads when parsing the BSG request and buffer entries.
- [High] On 32-bit architectures, pointer wrap-around allows bypassing bounds checks for specific buffer types, leading to out-of-bounds heap access.
- [High] DMA coherent memory allocated for NVMe PRP lists is leaked on command posting failures or command timeouts.
- [High] Potential Use-After-Free of `stgt_priv` if the SCSI target is removed concurrently during a long-running BSG command.
- [High] Data race and use-after-timeout on `mrioc->bsg_cmds` state between timeout handler and interrupt handler.
--

commit 6909218adbd3af92548ad9194e535cec60aaccf1
Author: Chandrakanth Patil <chandrakanth.patil@broadcom.com>

mpi3mr: Fix buffer overflow in BSG passthrough request copy

The size of an incoming BSG request is checked using a variable that is
narrower than the field it is read from, so large values wrap and pass
the check. The copy that follows then uses the full value and writes
past the request buffer.

Widen the variable and copy only the amount that was checked.

Fixes: 506bc1a0d6ba ("scsi: mpi3mr: Add support for MPT commands")
Signed-off-by: Chandrakanth Patil <chandrakanth.patil@broadcom.com>

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_app.c b/drivers/scsi/mpi3mr/mpi3mr_app.c
> index 1353a8ff9c85d..8e5d24793efd0 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_app.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_app.c
> @@ -2384,7 +2384,7 @@ static long mpi3mr_bsg_process_mpt_cmds(struct bsg_job *job)
>  	long rval = -EINVAL;
>  	struct mpi3mr_ioc *mrioc = NULL;
>  	u8 *mpi_req = NULL, *sense_buff_k = NULL;
> -	u8 mpi_msg_size = 0;
> +	u32 mpi_msg_size = 0;
>  	struct mpi3mr_bsg_packet *bsg_req = NULL;
>  	struct mpi3mr_bsg_mptcmd *karg;
>  	struct mpi3mr_buf_entry *buf_entries = NULL;

[Severity: High]
This is a pre-existing issue, but does this lack validation of
job->request_len when parsing the BSG request and buffer entries?

drivers/scsi/mpi3mr/mpi3mr_app.c:mpi3mr_bsg_process_mpt_cmds() {
...
	bsg_req = job->request;
	karg = (struct mpi3mr_bsg_mptcmd *)&bsg_req->cmd.mptcmd;
...
	bufcnt = karg->buf_entry_list.num_of_entries;
...
	buf_entries = karg->buf_entry_list.buf_entry;
...
}

And similarly in mpi3mr_bsg_process_drv_cmds():

drivers/scsi/mpi3mr/mpi3mr_app.c:mpi3mr_bsg_process_drv_cmds() {
...
	bsg_req = job->request;
	drvrcmd = &bsg_req->cmd.drvrcmd;
	mrioc = mpi3mr_bsg_verify_adapter(drvrcmd->mrioc_id);
...
}

If a user intentionally specifies a request_len smaller than the size of
struct mpi3mr_bsg_packet, could this result in an out-of-bounds slab memory
read since the driver casts and accesses the buffer without checking the
length?

> @@ -2538,7 +2538,7 @@ static long mpi3mr_bsg_process_mpt_cmds(struct bsg_job *job)
>  				rval = -EINVAL;
>  				goto out;
>  			}
> -			memcpy(mpi_req, sgl_iter, buf_entries->buf_len);
> +			memcpy(mpi_req, sgl_iter, mpi_msg_size);
>  			break;
>  		default:
>  			invalid_be = 1;

[Severity: High]
This is a pre-existing issue, but does this pointer arithmetic bypass
bounds checks on 32-bit architectures?

drivers/scsi/mpi3mr/mpi3mr_app.c:mpi3mr_bsg_process_mpt_cmds() {
...
		case MPI3MR_BSG_BUFTYPE_MPI_REPLY:
			sgl_iter = sgl_din_iter;
			sgl_din_iter += buf_entries->buf_len;
...
}

If a user submits a massive buf_len (e.g., 0xFFFFFFFF), could the addition
to sgl_din_iter (which is a u8 *) cause an integer wrap-around on 32-bit
systems?

Since a wrapped pointer is mathematically smaller, would it bypass the
safety check:

	if (sgl_din_iter > (din_buf + job->reply_payload.payload_len))

And allow an out-of-bounds heap write when the driver copies the reply:

	memcpy(drv_buf_iter->bsg_buf, bsg_reply_buf, tmplen);

[Severity: High]
This is a pre-existing issue, but does this code leak DMA coherent memory
on command posting failures or timeouts?

drivers/scsi/mpi3mr/mpi3mr_app.c:mpi3mr_build_nvme_prp() {
...
	mrioc->prp_list_virt = dma_alloc_coherent(&mrioc->pdev->dev,
	    dev_pgsz, &mrioc->prp_list_dma, GFP_KERNEL);
...
}

drivers/scsi/mpi3mr/mpi3mr_app.c:mpi3mr_bsg_process_mpt_cmds() {
...
		if (rval) {
			mrioc->bsg_cmds.is_waiting = 0;
			dprint_bsg_err(mrioc,
			    "%s: posting bsg request is failed\n", __func__);
			rval = -EAGAIN;
			if (block_io && stgt_priv)
				atomic_dec(&stgt_priv->block_io);
			goto out_unlock;
		}
...
}

If mpi3mr_admin_request_post() fails or if the command times out, the code
jumps to out_unlock. Does out_unlock fail to free mrioc->prp_list_virt,
leading to a permanent leak of DMA memory?

[Severity: High]
This is a pre-existing issue, but is there a potential Use-After-Free of
stgt_priv if the SCSI target is removed concurrently?

drivers/scsi/mpi3mr/mpi3mr_app.c:mpi3mr_bsg_process_mpt_cmds() {
...
		if (tgtdev) {
			if (tgtdev->starget && tgtdev->starget->hostdata) {
				stgt_priv = (struct mpi3mr_stgt_priv_data *)
				    tgtdev->starget->hostdata;
				atomic_inc(&stgt_priv->block_io);
			}
			mpi3mr_tgtdev_put(tgtdev);
		}
...
		if (block_io && stgt_priv)
			atomic_dec(&stgt_priv->block_io);
...
}

The reference to the target device is dropped via mpi3mr_tgtdev_put(tgtdev)
before the long wait. If the physical SCSI device is hot-unplugged during
the wait for command completion, could the SCSI midlayer free the starget
and stgt_priv before atomic_dec() is called?

[Severity: High]
This is a pre-existing issue, but is there a data race on mrioc->bsg_cmds
state between the timeout handler and the interrupt handler?

drivers/scsi/mpi3mr/mpi3mr_app.c:mpi3mr_bsg_process_mpt_cmds() {
...
		if (!(mrioc->bsg_cmds.state & MPI3MR_CMD_COMPLETE)) {
			mrioc->bsg_cmds.is_waiting = 0;
			rval = -EAGAIN;
...
}

And in the interrupt handler:

drivers/scsi/mpi3mr/mpi3mr_fw.c:mpi3mr_process_admin_reply_desc() {
...
	cmdptr->state |= MPI3MR_CMD_COMPLETE;
...
	cmdptr->state &= ~MPI3MR_CMD_PENDING;
...
	if (cmdptr->is_waiting) {
		cmdptr->is_waiting = 0;
		complete(&cmdptr->done);
	}
...
}

If a BSG command times out, the timeout handler locklessly sets
is_waiting = 0 and aborts, allowing another thread to acquire the lock
and submit a new command using the same structure.

If a delayed hardware interrupt for the original command fires while the
new command is pending, could the non-atomic modifications to cmdptr->state
in mpi3mr_process_admin_reply_desc corrupt the state of the new command
and prematurely complete it?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825210411.301535-1-chandrakanth.patil@broadcom.com?part=1

[PATCH v2 02/17] mpi3mr: Fix out-of-bounds read when copying BSG MPI requests

[Chandrakanth Patil]

The MPI request is copied out of the caller supplied payload without
first checking that the requested amount is actually present, so the
copy can read past the end of the payload buffer.

Check the range before copying.

Fixes: 506bc1a0d6ba ("scsi: mpi3mr: Add support for MPT commands")
Signed-off-by: Chandrakanth Patil <chandrakanth.patil@broadcom.com>
---
v2:
 - No changes from v1.
 drivers/scsi/mpi3mr/mpi3mr_app.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/scsi/mpi3mr/mpi3mr_app.c b/drivers/scsi/mpi3mr/mpi3mr_app.c
index 8e5d24793efd..1cd1c38dcd56 100644
--- a/drivers/scsi/mpi3mr/mpi3mr_app.c
+++ b/drivers/scsi/mpi3mr/mpi3mr_app.c
@@ -2538,6 +2538,14 @@ static long mpi3mr_bsg_process_mpt_cmds(struct bsg_job *job)
 				rval = -EINVAL;
 				goto out;
 			}
+			if (sgl_iter + mpi_msg_size >
+			    dout_buf + job->request_payload.payload_len) {
+				dprint_bsg_err(mrioc, "%s: MPI request buf exceeds dout_buf\n",
+					       __func__);
+				mutex_unlock(&mrioc->bsg_cmds.mutex);
+				rval = -EINVAL;
+				goto out;
+			}
 			memcpy(mpi_req, sgl_iter, mpi_msg_size);
 			break;
 		default:
-- 
2.52.0

Re: [PATCH v2 02/17] mpi3mr: Fix out-of-bounds read when copying BSG MPI requests

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing bounds check on BSG request length enables out-of-bounds heap reads.
- [High] Missing endianness conversions on MPI structures cause corruption on big-endian systems.
--

commit ab35632c4adcdd391fb038b2fa77fa3b546f692a
Author: Chandrakanth Patil <chandrakanth.patil@broadcom.com>

mpi3mr: Fix out-of-bounds read when copying BSG MPI requests

The MPI request is copied out of the caller supplied payload without
first checking that the requested amount is actually present, so the
copy can read past the end of the payload buffer. Check the range
before copying.

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_app.c b/drivers/scsi/mpi3mr/mpi3mr_app.c
> index 8e5d24793efd0..1cd1c38dcd565 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_app.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_app.c
> @@ -2538,6 +2538,14 @@ static long mpi3mr_bsg_process_mpt_cmds(struct bsg_job *job)
>  				rval = -EINVAL;
>  				goto out;
>  			}
> +			if (sgl_iter + mpi_msg_size >
> +			    dout_buf + job->request_payload.payload_len) {
> +				dprint_bsg_err(mrioc, "%s: MPI request buf exceeds dout_buf\n",
> +					       __func__);
> +				mutex_unlock(&mrioc->bsg_cmds.mutex);
> +				rval = -EINVAL;
> +				goto out;
> +			}

[Severity: High]
This is a pre-existing issue, but since this patch fixes bounds checks in
this function, is the initial loop bounded correctly against the user
allocation?

drivers/scsi/mpi3mr/mpi3mr_app.c:mpi3mr_bsg_process_mpt_cmds() {
    ...
    bufcnt = karg->buf_entry_list.num_of_entries;
    ...
    buf_entries = karg->buf_entry_list.buf_entry;
    ...
    for (count = 0; count < bufcnt; count++, buf_entries++, drv_buf_iter++) {
    ...
}

A user-space process submitting an MPI command via mpi3mr_bsg_request could
provide a small request_len but a large, unchecked
karg->buf_entry_list.num_of_entries.

Does this code overflow buf_entries[] by reading from arbitrary adjacent
heap memory?


[Severity: High]
This isn't a bug introduced by this patch, but does this code cause corruption
on big-endian systems?

drivers/scsi/mpi3mr/mpi3mr_app.c:mpi3mr_bsg_process_mpt_cmds() {
    ...
    dev_handle = tm_req->dev_handle;
    ...
}

And later in the same function:

drivers/scsi/mpi3mr/mpi3mr_app.c:mpi3mr_bsg_process_mpt_cmds() {
    ...
    status_desc->ioc_status = mrioc->bsg_cmds.ioc_status;
    status_desc->ioc_log_info = mrioc->bsg_cmds.ioc_loginfo;
    ...
}

When an admin issues a SCSI Task Management BSG command, tm_req->dev_handle
(__le16) is directly assigned to dev_handle (u16) without le16_to_cpu(), and
native values are assigned to __le16/__le32 fields without cpu_to_le16/32().

Can this fail the mpi3mr_get_tgtdev_by_handle() lookup and corrupt the
response sent to user-space, bypassing the block_io increment and enabling
unsafe concurrent I/O during target reset?

>  			memcpy(mpi_req, sgl_iter, mpi_msg_size);
>  			break;
>  		default:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825210411.301535-1-chandrakanth.patil@broadcom.com?part=2

[PATCH v2 03/17] mpi3mr: Fix I/O block counter leak on admin request post failure

[Chandrakanth Patil]

The per device I/O block counter is raised before a task management
request is posted and only lowered once the request completes. If the
post itself fails the counter stays raised and I/O to that device
remains blocked.

Lower it on the failure path as well.

Fixes: 506bc1a0d6ba ("scsi: mpi3mr: Add support for MPT commands")
Signed-off-by: Chandrakanth Patil <chandrakanth.patil@broadcom.com>
---
v2:
 - No changes from v1.
 drivers/scsi/mpi3mr/mpi3mr_app.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/scsi/mpi3mr/mpi3mr_app.c b/drivers/scsi/mpi3mr/mpi3mr_app.c
index 1cd1c38dcd56..a4fe10a05cb3 100644
--- a/drivers/scsi/mpi3mr/mpi3mr_app.c
+++ b/drivers/scsi/mpi3mr/mpi3mr_app.c
@@ -2782,6 +2782,8 @@ static long mpi3mr_bsg_process_mpt_cmds(struct bsg_job *job)
 		dprint_bsg_err(mrioc,
 		    "%s: posting bsg request is failed\n", __func__);
 		rval = -EAGAIN;
+		if (block_io && stgt_priv)
+			atomic_dec(&stgt_priv->block_io);
 		goto out_unlock;
 	}
 	wait_for_completion_timeout(&mrioc->bsg_cmds.done,
-- 
2.52.0

Re: [PATCH v2 03/17] mpi3mr: Fix I/O block counter leak on admin request post failure

[sashiko-bot]

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

Pre-existing issues:
- [High] Use-after-free of `stgt_priv` during BSG command completion and the error path, due to a lack of reference counting on the SCSI target.
- [High] Out-of-bounds read when processing BSG buffer entries due to missing bounds check on `num_of_entries` against `job->request_len`.
--

commit cf44d8f7dd728a84e0a426ff369621891f88ea4e
Author: Chandrakanth Patil <chandrakanth.patil@broadcom.com>

mpi3mr: Fix I/O block counter leak on admin request post failure

The per device I/O block counter is raised before a task management
request is posted and only lowered once the request completes. If the
post itself fails the counter stays raised, so this commit lowers it
on the failure path as well.

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_app.c b/drivers/scsi/mpi3mr/mpi3mr_app.c
> index 1cd1c38dcd565..a4fe10a05cb3a 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_app.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_app.c
> @@ -2782,6 +2782,8 @@ static long mpi3mr_bsg_process_mpt_cmds(struct bsg_job *job)
>  		dprint_bsg_err(mrioc,
>  		    "%s: posting bsg request is failed\n", __func__);
>  		rval = -EAGAIN;
> +		if (block_io && stgt_priv)
> +			atomic_dec(&stgt_priv->block_io);

[Severity: High]
This isn't a bug introduced by this patch, but does dropping the target
device reference earlier leave stgt_priv unprotected?

Earlier in this function, the tgtdev reference is dropped without pinning
the SCSI target:

	if (tgtdev->starget && tgtdev->starget->hostdata) {
		stgt_priv = (struct mpi3mr_stgt_priv_data *)
		    tgtdev->starget->hostdata;
		atomic_inc(&stgt_priv->block_io);
	}
	mpi3mr_tgtdev_put(tgtdev);

If a concurrent device removal occurs, the SCSI midlayer could destroy the
target and free its hostdata via mpi3mr_target_destroy() while the driver
is processing the request or sleeping in wait_for_completion_timeout().

Could this lead to a use-after-free when atomic_dec(&stgt_priv->block_io)
is called on this error path and on the completion path?

>  		goto out_unlock;
>  	}

[Severity: High]
This is also a pre-existing issue, but does this function properly validate
the number of buffer entries against the allocated request payload size?

Earlier in mpi3mr_bsg_process_mpt_cmds():

	bufcnt = karg->buf_entry_list.num_of_entries;
	...
	for (count = 0; count < bufcnt; count++, buf_entries++, drv_buf_iter++) {
		switch (buf_entries->buf_type) {

If a userspace application supplies a small request_len but a large
num_of_entries, could the loop read past the end of the job->request payload?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825210411.301535-1-chandrakanth.patil@broadcom.com?part=3

[PATCH v2 04/17] mpi3mr: Fix target device reference leak in BSG task management

[Chandrakanth Patil]

The target device lookup takes a reference, but it is only dropped
inside a branch that also requires the SCSI target data to be set up.
When it is not, the reference is leaked.

Drop the reference whenever the lookup succeeded.

Fixes: 506bc1a0d6ba ("scsi: mpi3mr: Add support for MPT commands")
Signed-off-by: Chandrakanth Patil <chandrakanth.patil@broadcom.com>
---
v2:
 - No changes from v1.
 drivers/scsi/mpi3mr/mpi3mr_app.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/mpi3mr/mpi3mr_app.c b/drivers/scsi/mpi3mr/mpi3mr_app.c
index a4fe10a05cb3..94b992acb233 100644
--- a/drivers/scsi/mpi3mr/mpi3mr_app.c
+++ b/drivers/scsi/mpi3mr/mpi3mr_app.c
@@ -2745,10 +2745,12 @@ static long mpi3mr_bsg_process_mpt_cmds(struct bsg_job *job)
 	}
 	if (block_io) {
 		tgtdev = mpi3mr_get_tgtdev_by_handle(mrioc, dev_handle);
-		if (tgtdev && tgtdev->starget && tgtdev->starget->hostdata) {
-			stgt_priv = (struct mpi3mr_stgt_priv_data *)
-			    tgtdev->starget->hostdata;
-			atomic_inc(&stgt_priv->block_io);
+		if (tgtdev) {
+			if (tgtdev->starget && tgtdev->starget->hostdata) {
+				stgt_priv = (struct mpi3mr_stgt_priv_data *)
+				    tgtdev->starget->hostdata;
+				atomic_inc(&stgt_priv->block_io);
+			}
 			mpi3mr_tgtdev_put(tgtdev);
 		}
 	}
-- 
2.52.0

Re: [PATCH v2 04/17] mpi3mr: Fix target device reference leak in BSG task management

[sashiko-bot]

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

Pre-existing issues:
- [High] Time-of-check to time-of-use (TOCTOU) race condition leading to a Use-After-Free on `stgt_priv`.
- [High] Data race between BSG timeout handling and admin reply ISR causing a Use-After-Free on `sense_buff_k`.
- [Medium] Missing endianness conversion when reading `dev_handle` from `mpi3_scsi_task_mgmt_request`.
--

commit da89cfba68bbe3f0b3052ef72f3e8e056a2cdb4d
Author: Chandrakanth Patil <chandrakanth.patil@broadcom.com>

mpi3mr: Fix target device reference leak in BSG task management

This commit fixes a reference leak for the target device in the BSG task
management path. It ensures the reference is dropped whenever the device
lookup succeeds, regardless of whether the SCSI target data is fully set up.

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_app.c b/drivers/scsi/mpi3mr/mpi3mr_app.c
> index a4fe10a05cb3a..94b992acb2330 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_app.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_app.c
> @@ -2745,10 +2745,12 @@ static long mpi3mr_bsg_process_mpt_cmds(struct bsg_job *job)
>  	}
>  	if (block_io) {
>  		tgtdev = mpi3mr_get_tgtdev_by_handle(mrioc, dev_handle);

[Severity: Medium]
This isn't a bug introduced by this patch, but is dev_handle missing an
endianness conversion here?

It appears dev_handle is read natively from the request structure:

mpi3mr_bsg_process_mpt_cmds() {
    ...
    dev_handle = tm_req->dev_handle;
    ...
}

Since dev_handle is defined as little-endian (__le16) in
struct mpi3_scsi_task_mgmt_request, could Big-Endian systems read a
byte-swapped value, causing mpi3mr_get_tgtdev_by_handle() to look up the
wrong device?

> -		if (tgtdev && tgtdev->starget && tgtdev->starget->hostdata) {
> -			stgt_priv = (struct mpi3mr_stgt_priv_data *)
> -			    tgtdev->starget->hostdata;
> -			atomic_inc(&stgt_priv->block_io);
> +		if (tgtdev) {
> +			if (tgtdev->starget && tgtdev->starget->hostdata) {
> +				stgt_priv = (struct mpi3mr_stgt_priv_data *)
> +				    tgtdev->starget->hostdata;
> +				atomic_inc(&stgt_priv->block_io);

[Severity: High]
This is a pre-existing issue, but does caching stgt_priv here create a
use-after-free risk later in the function?

When a user-space process issues a blocking command, the thread will wait
for completion:

mpi3mr_bsg_process_mpt_cmds() {
    ...
    wait_for_completion_timeout(&mrioc->bsg_cmds.done, ...);
    if (block_io && stgt_priv)
        atomic_dec(&stgt_priv->block_io);
    ...
}

If the target device and its hostdata are concurrently destroyed while the
thread is blocked, tgt_dev->starget is cleared:

mpi3mr_target_destroy() {
    ...
    tgt_dev->starget = NULL;
    ...
}

If this race occurs, wouldn't the BSG thread wake up and dereference the
freed stgt_priv pointer when calling atomic_dec()?

> +			}
>  			mpi3mr_tgtdev_put(tgtdev);
>  		}

[Severity: High]
This is also a pre-existing issue, but is there a data race in the BSG
timeout handling that can cause a use-after-free on the sense buffer?

In the timeout path, the user thread locklessly clears the pointer and
frees the memory:

mpi3mr_bsg_process_mpt_cmds() {
    ...
    mrioc->bsg_cmds.sensebuf = NULL;
    mrioc->bsg_cmds.state = MPI3MR_CMD_NOTUSED;
    ...
    kfree(sense_buff_k);
    ...
}

If the admin reply ISR executes concurrently and checks the cmdptr->sensebuf
condition before it is cleared:

mpi3mr_fw.c:mpi3mr_process_admin_reply_desc() {
    if (sense_buf && cmdptr->sensebuf) {
        cmdptr->is_sense = 1;
        memcpy(cmdptr->sensebuf, sense_buf, MPI3MR_SENSE_BUF_SZ);
    }
    ...
}

Could the ISR execute memcpy() into the freed sense_buff_k memory block?

>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825210411.301535-1-chandrakanth.patil@broadcom.com?part=4

[PATCH v2 05/17] mpi3mr: Fix buffer overflow when caching log data

[Chandrakanth Patil]

Each log data slot holds a header followed by the payload, but the copy
was sized against the whole slot and so wrote one header length past
the end of it. Subtracting the header on its own is not enough either,
because the entry size is derived from the controller reply size and
can be smaller than the header.

Work out the payload room first and clamp the copy to it.

Fixes: 43ca11005098 ("scsi: mpi3mr: Add support for PEL commands")
Signed-off-by: Chandrakanth Patil <chandrakanth.patil@broadcom.com>
---
v2:
 - No changes from v1.
 drivers/scsi/mpi3mr/mpi3mr_app.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/mpi3mr/mpi3mr_app.c b/drivers/scsi/mpi3mr/mpi3mr_app.c
index 94b992acb233..cd772b2cb98a 100644
--- a/drivers/scsi/mpi3mr/mpi3mr_app.c
+++ b/drivers/scsi/mpi3mr/mpi3mr_app.c
@@ -2947,7 +2947,8 @@ static long mpi3mr_bsg_process_mpt_cmds(struct bsg_job *job)
 void mpi3mr_app_save_logdata_th(struct mpi3mr_ioc *mrioc, char *event_data,
 	u16 event_data_size)
 {
-	u32 index = mrioc->logdata_buf_idx, sz;
+	u32 index = mrioc->logdata_buf_idx;
+	size_t entry_payload_len, sz;
 	struct mpi3mr_logdata_entry *entry;
 
 	if (!(mrioc->logdata_buf))
@@ -2956,7 +2957,12 @@ void mpi3mr_app_save_logdata_th(struct mpi3mr_ioc *mrioc, char *event_data,
 	entry = (struct mpi3mr_logdata_entry *)
 		(mrioc->logdata_buf + (index * mrioc->logdata_entry_sz));
 	entry->valid_entry = 1;
-	sz = min(mrioc->logdata_entry_sz, event_data_size);
+	if (mrioc->logdata_entry_sz > MPI3MR_BSG_LOGDATA_ENTRY_HEADER_SZ)
+		entry_payload_len = (size_t)mrioc->logdata_entry_sz -
+		    MPI3MR_BSG_LOGDATA_ENTRY_HEADER_SZ;
+	else
+		entry_payload_len = 0;
+	sz = min_t(size_t, entry_payload_len, event_data_size);
 	memcpy(entry->data, event_data, sz);
 	mrioc->logdata_buf_idx =
 		((++index) % MPI3MR_BSG_LOGDATA_MAX_ENTRIES);
-- 
2.52.0

Re: [PATCH v2 05/17] mpi3mr: Fix buffer overflow when caching log data

[sashiko-bot]

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

Pre-existing issues:
- [High] Unvalidated firmware input allows a small `reply_frame_size`, leading to `logdata_entry_sz == 0`, which causes a ZERO_SIZE_PTR dereference and Division by Zero.
- [High] Integer overflow in `sz` calculation in `mpi3mr_get_logdata` leads to silent data truncation.
- [High] Missing locking and memory barriers for `logdata_buf` between BSG ioctl handlers and interrupt context causes torn reads and potential uninitialized memory access.
- [High] MPI3_EVENT_DEVICE_INFO_CHANGED falls through to MPI3_EVENT_LOG_DATA, ignoring device info changes and polluting log data.
- [High] Copying into a 1-element trailing array will trigger a FORTIFY_SOURCE panic under -fstrict-flex-arrays=3.
--

commit 64e5ca8eae679f9947b56b20427a7ee194af1d35
Author: Chandrakanth Patil <chandrakanth.patil@broadcom.com>

mpi3mr: Fix buffer overflow when caching log data

This patch modifies mpi3mr_app_save_logdata_th to calculate the payload
room first and clamp the copy to it. It prevents a buffer overflow when
the entry size is derived from the controller reply size and is smaller
than the header.

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_app.c b/drivers/scsi/mpi3mr/mpi3mr_app.c
> index 94b992acb2330..cd772b2cb98ae 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_app.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_app.c
> @@ -2947,7 +2947,8 @@ static long mpi3mr_bsg_process_mpt_cmds(struct bsg_job *job)
>  void mpi3mr_app_save_logdata_th(struct mpi3mr_ioc *mrioc, char *event_data,
>  	u16 event_data_size)
>  {
> -	u32 index = mrioc->logdata_buf_idx, sz;
> +	u32 index = mrioc->logdata_buf_idx;
> +	size_t entry_payload_len, sz;
>  	struct mpi3mr_logdata_entry *entry;
>  
>  	if (!(mrioc->logdata_buf))
>  		return;

[Severity: High]
This is a pre-existing issue, but are there missing memory barriers or locking
when accessing mrioc->logdata_buf?

mpi3mr_enable_logdata() allocates and initializes mrioc->logdata_buf,
mrioc->logdata_entry_sz, and mrioc->logdata_buf_idx from a user-space
BSG ioctl context without any serialization against the interrupt handler.

If firmware events trigger mpi3mr_app_save_logdata_th() via the hardware
interrupt handler concurrently, the lack of memory barriers might allow the
interrupt handler to observe a non-NULL buffer but an uninitialized size/index,
leading to corrupt calculations or out-of-bounds writes.

Additionally, user space copies the log buffer without serialization:

drivers/scsi/mpi3mr/mpi3mr_app.c:mpi3mr_get_logdata() {
    ...
    sg_copy_from_buffer(job->request_payload.sg_list,
                        job->request_payload.sg_cnt,
                        mrioc->logdata_buf, sz);
    ...
}

Could this lead to torn reads or corrupted logs if the interrupt handler
overwrites the ring buffer entries during the user-space copy operation?

>  
>  	entry = (struct mpi3mr_logdata_entry *)
>  		(mrioc->logdata_buf + (index * mrioc->logdata_entry_sz));
>  	entry->valid_entry = 1;

[Severity: High]
This is a pre-existing issue, but might this dereference a ZERO_SIZE_PTR and
cause a kernel panic?

If the device firmware sets reply_frame_size to 5 (or less),
mrioc->reply_sz becomes 20 (or less). In mpi3mr_enable_logdata():

drivers/scsi/mpi3mr/mpi3mr_app.c:mpi3mr_enable_logdata() {
    ...
    mrioc->logdata_entry_sz =
        (mrioc->reply_sz - (sizeof(struct mpi3_event_notification_reply) - 4))
        + MPI3MR_BSG_LOGDATA_ENTRY_HEADER_SZ;
    ...
}

Since the sizeof expression evaluates as a size_t, 20 - 24 underflows. The
result plus 4 wraps around to exactly 0 when assigned to the u16
logdata_entry_sz.

This causes kcalloc to return ZERO_SIZE_PTR (0x10). The check
if (!mrioc->logdata_buf) earlier in this function fails to catch this, leading
to a panic when writing to the unmapped address 0x10.

Additionally, could this cause a division by zero in mpi3mr_get_logdata()
when calculating num_entries?

drivers/scsi/mpi3mr/mpi3mr_app.c:mpi3mr_get_logdata() {
    ...
    num_entries = job->request_payload.payload_len / entry_sz;
    ...
}

> -	sz = min(mrioc->logdata_entry_sz, event_data_size);
> +	if (mrioc->logdata_entry_sz > MPI3MR_BSG_LOGDATA_ENTRY_HEADER_SZ)
> +		entry_payload_len = (size_t)mrioc->logdata_entry_sz -
> +		    MPI3MR_BSG_LOGDATA_ENTRY_HEADER_SZ;
> +	else
> +		entry_payload_len = 0;
> +	sz = min_t(size_t, entry_payload_len, event_data_size);
>  	memcpy(entry->data, event_data, sz);

[Severity: High]
This isn't a bug introduced by this patch, but will this memcpy trigger a
FORTIFY_SOURCE panic under -fstrict-flex-arrays=3?

The log data payload is copied into entry->data, which is defined as
__u8 data[1]; in the UAPI header struct mpi3mr_logdata_entry.

When compiled with -fstrict-flex-arrays=3, the compiler treats data strictly
as a 1-byte array. The fortified memcpy() will evaluate __member_size() as 1,
and since log events typically have a size > 1, this will deterministically
trigger a kernel panic upon receiving a log data event due to the detected
buffer overflow.

>  	mrioc->logdata_buf_idx =
>  		((++index) % MPI3MR_BSG_LOGDATA_MAX_ENTRIES);

[Severity: High]
This isn't a bug introduced by this patch, but could the size calculation in
mpi3mr_get_logdata() overflow and lead to silent data truncation?

drivers/scsi/mpi3mr/mpi3mr_app.c:mpi3mr_get_logdata() {
    u16 num_entries, sz, entry_sz = mrioc->logdata_entry_sz;
    ...
    sz = num_entries * entry_sz;
    ...
}

num_entries can be up to 400 (MPI3MR_BSG_LOGDATA_MAX_ENTRIES), and entry_sz is
typically 236 bytes. The product 400 * 236 equals 94400, which exceeds the
maximum value of a 16-bit unsigned integer (65535). 

Might this silently truncate/wrap around to 28864, causing
sg_copy_from_buffer() to return incomplete or corrupted log data back to
user space without generating an error?

[Severity: High]
This is a pre-existing issue, but is there an unintentional fall-through from
MPI3_EVENT_DEVICE_INFO_CHANGED to MPI3_EVENT_LOG_DATA?

drivers/scsi/mpi3mr/mpi3mr_os.c:mpi3mr_os_handle_events() {
    ...
    case MPI3_EVENT_DEVICE_INFO_CHANGED:
    case MPI3_EVENT_LOG_DATA:

        if (mrioc->reply_sz > offsetof(struct mpi3_event_notification_reply,
                                       event_data))
            avail_reply_room = mrioc->reply_sz -
                offsetof(struct mpi3_event_notification_reply, event_data);
    ...
}

The switch case for MPI3_EVENT_DEVICE_INFO_CHANGED lacks the required
process_evt_bh = 1; assignment and a break; statement. 

Will this cause device information changes to be permanently ignored (since
they are never queued to the fwevt_worker_thread), while erroneously passing the
device page0 data payload to mpi3mr_app_save_logdata_th(), polluting the
log data ring buffer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825210411.301535-1-chandrakanth.patil@broadcom.com?part=5

[PATCH v2 06/17] mpi3mr: Fix out-of-bounds reply frame access

[Chandrakanth Patil]

The reply frame address reported on completion is only checked against
the start and the end of the pool. An address near the top can pass the
check while leaving less than a full frame, and an unaligned one
resolves into the middle of a frame instead of the start of one.

Require a whole frame to fit and the address to be frame aligned.

Fixes: 824a156633df ("scsi: mpi3mr: Base driver code")
Signed-off-by: Chandrakanth Patil <chandrakanth.patil@broadcom.com>
---
v2:
 - Switched to do_div() for alignment checking to avoid potential 64-bit
   division issues on 32-bit platforms.
 drivers/scsi/mpi3mr/mpi3mr_fw.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/mpi3mr/mpi3mr_fw.c b/drivers/scsi/mpi3mr/mpi3mr_fw.c
index 31b19ed1528e..c90358a9e950 100644
--- a/drivers/scsi/mpi3mr/mpi3mr_fw.c
+++ b/drivers/scsi/mpi3mr/mpi3mr_fw.c
@@ -123,11 +123,16 @@ void mpi3mr_build_zero_len_sge(void *paddr)
 void *mpi3mr_get_reply_virt_addr(struct mpi3mr_ioc *mrioc,
 	dma_addr_t phys_addr)
 {
+	u64 offset;
+
 	if (!phys_addr)
 		return NULL;
 
+	offset = phys_addr - mrioc->reply_buf_dma;
+
 	if ((phys_addr < mrioc->reply_buf_dma) ||
-	    (phys_addr > mrioc->reply_buf_dma_max_address))
+	    (phys_addr > mrioc->reply_buf_dma_max_address - mrioc->reply_sz) ||
+	    do_div(offset, mrioc->reply_sz))
 		return NULL;
 
 	return mrioc->reply_buf + (phys_addr - mrioc->reply_buf_dma);
-- 
2.52.0

[PATCH v2 07/17] mpi3mr: Fix out-of-bounds sense buffer access

[Chandrakanth Patil]

The sense buffer address reported on completion is turned into a
virtual address with no range check, so an address outside the pool
resolves to memory that does not belong to it.

Check that it lies within the pool and is correctly aligned.

Fixes: 824a156633df ("scsi: mpi3mr: Base driver code")
Signed-off-by: Chandrakanth Patil <chandrakanth.patil@broadcom.com>
---
v2:
 - Switched to do_div() for alignment checking to avoid potential 64-bit
   division issues on 32-bit platforms.
 drivers/scsi/mpi3mr/mpi3mr_fw.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/scsi/mpi3mr/mpi3mr_fw.c b/drivers/scsi/mpi3mr/mpi3mr_fw.c
index c90358a9e950..4f1e6293d576 100644
--- a/drivers/scsi/mpi3mr/mpi3mr_fw.c
+++ b/drivers/scsi/mpi3mr/mpi3mr_fw.c
@@ -141,9 +141,19 @@ void *mpi3mr_get_reply_virt_addr(struct mpi3mr_ioc *mrioc,
 void *mpi3mr_get_sensebuf_virt_addr(struct mpi3mr_ioc *mrioc,
 	dma_addr_t phys_addr)
 {
+	u64 offset;
+
 	if (!phys_addr)
 		return NULL;
 
+	offset = phys_addr - mrioc->sense_buf_dma;
+
+	if (phys_addr < mrioc->sense_buf_dma ||
+	    (phys_addr > mrioc->sense_buf_dma +
+	     (mrioc->num_sense_bufs * MPI3MR_SENSE_BUF_SZ) - MPI3MR_SENSE_BUF_SZ) ||
+	    do_div(offset, MPI3MR_SENSE_BUF_SZ))
+		return NULL;
+
 	return mrioc->sense_buf + (phys_addr - mrioc->sense_buf_dma);
 }
 
-- 
2.52.0

Re: [PATCH v2 07/17] mpi3mr: Fix out-of-bounds sense buffer access

[sashiko-bot]

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

Pre-existing issues:
- [High] Out-of-bounds reply_dma addresses are reposted back to the firmware in the admin queue, allowing potential memory corruption.
- [High] Missing `dma_rmb()` after polling the DMA completion queue phase bit allows the CPU to read stale descriptor and sense data on weakly-ordered architectures.
--

commit 9016299b1f02eb19e9cbac5395a3c1dc0ee4a8cc
Author: Chandrakanth Patil <chandrakanth.patil@broadcom.com>

mpi3mr: Fix out-of-bounds sense buffer access

The sense buffer address reported on completion is turned into a
virtual address with no range check, so an address outside the pool
resolves to memory that does not belong to it.

Check that it lies within the pool and is correctly aligned.

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_fw.c b/drivers/scsi/mpi3mr/mpi3mr_fw.c
> index 82fadb4d2b21f..5c2547bb67a52 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_fw.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_fw.c
> @@ -142,9 +142,19 @@ void *mpi3mr_get_reply_virt_addr(struct mpi3mr_ioc *mrioc,
>  void *mpi3mr_get_sensebuf_virt_addr(struct mpi3mr_ioc *mrioc,
>  	dma_addr_t phys_addr)
>  {
> +	u64 offset;
> +
>  	if (!phys_addr)
>  		return NULL;
>  
> +	offset = phys_addr - mrioc->sense_buf_dma;
> +
> +	if (phys_addr < mrioc->sense_buf_dma ||
> +	    (phys_addr > mrioc->sense_buf_dma +
> +	     (mrioc->num_sense_bufs * MPI3MR_SENSE_BUF_SZ) - MPI3MR_SENSE_BUF_SZ) ||
> +	    do_div(offset, MPI3MR_SENSE_BUF_SZ))
> +		return NULL;
> +
>  	return mrioc->sense_buf + (phys_addr - mrioc->sense_buf_dma);
>  }

While reviewing this patch, I noticed some pre-existing issues in the driver:

> static void mpi3mr_process_admin_reply_desc(struct mpi3mr_ioc *mrioc,
> 	struct mpi3_default_reply_descriptor *reply_desc, u64 *reply_dma)
> {
[ ... ]
> 	case MPI3_REPLY_DESCRIPT_FLAGS_TYPE_ADDRESS_REPLY:
> 		addr_desc = (struct mpi3_address_reply_descriptor *)reply_desc;
> 		*reply_dma = le64_to_cpu(addr_desc->reply_frame_address);
> 		def_reply = mpi3mr_get_reply_virt_addr(mrioc, *reply_dma);
> 		if (!def_reply)
> 			goto out;

[Severity: High]
This is a pre-existing issue, but does mpi3mr_process_admin_reply_desc()
leave *reply_dma populated with an invalid physical address when
mpi3mr_get_reply_virt_addr() returns NULL?

If def_reply is NULL, the code jumps to out without clearing *reply_dma.

Then in mpi3mr_process_admin_reply_q():

> 		mpi3mr_process_admin_reply_desc(mrioc, reply_desc, &reply_dma);
> 		if (reply_dma)
> 			mpi3mr_repost_reply_buf(mrioc, reply_dma);

Since *reply_dma was not cleared on error, doesn't this result in the
unvalidated or out-of-bounds address being blindly reposted to the hardware
via mpi3mr_repost_reply_buf()?

This could poison the hardware's reply free queue with arbitrary host
addresses, potentially allowing memory corruption during future DMA writes.


I also noticed another pre-existing issue in the polling logic:

> static int mpi3mr_process_admin_reply_q(struct mpi3mr_ioc *mrioc)
> {
[ ... ]
> 	if ((le16_to_cpu(reply_desc->reply_flags) &
> 	    MPI3_REPLY_DESCRIPT_FLAGS_PHASE_MASK) != exp_phase) {
> 		atomic_dec(&mrioc->admin_reply_q_in_use);
> 		return 0;
> 	}

[Severity: High]
This isn't a bug introduced by this patch, but is a dma_rmb() missing after
polling the DMA completion queue phase bit?

Without an address dependency between the phase bit and the rest of the
descriptor (since the pointer is already known), weakly-ordered CPU
architectures like ARM64 and PowerPC might speculatively fetch the rest of the
descriptor or the sense data before the phase bit read has actually completed.

The same pattern appears in mpi3mr_process_op_reply_q():

> 	if ((le16_to_cpu(reply_desc->reply_flags) &
> 	    MPI3_REPLY_DESCRIPT_FLAGS_PHASE_MASK) != exp_phase) {
> 		atomic_dec(&op_reply_q->in_use);
> 		return 0;
> 	}

And when the sense data is later read in mpi3mr_process_op_reply_desc() in
drivers/scsi/mpi3mr/mpi3mr_os.c:

> 		if (sense_buf) {
> 			scsi_normalize_sense(sense_buf, sense_count, &sshdr);

Could this result in the CPU reading stale descriptor or sense data? A
dma_rmb() seems required immediately after observing the phase bit toggle
across both the admin and operational completion paths.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825210411.301535-1-chandrakanth.patil@broadcom.com?part=7

[PATCH v2 08/17] mpi3mr: Fix out-of-bounds bitmap access during device removal

[Chandrakanth Patil]

Device handles reported by the controller are used to index the remove
pending bitmap and to build a task management request without being
compared against the maximum handle the controller reported.

Check the handle before using it.

Fixes: 13ef29ea4aa0 ("scsi: mpi3mr: Add support for device add/remove event handling")
Signed-off-by: Chandrakanth Patil <chandrakanth.patil@broadcom.com>
---
v2:
 - Relocated handle bounds check to the entry of mpi3mr_dev_rmhs_send_tm()
   so out-of-bounds handles are rejected immediately without polluting
   delayed_rmhs_list.
 drivers/scsi/mpi3mr/mpi3mr_os.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
index f80a21ec161b..2a35f146fe69 100644
--- a/drivers/scsi/mpi3mr/mpi3mr_os.c
+++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
@@ -2401,7 +2401,8 @@ static void mpi3mr_dev_rmhs_complete_iou(struct mpi3mr_ioc *mrioc,
 		ioc_info(mrioc,
 		    "%s :dev removal handshake completed successfully: handle(0x%04x)\n",
 		    __func__, drv_cmd->dev_handle);
-		clear_bit(drv_cmd->dev_handle, mrioc->removepend_bitmap);
+		if (drv_cmd->dev_handle < mrioc->facts.max_devhandle)
+			clear_bit(drv_cmd->dev_handle, mrioc->removepend_bitmap);
 	}
 
 	if (!list_empty(&mrioc->delayed_rmhs_list)) {
@@ -2515,6 +2516,20 @@ static void mpi3mr_dev_rmhs_send_tm(struct mpi3mr_ioc *mrioc, u16 handle,
 	struct mpi3mr_tgt_dev *tgtdev = NULL;
 	unsigned long flags;
 
+	if (handle >= mrioc->facts.max_devhandle) {
+		ioc_err(mrioc, "dev_remove_hs: handle(0x%04x) >= max_devhandle(0x%04x)\n",
+			handle, mrioc->facts.max_devhandle);
+		if (drv_cmd) {
+			cmd_idx = drv_cmd->host_tag - MPI3MR_HOSTTAG_DEVRMCMD_MIN;
+			drv_cmd->state = MPI3MR_CMD_NOTUSED;
+			drv_cmd->callback = NULL;
+			drv_cmd->dev_handle = MPI3MR_INVALID_DEV_HANDLE;
+			drv_cmd->retry_count = 0;
+			clear_bit(cmd_idx, mrioc->devrem_bitmap);
+		}
+		return;
+	}
+
 	spin_lock_irqsave(&mrioc->tgtdev_lock, flags);
 	tgtdev = __mpi3mr_get_tgtdev_by_handle(mrioc, handle);
 	if (tgtdev && (iou_rc == MPI3_CTRL_OP_REMOVE_DEVICE))
-- 
2.52.0

Re: [PATCH v2 08/17] mpi3mr: Fix out-of-bounds bitmap access during device removal

[sashiko-bot]

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

Pre-existing issues:
- [High] Failure to dispatch a Task Management request permanently strands the delayed device removal queue.
- [High] Silent failure during memory allocation drops device removal requests.
--

commit caefa471752245fefd26d7a88577789dca45c515
Author: Chandrakanth Patil <chandrakanth.patil@broadcom.com>

mpi3mr: Fix out-of-bounds bitmap access during device removal

This commit checks the device handle before indexing the remove pending
bitmap or building a task management request, preventing out-of-bounds
access if the controller reports an invalid handle.

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
> index f80a21ec161b6..2a35f146fe69b 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_os.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
> @@ -2401,7 +2401,8 @@ static void mpi3mr_dev_rmhs_complete_iou(struct mpi3mr_ioc *mrioc,
>  		ioc_info(mrioc,
>  		    "%s :dev removal handshake completed successfully: handle(0x%04x)\n",
>  		    __func__, drv_cmd->dev_handle);
> -		clear_bit(drv_cmd->dev_handle, mrioc->removepend_bitmap);
> +		if (drv_cmd->dev_handle < mrioc->facts.max_devhandle)
> +			clear_bit(drv_cmd->dev_handle, mrioc->removepend_bitmap);
>  	}
>  
>  	if (!list_empty(&mrioc->delayed_rmhs_list)) {

[Severity: High]
This is a pre-existing issue, but does a dispatch failure permanently strand
the delayed device removal queue here?

If mpi3mr_dev_rmhs_send_tm() is called to process a delayed TM request, but
mpi3mr_admin_request_post() fails during dispatch:

    retval = mpi3mr_admin_request_post(mrioc, &tm_req, sizeof(tm_req), 1);
    if (retval) {
        ...
        goto out_failed;
    }
    ...
out_failed:
    drv_cmd->state = MPI3MR_CMD_NOTUSED;
    clear_bit(cmd_idx, mrioc->devrem_bitmap);

The command is marked as unused and dropped. Because
mpi3mr_dev_rmhs_complete_iou() only dequeues one element from
delayed_rmhs_list and does not loop if the dispatch fails, the active
command slot becomes idle.

Does this break the chain of asynchronous completions, leaving any remaining
elements in the delayed list stranded indefinitely?

> @@ -2515,6 +2516,20 @@ static void mpi3mr_dev_rmhs_send_tm(struct mpi3mr_ioc *mrioc, u16 handle,
>  	struct mpi3mr_tgt_dev *tgtdev = NULL;
>  	unsigned long flags;
>  
> +	if (handle >= mrioc->facts.max_devhandle) {
> +		ioc_err(mrioc, "dev_remove_hs: handle(0x%04x) >= max_devhandle(0x%04x)\n",
> +			handle, mrioc->facts.max_devhandle);
> +		if (drv_cmd) {
> +			cmd_idx = drv_cmd->host_tag - MPI3MR_HOSTTAG_DEVRMCMD_MIN;
> +			drv_cmd->state = MPI3MR_CMD_NOTUSED;
> +			drv_cmd->callback = NULL;
> +			drv_cmd->dev_handle = MPI3MR_INVALID_DEV_HANDLE;
> +			drv_cmd->retry_count = 0;
> +			clear_bit(cmd_idx, mrioc->devrem_bitmap);
> +		}
> +		return;
> +	}
> +
>  	spin_lock_irqsave(&mrioc->tgtdev_lock, flags);

[Severity: High]
This is a pre-existing issue, but does a memory allocation failure silently
drop device removal requests here?

Later in mpi3mr_dev_rmhs_send_tm(), if no slots are available in
devrem_bitmap, the code falls back to adding the request to the delayed
list:

    if (cmd_idx >= MPI3MR_NUM_DEVRMCMD) {
        delayed_dev_rmhs = kzalloc_obj(*delayed_dev_rmhs, GFP_ATOMIC);
        if (!delayed_dev_rmhs)
            return;

If the atomic allocation fails, the function returns silently without
propagating an error. Can this result in the controller waiting
indefinitely for a handshake if the removal event is lost?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825210411.301535-1-chandrakanth.patil@broadcom.com?part=8

[PATCH v2 09/17] mpi3mr: Fix target device reference leak in device removal handshake

[Chandrakanth Patil]

The device removal handshake looks up the target device to update its
state but never drops the reference that the lookup takes.

Drop it once the state has been updated.

Fixes: 13ef29ea4aa0 ("scsi: mpi3mr: Add support for device add/remove event handling")
Signed-off-by: Chandrakanth Patil <chandrakanth.patil@broadcom.com>
---
v2:
 - No changes from v1.
 drivers/scsi/mpi3mr/mpi3mr_os.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
index 2a35f146fe69..9aeec6146acb 100644
--- a/drivers/scsi/mpi3mr/mpi3mr_os.c
+++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
@@ -2536,6 +2536,9 @@ static void mpi3mr_dev_rmhs_send_tm(struct mpi3mr_ioc *mrioc, u16 handle,
 		tgtdev->state = MPI3MR_DEV_REMOVE_HS_STARTED;
 	spin_unlock_irqrestore(&mrioc->tgtdev_lock, flags);
 
+	if (tgtdev)
+		mpi3mr_tgtdev_put(tgtdev);
+
 	if (drv_cmd)
 		goto issue_cmd;
 	do {
-- 
2.52.0

Re: [PATCH v2 09/17] mpi3mr: Fix target device reference leak in device removal handshake

[sashiko-bot]

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

Pre-existing issues:
- [Medium] Target device reference count leak on `mpi3mr_target_alloc` failure paths.
- [Medium] Silent failure and wedged hardware state machine when device removal handshake TM allocation fails.
--

commit e0b0b39b67b7199d7920836b376fd3080aa21730
Author: Chandrakanth Patil <chandrakanth.patil@broadcom.com>

mpi3mr: Fix target device reference leak in device removal handshake

The device removal handshake looks up the target device to update its
state but never drops the reference that the lookup takes.

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
> index 2a35f146fe69b..9aeec6146acbb 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_os.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
> @@ -2536,6 +2536,9 @@ static void mpi3mr_dev_rmhs_send_tm(struct mpi3mr_ioc *mrioc, u16 handle,
>  		tgtdev->state = MPI3MR_DEV_REMOVE_HS_STARTED;
>  	spin_unlock_irqrestore(&mrioc->tgtdev_lock, flags);
>  
> +	if (tgtdev)
> +		mpi3mr_tgtdev_put(tgtdev);
> +
>  	if (drv_cmd)
>  		goto issue_cmd;
>  	do {

[Severity: Medium]
This isn't a bug introduced by this patch, but further down in
mpi3mr_dev_rmhs_send_tm(), if the pre-allocated command slots are exhausted,
the code falls back to allocating a delayed_dev_rmhs node:

    delayed_dev_rmhs = kzalloc_obj(*delayed_dev_rmhs, GFP_ATOMIC);
    if (!delayed_dev_rmhs)
        return;

If this GFP_ATOMIC allocation fails, the function returns without scheduling a
retry or rolling back the device state (MPI3MR_DEV_REMOVE_HS_STARTED).

Can this silent failure leave the hardware state machine permanently wedged,
waiting for the host to send this TM request?

[Severity: Medium]
This is a pre-existing issue, but looking at mpi3mr_target_alloc() in the same
file, there appears to be a reference count leak on the error paths.

If __mpi3mr_get_tgtdev_by_perst_id() or
__mpi3mr_get_tgtdev_by_addr_and_rphy() successfully finds a target device,
but the device doesn't satisfy the conditional checks for PCIe devices:

    if (tgt_dev && !tgt_dev->is_hidden && tgt_dev->non_stl) {
        ...
    } else
        retval = -ENXIO;

Or for SAS/SATA devices:

    if (tgt_dev && !tgt_dev->is_hidden && !tgt_dev->non_stl &&
        (tgt_dev->dev_type == MPI3_DEVICE_DEVFORM_SAS_SATA)) {
        ...
    } else
        retval = -ENXIO;

In both cases, the code returns -ENXIO without dropping the reference count
via mpi3mr_tgtdev_put(tgt_dev), and without assigning it to
scsi_tgt_priv_data->tgt_dev for later cleanup.

Would this leak the mpi3mr_tgt_dev memory structures during device scanning
if hidden or non-standard devices are present?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825210411.301535-1-chandrakanth.patil@broadcom.com?part=9

[PATCH v2 10/17] mpi3mr: Fix out-of-bounds read in SAS topology change events

[Chandrakanth Patil]

The number of entries in a SAS topology change event is used to walk
the entry array without being compared against the amount of event data
that was received, so the walk can run past the end of the buffer.

Work out how many entries the payload can hold and skip the event if it
claims more.

Fixes: 13ef29ea4aa0 ("scsi: mpi3mr: Add support for device add/remove event handling")
Signed-off-by: Chandrakanth Patil <chandrakanth.patil@broadcom.com>
---
v2:
 - Cached num_entries in a local variable before bounds check and used it
   for the loop condition to prevent re-fetching from DMA memory (TOCTOU).
 drivers/scsi/mpi3mr/mpi3mr_os.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
index 9aeec6146acb..6a80b784200b 100644
--- a/drivers/scsi/mpi3mr/mpi3mr_os.c
+++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
@@ -2827,12 +2827,28 @@ static void mpi3mr_sastopochg_evt_th(struct mpi3mr_ioc *mrioc,
 	struct mpi3_event_data_sas_topology_change_list *topo_evt =
 	    (struct mpi3_event_data_sas_topology_change_list *)event_reply->event_data;
 	int i;
-	u16 handle;
-	u8 reason_code;
+	u16 handle, avail_len;
+	u8 reason_code, max_entries, num_entries;
 	struct mpi3mr_tgt_dev *tgtdev = NULL;
 	struct mpi3mr_stgt_priv_data *scsi_tgt_priv_data = NULL;
 
-	for (i = 0; i < topo_evt->num_entries; i++) {
+	avail_len = event_reply->event_data_length * 4;
+	if (avail_len < offsetof(struct mpi3_event_data_sas_topology_change_list, phy_entry)) {
+		ioc_err(mrioc, "SAS topology event: event data too small (%u bytes)\n",
+			avail_len);
+		return;
+	}
+	max_entries = (avail_len -
+		      offsetof(struct mpi3_event_data_sas_topology_change_list, phy_entry)) /
+		      sizeof(struct mpi3_event_sas_topo_phy_entry);
+	num_entries = topo_evt->num_entries;
+	if (num_entries > max_entries) {
+		ioc_err(mrioc, "SAS topology event: num_entries(%d) exceeds max(%d)\n",
+			num_entries, max_entries);
+		return;
+	}
+
+	for (i = 0; i < num_entries; i++) {
 		handle = le16_to_cpu(topo_evt->phy_entry[i].attached_dev_handle);
 		if (!handle)
 			continue;
-- 
2.52.0

Re: [PATCH v2 10/17] mpi3mr: Fix out-of-bounds read in SAS topology change events

[sashiko-bot]

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

New issues:
- [High] Compiler-inserted bounds checks from __counted_by bypass the local caching of num_entries, defeating the TOCTOU mitigation and risking spurious kernel panics.

Pre-existing issues:
- [High] The bounds check aborts the top-half handler but fails to prevent the malformed event from reaching the bottom-half handler, resulting in a heap out-of-bounds read.
- [High] Lockless access of tgtdev->starget in event handlers leads to Use-After-Free racing with mpi3mr_target_destroy().
--

commit 9d8265fdf635644a7462d4ecd8e229ec48572ac3
Author: Chandrakanth Patil <chandrakanth.patil@broadcom.com>

mpi3mr: Fix out-of-bounds read in SAS topology change events

The number of entries in a SAS topology change event is used to walk
the entry array without being compared against the amount of event data
that was received, so the walk can run past the end of the buffer.

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
> index 9aeec6146acbb..6a80b784200b3 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_os.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
> @@ -2827,12 +2827,28 @@ static void mpi3mr_sastopochg_evt_th(struct mpi3mr_ioc *mrioc,
[ ... ]
> +	if (num_entries > max_entries) {
> +		ioc_err(mrioc, "SAS topology event: num_entries(%d) exceeds max(%d)\n",
> +			num_entries, max_entries);
> +		return;

[Severity: High]
This is a pre-existing issue, but returning early here aborts top-half
processing without clearing the caller's process_evt_bh flag.

In drivers/scsi/mpi3mr/mpi3mr_os.c:mpi3mr_os_handle_events(), process_evt_bh
is set to 1 before calling mpi3mr_sastopochg_evt_th(). If we return early,
the event is unconditionally scheduled for bottom-half processing.

Since the bottom-half blindly loops using the attacker-controlled num_entries
on the fwevt buffer:

drivers/scsi/mpi3mr/mpi3mr_os.c:mpi3mr_sastopochg_evt_bh() {
    ...
    for (i = 0; i < event_data->num_entries; i++) {
    ...
}

Could this leave the bottom-half exposed to a heap out-of-bounds read when
processing a malformed event?

> +	}
> +
> +	for (i = 0; i < num_entries; i++) {
>  		handle = le16_to_cpu(topo_evt->phy_entry[i].attached_dev_handle);

[Severity: High]
Since phy_entry is annotated with __counted_by(num_entries), won't the
compiler-inserted bounds check for topo_evt->phy_entry[i] directly re-fetch
topo_evt->num_entries from DMA memory?

If the firmware alters topo_evt->num_entries to be less than the loop index i
during the loop in mpi3mr_sastopochg_evt_th(), wouldn't this bypass the local
num_entries cache and trigger an array bounds sanitizer violation panic?

[Severity: High]
This isn't a bug introduced by this patch, but further down in
mpi3mr_sastopochg_evt_th() there is a lockless access to tgtdev->starget
that can race with device removal:

drivers/scsi/mpi3mr/mpi3mr_os.c:mpi3mr_sastopochg_evt_th() {
    ...
    if (tgtdev && tgtdev->starget && tgtdev->starget->hostdata)
        scsi_tgt_priv_data = (struct mpi3mr_stgt_priv_data *)
            tgtdev->starget->hostdata;
    ...
    case MPI3_EVENT_SAS_TOPO_PHY_RC_TARG_NOT_RESPONDING:
        if (scsi_tgt_priv_data) {
            scsi_tgt_priv_data->dev_removed = 1;
    ...
}

Is it possible that if this executes concurrently with mpi3mr_target_destroy(),
tgtdev->starget could be cleared under lock and the scsi_target freed by the
SCSI midlayer, causing a Use-After-Free when writing to
scsi_tgt_priv_data->dev_removed?

>  		if (!handle)
>  			continue;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825210411.301535-1-chandrakanth.patil@broadcom.com?part=10

[PATCH v2 11/17] mpi3mr: Fix out-of-bounds read of event data

[Chandrakanth Patil]

The event data length from the reply is used as is, both when caching
log data and when sizing the buffer handed to the bottom half. A length
larger than the frame makes both of them read past the end of it.

Clamp the length to what the frame can hold.

Fixes: 13ef29ea4aa0 ("scsi: mpi3mr: Add support for device add/remove event handling")
Fixes: d0d19250ed81 ("scsi: mpi3mr: Rename log data save helper to reflect threaded/BH context")
Signed-off-by: Chandrakanth Patil <chandrakanth.patil@broadcom.com>
---
v2:
 - No changes from v1.
 drivers/scsi/mpi3mr/mpi3mr_os.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
index 6a80b784200b..65a6192cf396 100644
--- a/drivers/scsi/mpi3mr/mpi3mr_os.c
+++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
@@ -3118,7 +3118,7 @@ void mpi3mr_add_event_wait_for_device_refresh(struct mpi3mr_ioc *mrioc)
 void mpi3mr_os_handle_events(struct mpi3mr_ioc *mrioc,
 	struct mpi3_event_notification_reply *event_reply)
 {
-	u16 evt_type, sz;
+	u16 evt_type, sz, avail_reply_room;
 	struct mpi3mr_fwevt *fwevt = NULL;
 	bool ack_req = 0, process_evt_bh = 0;
 
@@ -3179,7 +3179,12 @@ void mpi3mr_os_handle_events(struct mpi3mr_ioc *mrioc,
 	case MPI3_EVENT_DEVICE_INFO_CHANGED:
 	case MPI3_EVENT_LOG_DATA:
 
-		sz = event_reply->event_data_length * 4;
+		if (mrioc->reply_sz > offsetof(struct mpi3_event_notification_reply, event_data))
+			avail_reply_room = mrioc->reply_sz -
+			    offsetof(struct mpi3_event_notification_reply, event_data);
+		else
+			avail_reply_room = 0;
+		sz = min_t(u16, event_reply->event_data_length * 4, avail_reply_room);
 		mpi3mr_app_save_logdata_th(mrioc,
 			(char *)event_reply->event_data, sz);
 		break;
@@ -3213,7 +3218,12 @@ void mpi3mr_os_handle_events(struct mpi3mr_ioc *mrioc,
 		dprint_event_th(mrioc,
 		    "scheduling bottom half handler for event(0x%02x) - (0x%08x), ack_required=%d\n",
 		    evt_type, le32_to_cpu(event_reply->event_context), ack_req);
-		sz = event_reply->event_data_length * 4;
+		if (mrioc->reply_sz > offsetof(struct mpi3_event_notification_reply, event_data))
+			avail_reply_room = mrioc->reply_sz -
+			    offsetof(struct mpi3_event_notification_reply, event_data);
+		else
+			avail_reply_room = 0;
+		sz = min_t(u16, event_reply->event_data_length * 4, avail_reply_room);
 		fwevt = mpi3mr_alloc_fwevt(sz);
 		if (!fwevt) {
 			dprint_event_th(mrioc,
-- 
2.52.0

Re: [PATCH v2 11/17] mpi3mr: Fix out-of-bounds read of event data

[sashiko-bot]

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

New issues:
- [High] Heap out-of-bounds read in bottom-half topology handlers due to newly clamped allocation size without clamped logical array counts.

Pre-existing issues:
- [High] MPI3_EVENT_DEVICE_INFO_CHANGED events are erroneously processed as log data and entirely bypass bottom-half processing.
- [High] Incomplete fix leaves DMA buffer out-of-bounds reads unpatched in top-half topology event handlers.
- [High] Bottom-half event handlers risk out-of-bounds slab reads by blindly casting event data without checking length.
--

commit bbda3bacb81906126bd46f33bc2d5656197ac85e
Author: Chandrakanth Patil <chandrakanth.patil@broadcom.com>

mpi3mr: Fix out-of-bounds read of event data

This commit restricts the size of event data read from the reply buffer to
prevent reading past the end of the frame when caching log data or allocating
the buffer for bottom-half event processing.

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
> index 6a80b784200b3..65a6192cf3960 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_os.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_os.c

[ ... ]

> @@ -3179,7 +3179,12 @@ void mpi3mr_os_handle_events(struct mpi3mr_ioc *mrioc,
>  	case MPI3_EVENT_DEVICE_INFO_CHANGED:
>  	case MPI3_EVENT_LOG_DATA:

[Severity: High]
This is a pre-existing issue, but is it intentional that
MPI3_EVENT_DEVICE_INFO_CHANGED falls through to MPI3_EVENT_LOG_DATA?

When this happens, the payload is passed to mpi3mr_app_save_logdata_th()
and process_evt_bh is not set to 1. This appears to cause the bottom-half
event to never be scheduled, and might overwrite firmware log buffers with
unrelated device information page data.

> -		sz = event_reply->event_data_length * 4;
> +		if (mrioc->reply_sz > offsetof(struct mpi3_event_notification_reply, event_data))
> +			avail_reply_room = mrioc->reply_sz -
> +			    offsetof(struct mpi3_event_notification_reply, event_data);
> +		else
> +			avail_reply_room = 0;
> +		sz = min_t(u16, event_reply->event_data_length * 4, avail_reply_room);
>  		mpi3mr_app_save_logdata_th(mrioc,
>  			(char *)event_reply->event_data, sz);
>  		break;

[Severity: High]
This is a pre-existing issue, but while this fixes the buffer copy size
here, does it leave out-of-bounds reads unpatched in the top-half topology
event handlers?

In mpi3mr_pcietopochg_evt_th(), the bounds checking still uses the
unclamped firmware-provided length directly:

avail_len = event_reply->event_data_length * 4;
...
max_entries = (avail_len - ...) / sizeof(...);

If the event_data_length provided by firmware is larger than the physical
DMA reply frame size, could this lead to reading out of bounds directly
from the DMA mapping during the top-half processing loop?

[ ... ]

> @@ -3213,7 +3218,12 @@ void mpi3mr_os_handle_events(struct mpi3mr_ioc *mrioc,
>  		dprint_event_th(mrioc,
>  		    "scheduling bottom half handler for event(0x%02x) - (0x%08x), ack_required=%d\n",
>  		    evt_type, le32_to_cpu(event_reply->event_context), ack_req);
> -		sz = event_reply->event_data_length * 4;
> +		if (mrioc->reply_sz > offsetof(struct mpi3_event_notification_reply, event_data))
> +			avail_reply_room = mrioc->reply_sz -
> +			    offsetof(struct mpi3_event_notification_reply, event_data);
> +		else
> +			avail_reply_room = 0;
> +		sz = min_t(u16, event_reply->event_data_length * 4, avail_reply_room);

[Severity: High]
Since this patch restricts the allocated size of the heap buffer fwevt by
clamping it to avail_reply_room, could this cause an out-of-bounds read in
the bottom-half handler?

For instance, in mpi3mr_sastopochg_evt_bh(), the loop iterates up to the
unclamped num_entries from the payload. If the allocation was truncated,
iterating up to the original unvalidated num_entries could read past the
end of the newly clamped fwevt heap allocation.

[Severity: High]
This is a pre-existing issue, but could truncating the event data size
cause out-of-bounds slab reads later if the buffer size is exceptionally
small?

If the clamped sz ends up being extremely small or zero, the bottom-half
handlers like mpi3mr_fwevt_bh() blindly cast fwevt->event_data to
fixed-size structures such as struct mpi3_device_page0 and access fields
like persistent_id. Should there be a minimum size check before accessing
these fields in the bottom half?

>  		fwevt = mpi3mr_alloc_fwevt(sz);
>  		if (!fwevt) {
>  			dprint_event_th(mrioc,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825210411.301535-1-chandrakanth.patil@broadcom.com?part=11

[PATCH v2 12/17] mpi3mr: Fix out-of-bounds phy array access on link change

[Chandrakanth Patil]

The phy number that comes with a link change is used to index the
node's phy array without being compared against the number of phys
allocated for that node.

Check it first.

Fixes: 42fc9fee116f ("scsi: mpi3mr: Add helper functions to manage device's port")
Signed-off-by: Chandrakanth Patil <chandrakanth.patil@broadcom.com>
---
v2:
 - No changes from v1.
 drivers/scsi/mpi3mr/mpi3mr_transport.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/scsi/mpi3mr/mpi3mr_transport.c b/drivers/scsi/mpi3mr/mpi3mr_transport.c
index 240f67a8e2e3..2afff1a002ed 100644
--- a/drivers/scsi/mpi3mr/mpi3mr_transport.c
+++ b/drivers/scsi/mpi3mr/mpi3mr_transport.c
@@ -1090,6 +1090,13 @@ void mpi3mr_update_links(struct mpi3mr_ioc *mrioc,
 		return;
 	}
 
+	if (phy_number >= mr_sas_node->num_phys) {
+		spin_unlock_irqrestore(&mrioc->sas_node_lock, flags);
+		ioc_err(mrioc, "%s: phy_number(%d) >= num_phys(%d)\n",
+			__func__, phy_number, mr_sas_node->num_phys);
+		return;
+	}
+
 	mr_sas_phy = &mr_sas_node->phy[phy_number];
 	mr_sas_phy->attached_handle = handle;
 	spin_unlock_irqrestore(&mrioc->sas_node_lock, flags);
-- 
2.52.0

Re: [PATCH v2 12/17] mpi3mr: Fix out-of-bounds phy array access on link change

[sashiko-bot]

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

Pre-existing issues:
- [High] Double-put on `tgtdev` causing a reference count underflow and potential use-after-free.
- [High] Bitwise shift `(1 << i)` is used to construct a `u64 phy_mask`, causing sign-extension and out-of-bounds array access for i >= 31.
- [High] Casting a `u64` bitmask to `(ulong *)` for `for_each_set_bit()` swaps bit mask halves on 32-bit big-endian architectures.
--

commit 0f7fb7e1d3f3dab2157cb8912f1ddaf0e9989416
Author: Chandrakanth Patil <chandrakanth.patil@broadcom.com>

mpi3mr: Fix out-of-bounds phy array access on link change

This commit adds a bounds check in mpi3mr_update_links() to ensure the
phy_number from a link change event does not exceed the number of allocated
phys for the node. This prevents an out-of-bounds array access on the
node's phy array.

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_transport.c b/drivers/scsi/mpi3mr/mpi3mr_transport.c
> index 240f67a8e2e3b..2afff1a002ed9 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_transport.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_transport.c
[ ... ]

[Severity: High]
This is a pre-existing issue, but looking at mpi3mr_sastopochg_evt_bh()
in drivers/scsi/mpi3mr/mpi3mr_os.c, when processing events with a target
not responding reason code:

drivers/scsi/mpi3mr/mpi3mr_os.c:mpi3mr_sastopochg_evt_bh() {
    ...
            mpi3mr_tgtdev_put(tgtdev);
            break;
    ...
        if (tgtdev)
            mpi3mr_tgtdev_put(tgtdev);
}

Does this result in a double decrement of the tgtdev reference count,
causing premature freeing of the target device structure? Similar logic
appears to exist in mpi3mr_pcietopochg_evt_bh() and
mpi3mr_devinfochg_evt_bh().

[Severity: High]
This is a pre-existing issue, but looking at how the phy mask is updated
in mpi3mr_sas_port_add() in drivers/scsi/mpi3mr/mpi3mr_transport.c:

drivers/scsi/mpi3mr/mpi3mr_transport.c:mpi3mr_sas_port_add() {
    ...
            mr_sas_port->phy_mask |= (1 << i);
    ...
}

Since 1 is a signed 32-bit integer, evaluating it up to num_phys (which can
be 32 or greater on enterprise HBAs) could produce a negative value that
sign-extends to 0xFFFFFFFF80000000ULL when implicitly cast to the u64
phy_mask.

Can this cause out-of-bounds array accesses on the phy arrays?

Another instance with the identical signed shift occurs in
mpi3mr_refresh_sas_ports():

drivers/scsi/mpi3mr/mpi3mr_transport.c:mpi3mr_refresh_sas_ports() {
    ...
        h_port[port_idx].phy_mask = (1 << i);
    ...
}

[Severity: High]
This is a pre-existing issue, but looking at how phys_to_be_added is
processed in mpi3mr_update_mr_sas_port():

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)) {
    ...
}

The variable phys_to_be_added is allocated as a 64-bit u64. On 32-bit
big-endian platforms, unsigned long is 4 bytes.

Is it possible that casting the u64 bitmask to (ulong *) for
for_each_set_bit() causes it to process the most significant 32 bits first,
treating bits 32-63 as bits 0-31 and vice versa, leading to incorrect port
mappings?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825210411.301535-1-chandrakanth.patil@broadcom.com?part=12

[PATCH v2 13/17] mpi3mr: Fix buffer overflow in the BSG target device map

[Chandrakanth Patil]

The size of the target device map buffer is held in a u16 while the
number of devices it is derived from is not bounded to fit. With enough
devices the size wraps, a short buffer is allocated, and the loop that
fills it writes past the end.

Do the calculation in size_t.

Fixes: fb428a2005fc ("scsi: mpi3mr: Fix issues in mpi3mr_get_all_tgt_info()")
Signed-off-by: Chandrakanth Patil <chandrakanth.patil@broadcom.com>
---
v2:
 - No changes from v1.
 drivers/scsi/mpi3mr/mpi3mr_app.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/mpi3mr/mpi3mr_app.c b/drivers/scsi/mpi3mr/mpi3mr_app.c
index cd772b2cb98a..f5b48c95cf05 100644
--- a/drivers/scsi/mpi3mr/mpi3mr_app.c
+++ b/drivers/scsi/mpi3mr/mpi3mr_app.c
@@ -1466,7 +1466,8 @@ static long mpi3mr_bsg_pel_enable(struct mpi3mr_ioc *mrioc,
 static long mpi3mr_get_all_tgt_info(struct mpi3mr_ioc *mrioc,
 	struct bsg_job *job)
 {
-	u16 num_devices = 0, i = 0, size;
+	u16 num_devices = 0, i = 0;
+	size_t size;
 	unsigned long flags;
 	struct mpi3mr_tgt_dev *tgtdev;
 	struct mpi3mr_device_map_info *devmap_info = NULL;
@@ -1492,8 +1493,8 @@ static long mpi3mr_get_all_tgt_info(struct mpi3mr_ioc *mrioc,
 		return 0;
 	}
 
-	kern_entrylen = num_devices * sizeof(*devmap_info);
-	size = sizeof(u64) + kern_entrylen;
+	kern_entrylen = (uint32_t)num_devices * sizeof(*devmap_info);
+	size = sizeof(u64) + (size_t)kern_entrylen;
 	alltgt_info = kzalloc(size, GFP_KERNEL);
 	if (!alltgt_info)
 		return -ENOMEM;
-- 
2.52.0

Re: [PATCH v2 13/17] mpi3mr: Fix buffer overflow in the BSG target device map

[sashiko-bot]

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

Pre-existing issues:
- [Medium] Pre-existing integer overflow in `mpi3mr_get_logdata` when calculating total buffer size
- [Medium] Time-of-Check to Time-of-Use (TOCTOU) race condition in `mpi3mr_get_all_tgt_info`
--

commit db64ca74e30e89ca553a673a670db46d0985b8fa
Author: Chandrakanth Patil <chandrakanth.patil@broadcom.com>

    mpi3mr: Fix buffer overflow in the BSG target device map

This commit changes the size variable to size_t in mpi3mr_get_all_tgt_info
to fix a buffer overflow when calculating the target device map buffer size.

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_app.c b/drivers/scsi/mpi3mr/mpi3mr_app.c
> index cd772b2cb98ae..f5b48c95cf055 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_app.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_app.c
> @@ -1466,7 +1466,8 @@ static long mpi3mr_bsg_pel_enable(struct mpi3mr_ioc *mrioc,
>  static long mpi3mr_get_all_tgt_info(struct mpi3mr_ioc *mrioc,
>  	struct bsg_job *job)
>  {
> -	u16 num_devices = 0, i = 0, size;
> +	u16 num_devices = 0, i = 0;
> +	size_t size;
>  	unsigned long flags;
>  	struct mpi3mr_tgt_dev *tgtdev;
>  	struct mpi3mr_device_map_info *devmap_info = NULL;

[ ... ]

> @@ -1492,8 +1493,8 @@ static long mpi3mr_get_all_tgt_info(struct mpi3mr_ioc *mrioc,
>  		return 0;
>  	}

[Severity: Medium]
This is a pre-existing issue, but is there a Time-of-Check to Time-of-Use
(TOCTOU) race condition here?

Right above this block, the code evaluates list_empty(&mrioc->tgtdev_list)
without holding a lock:

	if ((job->request_payload.payload_len <= sizeof(u64)) ||
		list_empty(&mrioc->tgtdev_list)) {
		sg_copy_from_buffer(job->request_payload.sg_list,
				    job->request_payload.sg_cnt,
				    &num_devices, sizeof(num_devices));
		return 0;
	}

If a target device is removed (e.g., hot unplug) right after the tgtdev_lock
is released (where num_devices was counted) but before this list_empty()
check, could we return a non-zero device count to user space without actually
populating the device mapping data array?

This might lead the application to read uninitialized memory from its own
buffer as valid device information.

> -	kern_entrylen = num_devices * sizeof(*devmap_info);
> -	size = sizeof(u64) + kern_entrylen;
> +	kern_entrylen = (uint32_t)num_devices * sizeof(*devmap_info);
> +	size = sizeof(u64) + (size_t)kern_entrylen;
>  	alltgt_info = kzalloc(size, GFP_KERNEL);
>  	if (!alltgt_info)
>  		return -ENOMEM;

[Severity: Medium]
This isn't a bug introduced by this patch, but while reviewing the integer
overflow fix here, I noticed a similar pre-existing issue in
mpi3mr_get_logdata().

In mpi3mr_get_logdata(), the sz variable is declared as u16 and can
overflow when multiplied:

drivers/scsi/mpi3mr/mpi3mr_app.c:mpi3mr_get_logdata() {
    ...
	u16 sz, num_entries, entry_sz;
    ...
	sz = num_entries * entry_sz;
    ...
}

If the firmware advertises a reply_frame_size that makes entry_sz > 163 bytes
(e.g., a standard 256-byte frame), and we request MPI3MR_BSG_LOGDATA_MAX_ENTRIES
(400), could num_entries * entry_sz overflow the 16-bit sz variable?

This could silently truncate the amount of log data copied to user space,
causing the management tool to read uninitialized memory for the remaining
entries.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825210411.301535-1-chandrakanth.patil@broadcom.com?part=13

[PATCH v2 14/17] mpi3mr: Fix out-of-bounds read in PCIe topology change events

[Chandrakanth Patil]

The number of entries in a PCIe topology change event is used to walk
the entry array without being compared against the amount of event
data that was received.

Bound the entry count to the received event data before use.

Fixes: 8e653455547a ("scsi: mpi3mr: Add support for PCIe device event handling")
Signed-off-by: Chandrakanth Patil <chandrakanth.patil@broadcom.com>
---
v2:
 - Cached num_entries in a local variable before bounds check and used it
   for the loop condition to prevent re-fetching from DMA memory (TOCTOU).
 drivers/scsi/mpi3mr/mpi3mr_os.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
index 65a6192cf396..543e9b8e3fc3 100644
--- a/drivers/scsi/mpi3mr/mpi3mr_os.c
+++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
@@ -2761,12 +2761,28 @@ static void mpi3mr_pcietopochg_evt_th(struct mpi3mr_ioc *mrioc,
 	struct mpi3_event_data_pcie_topology_change_list *topo_evt =
 	    (struct mpi3_event_data_pcie_topology_change_list *)event_reply->event_data;
 	int i;
-	u16 handle;
-	u8 reason_code;
+	u16 handle, avail_len;
+	u8 reason_code, max_entries, num_entries;
 	struct mpi3mr_tgt_dev *tgtdev = NULL;
 	struct mpi3mr_stgt_priv_data *scsi_tgt_priv_data = NULL;
 
-	for (i = 0; i < topo_evt->num_entries; i++) {
+	avail_len = event_reply->event_data_length * 4;
+	if (avail_len < offsetof(struct mpi3_event_data_pcie_topology_change_list, port_entry)) {
+		ioc_err(mrioc, "PCIe topology event: event data too small (%u bytes)\n",
+			avail_len);
+		return;
+	}
+	max_entries = (avail_len -
+		      offsetof(struct mpi3_event_data_pcie_topology_change_list, port_entry)) /
+		      sizeof(struct mpi3_event_pcie_topo_port_entry);
+	num_entries = topo_evt->num_entries;
+	if (num_entries > max_entries) {
+		ioc_err(mrioc, "PCIe topology event: num_entries(%d) exceeds max(%d)\n",
+			num_entries, max_entries);
+		return;
+	}
+
+	for (i = 0; i < num_entries; i++) {
 		handle = le16_to_cpu(topo_evt->port_entry[i].attached_dev_handle);
 		if (!handle)
 			continue;
-- 
2.52.0

Re: [PATCH v2 14/17] mpi3mr: Fix out-of-bounds read in PCIe topology change events

[sashiko-bot]

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

Pre-existing issues:
- [High] The bounds check blindly trusts `event_data_length` provided by the hardware without clamping it to the physical size of the DMA reply buffer, failing to prevent out-of-bounds reads in the top-half handlers.
- [High] The patch adds bounds checking to the top-half handler, but fails to prevent the bottom-half handler from being scheduled and iterating out-of-bounds over the same malformed event data.
- [High] The compiler-inserted bounds checks from the `__counted_by` attribute bypass the local variable cache, re-introducing a TOCTOU vulnerability.
- [High] Lockless access to `tgtdev->starget` and `starget->hostdata` leads to a Use-After-Free vulnerability.
--

commit 63bdf6fdca71d0debbf3d9facb074c2cc5e861c5
Author: Chandrakanth Patil <chandrakanth.patil@broadcom.com>

mpi3mr: Fix out-of-bounds read in PCIe topology change events

This commit bounds the entry count to the received event data before use to
prevent out-of-bounds reads when processing PCIe topology change events.

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
> index 65a6192cf3960..543e9b8e3fc37 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_os.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
> @@ -2761,12 +2761,28 @@ static void mpi3mr_pcietopochg_evt_th(struct mpi3mr_ioc *mrioc,
>  	struct mpi3_event_data_pcie_topology_change_list *topo_evt =
>  	    (struct mpi3_event_data_pcie_topology_change_list *)event_reply->event_data;
>  	int i;
> -	u16 handle;
> -	u8 reason_code;
> +	u16 handle, avail_len;
> +	u8 reason_code, max_entries, num_entries;
>  	struct mpi3mr_tgt_dev *tgtdev = NULL;
>  	struct mpi3mr_stgt_priv_data *scsi_tgt_priv_data = NULL;
>  
> -	for (i = 0; i < topo_evt->num_entries; i++) {
> +	avail_len = event_reply->event_data_length * 4;

[Severity: High]
This is a pre-existing issue, but the patch provides an incomplete fix. Does
this code blindly trust event_data_length provided by the hardware without
clamping it to the physical size of the DMA reply buffer?

If a malicious or buggy hardware controller sends a PCIe topology change
event with a large event_data_length (e.g., 255) and a large num_entries,
avail_len could become up to 1020, exceeding the actual physical buffer size
(mrioc->reply_sz, typically 128 bytes).

Since avail_len is not clamped, max_entries would become artificially large,
bypassing the loop bounds check. Could this result in an out-of-bounds read
of kernel memory adjacent to the DMA reply buffer?

> +	if (avail_len < offsetof(struct mpi3_event_data_pcie_topology_change_list, port_entry)) {
> +		ioc_err(mrioc, "PCIe topology event: event data too small (%u bytes)\n",
> +			avail_len);
> +		return;
> +	}
> +	max_entries = (avail_len -
> +		      offsetof(struct mpi3_event_data_pcie_topology_change_list, port_entry)) /
> +		      sizeof(struct mpi3_event_pcie_topo_port_entry);
> +	num_entries = topo_evt->num_entries;
> +	if (num_entries > max_entries) {
> +		ioc_err(mrioc, "PCIe topology event: num_entries(%d) exceeds max(%d)\n",
> +			num_entries, max_entries);
> +		return;

[Severity: High]
This isn't a bug introduced by this patch, but the patch's attempt to fix the
bounds check might be bypassed in the bottom half. If the top-half handler
returns early here, what happens to process_evt_bh which was set to 1 prior
to this call by mpi3mr_os_handle_events()?

If process_evt_bh is not cleared, wouldn't the bottom-half handler
mpi3mr_pcietopochg_evt_bh() still be scheduled and iterate out-of-bounds over
the same malformed event data?

mpi3mr_pcietopochg_evt_bh():
	for (i = 0; i < event_data->num_entries; i++) {

> +	}
> +
> +	for (i = 0; i < num_entries; i++) {
>  		handle = le16_to_cpu(topo_evt->port_entry[i].attached_dev_handle);

[Severity: High]
This isn't a bug introduced by this patch, but the patch's attempt to fix the
TOCTOU vulnerability might be nullified. Even though num_entries is cached in
a local variable, doesn't accessing topo_evt->port_entry[i] trigger a
compiler-inserted bounds check because of the __counted_by(num_entries)
attribute in the structure definition?

Under CONFIG_UBSAN_BOUNDS, this would force the compiler to re-read
topo_evt->num_entries directly from DMA memory to perform the bounds check.

If a malicious device modifies this value post-validation, could it trigger
a bounds check failure and panic the kernel?

>  		if (!handle)
>  			continue;
>  		reason_code = topo_evt->port_entry[i].port_status;
>  		scsi_tgt_priv_data =  NULL;
>  		tgtdev = mpi3mr_get_tgtdev_by_handle(mrioc, handle);
>  		if (tgtdev && tgtdev->starget && tgtdev->starget->hostdata)

[Severity: High]
This is a pre-existing issue that was not addressed, but is it safe to access
tgtdev->starget and starget->hostdata locklessly here?

If mpi3mr_target_destroy() executes concurrently, it can take the lock, set
tgtdev->starget = NULL, release the lock, and call kfree(starget->hostdata).

Could the top-half handler race with this destruction and lead to a
Use-After-Free when dereferencing scsi_tgt_priv_data immediately afterwards?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825210411.301535-1-chandrakanth.patil@broadcom.com?part=14

[PATCH v2 15/17] mpi3mr: zero out diagnostic buffer status memory

[Chandrakanth Patil]

Memory allocated for the BSG diagnostic buffer status is not zero'd
before it is copied back to user space. Several fields in this
structure are reserved and never written by the driver. Use kzalloc
instead of kmalloc to zero out the allocated memory.

Fixes: 78b506984ebe ("scsi: mpi3mr: Add ioctl support for HDB")
Signed-off-by: Chandrakanth Patil <chandrakanth.patil@broadcom.com>
---
v2:
 - No changes from v1.
 drivers/scsi/mpi3mr/mpi3mr_app.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/mpi3mr/mpi3mr_app.c b/drivers/scsi/mpi3mr/mpi3mr_app.c
index f5b48c95cf05..0cdcb8d236d3 100644
--- a/drivers/scsi/mpi3mr/mpi3mr_app.c
+++ b/drivers/scsi/mpi3mr/mpi3mr_app.c
@@ -1244,7 +1244,7 @@ static long mpi3mr_bsg_query_hdb(struct mpi3mr_ioc *mrioc,
 
 	length = (sizeof(*hbd_status) + ((MPI3MR_MAX_NUM_HDB - 1) *
 		    sizeof(*hbd_status_entry)));
-	hbd_status = kmalloc(length, GFP_KERNEL);
+	hbd_status = kzalloc(length, GFP_KERNEL);
 	if (!hbd_status)
 		return -ENOMEM;
 	hbd_status_entry = &hbd_status->entry[0];
-- 
2.52.0

[PATCH v2 16/17] mpi3mr: Fix use-after-free of the firmware event workqueue

[Chandrakanth Patil]

The enqueue path reads the workqueue pointer before taking the event
lock, while the remove path clears that pointer and destroys the
workqueue while holding it. The enqueue can therefore reach
queue_work() after the workqueue is gone.

Move the check inside the lock.

Fixes: 13ef29ea4aa0 ("scsi: mpi3mr: Add support for device add/remove event handling")
Signed-off-by: Chandrakanth Patil <chandrakanth.patil@broadcom.com>
---
v2:
 - No changes from v1.
 drivers/scsi/mpi3mr/mpi3mr_os.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
index 543e9b8e3fc3..e97e343b14fa 100644
--- a/drivers/scsi/mpi3mr/mpi3mr_os.c
+++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
@@ -233,10 +233,11 @@ static void mpi3mr_fwevt_add_to_list(struct mpi3mr_ioc *mrioc,
 {
 	unsigned long flags;
 
-	if (!mrioc->fwevt_worker_thread)
-		return;
-
 	spin_lock_irqsave(&mrioc->fwevt_lock, flags);
+	if (!mrioc->fwevt_worker_thread) {
+		spin_unlock_irqrestore(&mrioc->fwevt_lock, flags);
+		return;
+	}
 	/* get fwevt reference count while adding it to fwevt_list */
 	mpi3mr_fwevt_get(fwevt);
 	INIT_LIST_HEAD(&fwevt->list);
-- 
2.52.0

Re: [PATCH v2 16/17] mpi3mr: Fix use-after-free of the firmware event workqueue

[sashiko-bot]

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

Pre-existing issues:
- [High] Use-after-free in `mpi3mr_cancel_work` when canceling concurrently executing firmware event work.
- [High] Data race leading to Use-After-Free of `mrioc->current_event` during firmware event cleanup.
--

commit c5c6dc4f18099cddf01a3785308a8b8fac21872d
Author: Chandrakanth Patil <chandrakanth.patil@broadcom.com>

mpi3mr: Fix use-after-free of the firmware event workqueue
    
The enqueue path reads the workqueue pointer before taking the event
lock, while the remove path clears that pointer and destroys the
workqueue while holding it. The enqueue can therefore reach
queue_work() after the workqueue is gone.
    
Move the check inside the lock.

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
> index 543e9b8e3fc37..e97e343b14fa5 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_os.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
> @@ -233,10 +233,11 @@ static void mpi3mr_fwevt_add_to_list(struct mpi3mr_ioc *mrioc,
>  {
>  	unsigned long flags;
>  
> -	if (!mrioc->fwevt_worker_thread)
> -		return;
> -
>  	spin_lock_irqsave(&mrioc->fwevt_lock, flags);
> +	if (!mrioc->fwevt_worker_thread) {
> +		spin_unlock_irqrestore(&mrioc->fwevt_lock, flags);
> +		return;
> +	}
>  	/* get fwevt reference count while adding it to fwevt_list */
>  	mpi3mr_fwevt_get(fwevt);
>  	INIT_LIST_HEAD(&fwevt->list);

[Severity: High]
This isn't a bug introduced by this patch, but looking at how firmware events
are managed and cleaned up, is there a use-after-free risk during cancellation?

When mpi3mr_cleanup_fwevt_list() flushes pending events, it removes the event
from the list and immediately drops its list reference:

drivers/scsi/mpi3mr/mpi3mr_os.c:mpi3mr_dequeue_fwevt() {
    ...
    list_del_init(&fwevt->list);
    /*
     * Put fwevt reference count after
     * removing it from fwevt_list
     */
    mpi3mr_fwevt_put(fwevt);
    ...
}

It then calls mpi3mr_cancel_work(fwevt), which invokes
cancel_work_sync(&fwevt->work). If the work is already executing but hasn't yet
removed itself from the list in mpi3mr_fwevt_bh(), cancel_work_sync() will
block waiting for the worker thread.

When the worker thread completes, it drops all remaining references and frees
the event memory:

drivers/scsi/mpi3mr/mpi3mr_os.c:mpi3mr_fwevt_worker() {
    ...
    mpi3mr_fwevt_bh(fwevt->mrioc, fwevt);
    /*
     * Put fwevt reference count after
     * dequeuing it from worker queue
     */
    mpi3mr_fwevt_put(fwevt);
}

Does this leave cancel_work_sync() operating on a freed work_struct?


[Severity: High]
This is a pre-existing issue, but there appears to be a data race when accessing
mrioc->current_event in the cleanup path.

In mpi3mr_cleanup_fwevt_list(), after draining the list, the current_event
pointer is accessed without holding a lock or taking a reference:

drivers/scsi/mpi3mr/mpi3mr_os.c:mpi3mr_cleanup_fwevt_list() {
    ...
    if (mrioc->current_event) {
        fwevt = mrioc->current_event;
        ...
        if (current_work() == &fwevt->work || fwevt->pending_at_sml) {
    ...
}

Concurrently, if the worker thread is finishing execution in mpi3mr_fwevt_bh(),
it drops the reference and sets the pointer to NULL:

    mpi3mr_fwevt_put(fwevt);
    mrioc->current_event = NULL;

If the cleanup thread reads mrioc->current_event before it is set to NULL, and
the worker thread completes and drops its final reference (freeing the event),
could the cleanup thread dereference freed memory when evaluating
fwevt->pending_at_sml or calling mpi3mr_cancel_work(fwevt)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825210411.301535-1-chandrakanth.patil@broadcom.com?part=16

[PATCH v2 17/17] mpi3mr: Fix NULL pointer dereference on PCI error recovery

[Chandrakanth Patil]

On a frozen PCI channel the driver unmaps the register window and frees
the interrupts, but leaves the firmware event workqueue running. An
event handler that is already in flight can still reach the register
window after it has been unmapped.

Stop event processing and drain the queue first, and start it again
when the channel resumes.

Fixes: 30bafe1774f0 ("scsi: mpi3mr: Support PCI Error Recovery callback handlers")
Signed-off-by: Chandrakanth Patil <chandrakanth.patil@broadcom.com>
---
v2:
 - Removed stop_drv_processing and mpi3mr_cleanup_fwevt_list() calls from
   pci_channel_io_frozen to avoid workqueue cancellation deadlocks and
   preserve transparent I/O recovery (SCSI_MLQUEUE_HOST_BUSY).
 - Added pci_err_recovery check in mpi3mr_fwevt_bh() to safely skip bottom-half
   event handling during PCI error recovery.
 drivers/scsi/mpi3mr/mpi3mr_os.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
index e97e343b14fa..a389f7f2c741 100644
--- a/drivers/scsi/mpi3mr/mpi3mr_os.c
+++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
@@ -2130,9 +2130,11 @@ static void mpi3mr_fwevt_bh(struct mpi3mr_ioc *mrioc,
 	mpi3mr_fwevt_del_from_list(mrioc, fwevt);
 	mrioc->current_event = fwevt;
 
-	if (mrioc->stop_drv_processing) {
-		dprint_event_bh(mrioc, "ignoring event(0x%02x) in the bottom half handler\n"
-				"due to stop_drv_processing\n", fwevt->event_id);
+	if (mrioc->stop_drv_processing || mrioc->pci_err_recovery) {
+		dprint_event_bh(mrioc,
+				"ignoring event(0x%02x) in the bottom half handler\n"
+				"due to stop_drv_processing or pci_err_recovery\n",
+				fwevt->event_id);
 		goto out;
 	}
 
-- 
2.52.0

Re: [PATCH v2 17/17] mpi3mr: Fix NULL pointer dereference on PCI error recovery

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Concurrent execution between `mpi3mr_cleanup_fwevt_list()` and the firmware event worker thread results in a Use-After-Free of the `mrioc->current_event` pointer.
- [High] The patch fails to resolve the 'already in flight' race condition described in its commit message, leaving the driver vulnerable to NULL pointer dereferences via a Time-Of-Check to Time-Of-Use (TOCTOU) race.
--

commit 67ebeaa54b154ad043a90c5a2e98345f3782e9c3
Author: Chandrakanth Patil <chandrakanth.patil@broadcom.com>

mpi3mr: Fix NULL pointer dereference on PCI error recovery

This patch attempts to fix a race condition on a frozen PCI channel by
preventing in-flight firmware event handlers from accessing unmapped
register windows. It adds a check for pci_err_recovery early in the
bottom-half handler to skip event processing during recovery.

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
> index e97e343b14fa5..a389f7f2c741e 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_os.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
> @@ -2130,9 +2130,11 @@ static void mpi3mr_fwevt_bh(struct mpi3mr_ioc *mrioc,
>  	mpi3mr_fwevt_del_from_list(mrioc, fwevt);
>  	mrioc->current_event = fwevt;

[Severity: Critical]
This is a pre-existing issue, but does assigning mrioc->current_event here
lead to a use-after-free during driver teardown or reset?

If mpi3mr_cleanup_fwevt_list() is executing concurrently, it performs a
lockless read of mrioc->current_event:

drivers/scsi/mpi3mr/mpi3mr_os.c:mpi3mr_cleanup_fwevt_list() {
        ...
        fwevt = mrioc->current_event;
        ...
}

At the end of mpi3mr_fwevt_bh(), the worker thread drops the reference and
NULLs the pointer:

drivers/scsi/mpi3mr/mpi3mr_os.c:mpi3mr_fwevt_bh() {
        ...
out:
        mpi3mr_fwevt_put(fwevt);
        mrioc->current_event = NULL;
}

If mpi3mr_cleanup_fwevt_list() captures the pointer before the worker thread
NULLs it, but accesses it after mpi3mr_fwevt_put() frees the memory, could
this corrupt memory when interacting with the freed event's work struct?

>  
> -	if (mrioc->stop_drv_processing) {
> -		dprint_event_bh(mrioc, "ignoring event(0x%02x) in the bottom half handler\n"
> -				"due to stop_drv_processing\n", fwevt->event_id);
> +	if (mrioc->stop_drv_processing || mrioc->pci_err_recovery) {

[Severity: High]
This isn't a bug introduced by this patch, but does this lockless check fully
resolve the race condition described in the commit message?

Since this check is evaluated early in the handler, could an in-flight worker
pass this check, get preempted, and then attempt to access hardware registers
concurrently with mpi3mr_error_detected() unmapping and NULLing sysif_regs?

For example, if the worker proceeds to mpi3mr_admin_request_post():

drivers/scsi/mpi3mr/mpi3mr_fw.c:mpi3mr_admin_request_post() {
        ...
        writel(mrioc->admin_req_pi, &mrioc->sysif_regs->admin_request_queue_pi);
        ...
}

Would this still result in a NULL pointer dereference or unmapped memory
access if the PCI error recovery path clears mrioc->sysif_regs after the
boolean check in mpi3mr_fwevt_bh() has already passed?

> +		dprint_event_bh(mrioc,
> +				"ignoring event(0x%02x) in the bottom half handler\n"
> +				"due to stop_drv_processing or pci_err_recovery\n",
> +				fwevt->event_id);
>  		goto out;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825210411.301535-1-chandrakanth.patil@broadcom.com?part=17