[PATCH 0/17] mpi3mr: fix out-of-bounds accesses and reference leaks

[PATCH 0/17] 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

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        |  9 +++--
 drivers/scsi/mpi3mr/mpi3mr_os.c        | 76 +++++++++++++++++++++++++++++++++++++++-----
 drivers/scsi/mpi3mr/mpi3mr_transport.c |  7 ++++
 4 files changed, 111 insertions(+), 24 deletions(-)

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

Re: [PATCH 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:
- [Critical] Pointer arithmetic wrap-around on 32-bit architectures allows bypassing bounds checks, leading to arbitrary kernel memory read/write via DMA.
- [High] DMA coherent memory allocated for NVMe PRPs is leaked on BSG command timeout or submission failure.
- [High] Target device `block_io` atomic counter is leaked if BSG command submission fails.
- [High] Lockless TOCTOU check of `MPI3MR_CMD_COMPLETE` causes a Use-After-Free of `sense_buff_k` and data races on `reply` buffers.
- [High] Missing validation of the BSG request length (`job->request_len`) allows a user to send an undersized request buffer, leading to out-of-bounds memory reads and kernel panics.
--

--- Patch [1]: [PATCH 01/17] mpi3mr: Fix buffer overflow in BSG passthrough request copy ---
commit ed6513158d24e842e223917348ef598d4d161609
Author: Chandrakanth Patil <chandrakanth.patil@broadcom.com>
Subject: mpi3mr: Fix buffer overflow in BSG passthrough request copy

This commit fixes a buffer overflow in the BSG passthrough request copy.
The check for an incoming BSG request size used a narrower variable than
its source field, allowing large values to wrap and pass the check.
The variable is widened to ensure the full size is validated properly.

> 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;
> @@ -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: Critical]
This is a pre-existing issue, but can the pointer arithmetic nearby lead to
an integer wrap-around on 32-bit architectures?

When iterating over buffer entries in the same function:

	case MPI3MR_BSG_BUFTYPE_DATA_OUT:
		sgl_iter = sgl_dout_iter;
		sgl_dout_iter += buf_entries->buf_len;

If a user submits a BSG request with a crafted buf_len, does the u32 value wrap
the sgl_dout_iter pointer on 32-bit systems? This wrapped pointer could then
bypass the later bounds check:

	if (sgl_dout_iter > (dout_buf + job->request_payload.payload_len)) {

and allow arbitrary kernel memory access during subsequent payload processing.

While reviewing this patch, I noticed a few other pre-existing issues.

> drivers/scsi/mpi3mr/mpi3mr_app.c:mpi3mr_bsg_request()
> {
> 	long rval = -EINVAL;
> 	unsigned int reply_payload_rcv_len = 0;
> 
> 	struct mpi3mr_bsg_packet *bsg_req = job->request;
> 
> 	switch (bsg_req->cmd_type) {

[Severity: High]
This isn't a bug introduced by this patch, but does this function properly
validate job->request_len before dereferencing bsg_req->cmd_type?

If userspace submits a BSG request with a request_len of zero, could this
trigger a zero size pointer dereference and cause a kernel panic? Similarly,
if the length is too small, would this read uninitialized heap memory?

> drivers/scsi/mpi3mr/mpi3mr_app.c:mpi3mr_bsg_process_mpt_cmds()
> {
[ ... ]
> 	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);
> 			mpi3mr_tgtdev_put(tgtdev);
> 		}
> 	}
[ ... ]
> 	rval = mpi3mr_admin_request_post(mrioc, mpi_req,
> 	    MPI3MR_ADMIN_REQ_FRAME_SZ, 0);
> 
> 	if (rval) {
> 		mrioc->bsg_cmds.is_waiting = 0;
> 		dprint_bsg_err(mrioc,
> 		    "%s: posting bsg request is failed\n", __func__);
> 		rval = -EAGAIN;
> 		goto out_unlock;
> 	}

[Severity: High]
This is a pre-existing issue, but does this error path permanently leak the
block_io atomic counter?

If mpi3mr_admin_request_post() fails, the goto out_unlock bypasses the
atomic_dec(&stgt_priv->block_io) that occurs after the completion timeout
logic. Does this leave the SCSI target device blocked indefinitely?

[Severity: High]
Similarly, this is a pre-existing issue, but does the same goto out_unlock
skip freeing the DMA memory allocated for NVMe PRPs?

If mpi3mr_build_nvme_prp() successfully allocated DMA memory in
mrioc->prp_list_virt earlier in this function, the jump to out_unlock skips
the success-path cleanup block:

	if (mrioc->prp_list_virt) {
		dma_free_coherent(&mrioc->pdev->dev, mrioc->prp_sz,
		    mrioc->prp_list_virt, mrioc->prp_list_dma);

Would subsequent commands overwrite mrioc->prp_list_virt and permanently
leak the previous DMA coherent allocation?

> drivers/scsi/mpi3mr/mpi3mr_fw.c:mpi3mr_process_admin_reply_desc()
> {
[ ... ]
> 	cmdptr = mpi3mr_get_drv_cmd(mrioc, host_tag, def_reply);
> 	if (cmdptr) {
> 		if (cmdptr->state & MPI3MR_CMD_PENDING) {
> 			cmdptr->state |= MPI3MR_CMD_COMPLETE;
> 			cmdptr->ioc_loginfo = ioc_loginfo;
[ ... ]
> 			if (sense_buf && cmdptr->sensebuf) {
> 				cmdptr->is_sense = 1;
> 				memcpy(cmdptr->sensebuf, sense_buf,
> 				       MPI3MR_SENSE_BUF_SZ);
> 			}

[Severity: High]
This isn't a bug introduced by this patch, but does setting the
MPI3MR_CMD_COMPLETE flag before the memory copy create a race condition?

If the application thread in mpi3mr_bsg_process_mpt_cmds() wakes up from a
natural timeout exactly when this flag is set, it performs a lockless check:

	if (!(mrioc->bsg_cmds.state & MPI3MR_CMD_COMPLETE)) {

Could the application thread assume the handler has fully finished, proceed
to read uninitialized reply memory, and free sense_buff_k before the interrupt
handler finishes copying into it? Would this result in a use-after-free when
the interrupt handler eventually executes the memcpy?

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

[PATCH 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>
---
 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:

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

[sashiko-bot]

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

Pre-existing issues:
- [Critical] On 32-bit architectures, pointer wrap-around and integer truncation bypass buffer bounds checks, leading to arbitrary kernel memory read/write.
- [Critical] The driver fails to validate the user-provided `job->request_len` before casting and parsing the BSG request packet, leading to an out-of-bounds heap read and a kernel panic.
- [High] Failure to decrement `stgt_priv->block_io` on early failure of `mpi3mr_admin_request_post`.
- [High] DMA coherent memory allocated for PRP lists is leaked on command timeout or post failure.
--

--- Patch [2]: [PATCH 02/17] mpi3mr: Fix out-of-bounds read when copying BSG MPI requests ---
commit 0259dbaa420364e7270c5c71574f90668291edf7
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.

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

[Severity: Critical]
This is a pre-existing issue, but can pointer arithmetic wrap around on 32-bit
architectures and bypass this new bounds check?

If a userspace application sends an MPI3MR_MPT_CMD BSG request with a crafted
buf_len (e.g., 0x10001000) such that the sum modulo 2^32 remains small, the
sgl_iter pointer wrap-around could mathematically bypass both the end-of-buffer
limit check and this newly added check.

Concurrently, can kern_buf_len / MPI3MR_IOCTL_SGE_SIZE truncate from 0x10001
to 1 in the u16 needed_desc variable in mpi3mr_map_data_buffer_dma()?

This would bypass the descriptor limits, allowing the driver to create DMA
mappings for arbitrary kernel addresses.

> +				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:

[Severity: Critical]
This is a pre-existing issue, but does the driver validate job->request_len
before casting and parsing the BSG request packet?

If a userspace application submits a BSG request with request_len set to 0,
the BSG core invokes memdup_user(), which returns ZERO_SIZE_PTR (0x10).

Will the driver unconditionally dereference this pointer here?

drivers/scsi/mpi3mr/mpi3mr_app.c:mpi3mr_bsg_request() {
	...
	switch (bsg_req->cmd_type) {
	...
}

Could this lead to an immediate unhandled kernel panic?  Additionally, if a
small non-zero length is provided, does the driver read bufcnt from the
unvalidated payload in mpi3mr_bsg_process_mpt_cmds() and loop over unallocated
heap memory?

[Severity: High]
This is a pre-existing issue, but is there a missing decrement of
stgt_priv->block_io on early failure of mpi3mr_admin_request_post()?

When processing a SCSI task management request that specifies blocking I/O,
stgt_priv->block_io is incremented. If mpi3mr_admin_request_post() fails,
the code jumps directly to out_unlock:

drivers/scsi/mpi3mr/mpi3mr_app.c:mpi3mr_bsg_process_mpt_cmds() {
	...
		goto out_unlock;
	...
}

Does this skip the atomic_dec(&stgt_priv->block_io) matching the earlier
increment, permanently blocking all future I/O to the affected SCSI target?

[Severity: High]
This is a pre-existing issue, but is DMA coherent memory allocated for PRP
lists leaked on command timeout or post failure?

When an NVMe encapsulated request utilizes a PRP list, mpi3mr_build_nvme_prp()
allocates a DMA coherent buffer in mrioc->prp_list_virt.

If mpi3mr_admin_request_post() fails, or if wait_for_completion_timeout()
times out, the code branches to out_unlock without freeing the memory:

drivers/scsi/mpi3mr/mpi3mr_app.c:mpi3mr_bsg_process_mpt_cmds() {
	...
	if (mrioc->prp_list_virt) {
		dma_free_coherent(&mrioc->pdev->dev, mrioc->prp_sz,
	...
}

Would a subsequent NVMe command blindly overwrite mrioc->prp_list_virt with a
new allocation, permanently leaking the previous DMA buffer?

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

[PATCH 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>
---
 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,

Re: [PATCH 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 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Dereferencing `stgt_priv` on the error and timeout/completion paths leads to a Use-After-Free if the SCSI target is concurrently removed.
--

--- Patch [3]: [PATCH 03/17] mpi3mr: Fix I/O block counter leak on admin request post failure ---
commit 1533f8fc86e8eb7e7a5042ad5aab564671eb7b8c
Author: Chandrakanth Patil <chandrakanth.patil@broadcom.com>

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

This commit addresses an issue where the per-device I/O block counter is not
decremented if posting a task management request fails. It lowers the counter
on the failure path to prevent I/O from remaining blocked.

> 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: Critical]
This is a pre-existing issue, but does this add another path that can trigger
a use-after-free if the SCSI target is concurrently removed?

Looking at mpi3mr_bsg_process_mpt_cmds(), stgt_priv is fetched from
tgtdev->starget->hostdata, but the tgtdev reference is immediately dropped
before posting the bsg request:

	if (block_io) {
		tgtdev = mpi3mr_get_tgtdev_by_handle(mrioc, dev_handle);
		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);
		}
	}

Because there are no references or locks held, if the device is concurrently
removed (e.g., via hot-unplug), the SCSI midlayer can invoke
mpi3mr_target_destroy(), which unconditionally frees the hostdata memory:

mpi3mr_target_destroy() {
	...
	kfree(starget->hostdata);
	...
}

Since the original completion path also dereferences stgt_priv after sleeping
in wait_for_completion_timeout(), both the timeout/completion path and this
newly added error path seem to potentially call
atomic_dec(&stgt_priv->block_io) on freed memory.

>  		goto out_unlock;
>  	}
>  	wait_for_completion_timeout(&mrioc->bsg_cmds.done,

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

[PATCH 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>
---
 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);
 		}
 	}

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

[sashiko-bot]

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

Pre-existing issues:
- [High] Use-after-free of SCSI target private data (`stgt_priv`) during completion of BSG task management commands if the device is hot-removed.
- [High] Unprotected lockless access to `tgtdev->starget` can lead to a NULL pointer dereference or Use-After-Free.
- [High] Missing bounds validation on the BSG `request_len` allows an Out-of-Bounds read when accessing the variable-length buffer entry list.
- [High] Little-endian `__le16` fields from the user request are assigned to CPU-endian `u16` variables without `le16_to_cpu()` conversion.
--

--- Patch [4]: [PATCH 04/17] mpi3mr: Fix target device reference leak in BSG task management ---
(Note: The format of this review has been altered to a free-form summary due
to automated recitation restrictions. The original patch diff is not quoted.)

commit c103d1267a7a6a9328e7aab3dbe852bbbb68d3e6
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 during BSG task
management command processing. The reference is now dropped whenever the
lookup succeeds, regardless of whether the SCSI target data is fully setup.

[Severity: High]
This isn't a bug introduced by this patch, but there appears to be a use
after free risk regarding the SCSI target private data. In
mpi3mr_bsg_process_mpt_cmds(), the block_io counter is incremented on
stgt_priv, and then the target device reference is dropped before sleeping
via wait_for_completion_timeout(). If the target is hot-removed during this
sleep, mpi3mr_target_destroy() will free starget->hostdata. Upon waking,
does the atomic_dec() on stgt_priv->block_io write to freed memory?

[Severity: High]
This is a pre-existing issue, but the access to the target device's starget
pointer seems unprotected. In mpi3mr_bsg_process_mpt_cmds(), tgtdev->starget
is checked for NULL and then dereferenced locklessly to access hostdata.
Since mpi3mr_target_destroy() can concurrently set tgtdev->starget to NULL
under the tgtdev_lock, could this result in a NULL pointer dereference if
the compiler emits multiple loads and the pointer becomes NULL between the
check and the dereference?

[Severity: High]
This isn't a bug introduced by this patch, but there might be an out of
bounds read when iterating over the BSG buffer entries. In
mpi3mr_bsg_process_mpt_cmds(), the loop reads buf_type and buf_len from the
buf_entries list. Is there any validation that the number of entries
provided by the user does not exceed the bounds of the allocated
job->request_len? Could a small request length with a large entry count read
arbitrary slab memory?

[Severity: High]
This is a pre-existing issue, but is there a missing endianness conversion
when parsing the device handle? In mpi3mr_bsg_process_mpt_cmds(),
tm_req->dev_handle is an __le16 field, but it is assigned directly to a u16
local variable without le16_to_cpu(). Similarly,
mpi_header->function_dependent is passed directly to mpi3mr_issue_tm(). On
big-endian architectures, will this block IO on the wrong device and fail
to block IO on the actual target of the task management command?

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

[PATCH 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>
---
 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);

[PATCH 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>
---
 drivers/scsi/mpi3mr/mpi3mr_fw.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/mpi3mr/mpi3mr_fw.c b/drivers/scsi/mpi3mr/mpi3mr_fw.c
index 681868716ebd..5e61448d8dbc 100644
--- a/drivers/scsi/mpi3mr/mpi3mr_fw.c
+++ b/drivers/scsi/mpi3mr/mpi3mr_fw.c
@@ -128,7 +128,8 @@ void *mpi3mr_get_reply_virt_addr(struct mpi3mr_ioc *mrioc,
 		return NULL;
 
 	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) ||
+	    ((phys_addr - mrioc->reply_buf_dma) % mrioc->reply_sz))
 		return NULL;
 
 	return mrioc->reply_buf + (phys_addr - mrioc->reply_buf_dma);

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

[sashiko-bot]

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

New issues:
- [Low] The newly added modulo operation `((phys_addr - mrioc->reply_buf_dma) % mrioc->reply_sz)` implicitly performs a 64-bit division, causing a build failure (linker error) on 32-bit platforms where `dma_addr_t` is 64-bit.

Pre-existing issues:
- [High] An integer overflow in the reply pool size calculation allows a malicious or compromised device to trigger a massive out-of-bounds DMA write, leading to host memory corruption.
--

--- Patch [6]: [PATCH 06/17] mpi3mr: Fix out-of-bounds reply frame access ---
commit 144f6c8866bb0bb9ede9699c897280faf1028d42
Author: Chandrakanth Patil <chandrakanth.patil@broadcom.com>

mpi3mr: Fix out-of-bounds reply frame access

This commit tightens the validation of reply frame addresses by requiring
the address to be frame-aligned and ensuring a whole frame fits within the
pool.

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_fw.c b/drivers/scsi/mpi3mr/mpi3mr_fw.c
> index 681868716ebdb..5e61448d8dbc6 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_fw.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_fw.c
> @@ -128,7 +128,8 @@ void *mpi3mr_get_reply_virt_addr(struct mpi3mr_ioc *mrioc,
>  		return NULL;
>  
>  	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) ||
> +	    ((phys_addr - mrioc->reply_buf_dma) % mrioc->reply_sz))

[Severity: Low]
Will this modulo operation cause a linker error on 32-bit platforms?

Since dma_addr_t can be 64-bit on 32-bit architectures (like x86 PAE), using
the modulo operator on it might implicitly perform a 64-bit division. This
typically results in an undefined reference to __umoddi3 during the build.

Could this be avoided by using explicit division macros like div64_u64_rem()
or similar?

>  		return NULL;
>  
>  	return mrioc->reply_buf + (phys_addr - mrioc->reply_buf_dma);

[Severity: High]
This is a pre-existing issue, but does the reply pool size calculation in
mpi3mr_alloc_reply_sense_bufs() suffer from a 32-bit integer overflow?

If a device provides crafted values for maximum outstanding requests and
reply frame size, this multiplication might overflow:

drivers/scsi/mpi3mr/mpi3mr_fw.c:mpi3mr_alloc_reply_sense_bufs() {
    ...
    u32 sz;
    ...
    sz = mrioc->num_reply_bufs * mrioc->reply_sz;
    ...
}

If this overflows, an undersized DMA pool would be created. Later, in
mpimr_initialize_reply_sbuf_queues(), the driver loops over the expected
number of buffers and writes out-of-bounds DMA addresses to the hardware:

drivers/scsi/mpi3mr/mpi3mr_fw.c:mpimr_initialize_reply_sbuf_queues() {
    ...
    for (i = 0, phy_addr = mrioc->reply_buf_dma;
         i < mrioc->num_reply_bufs; i++, phy_addr += mrioc->reply_sz)
        mrioc->reply_free_q[i] = cpu_to_le64(phy_addr);
    ...
}

Could this allow a malicious or compromised device to trigger out-of-bounds
DMA writes into host memory?

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

[PATCH 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>
---
 drivers/scsi/mpi3mr/mpi3mr_fw.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/scsi/mpi3mr/mpi3mr_fw.c b/drivers/scsi/mpi3mr/mpi3mr_fw.c
index 5e61448d8dbc..103ce9dd2266 100644
--- a/drivers/scsi/mpi3mr/mpi3mr_fw.c
+++ b/drivers/scsi/mpi3mr/mpi3mr_fw.c
@@ -141,6 +141,12 @@ void *mpi3mr_get_sensebuf_virt_addr(struct mpi3mr_ioc *mrioc,
 	if (!phys_addr)
 		return NULL;
 
+	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) ||
+	    ((phys_addr - mrioc->sense_buf_dma) % MPI3MR_SENSE_BUF_SZ))
+		return NULL;
+
 	return mrioc->sense_buf + (phys_addr - mrioc->sense_buf_dma);
 }
 

Re: [PATCH 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:

New issues:
- [Low] 64-bit division by a non-constant integer causes a `__umoddi3` link error on 32-bit platforms with 64-bit DMA addresses.

Pre-existing issues:
- [High] Missing `dma_rmb()` in the operational and admin reply queue polling loops.
--

--- Patch [7]: [PATCH 07/17] mpi3mr: Fix out-of-bounds sense buffer access ---
commit 7536a65ff98a39b2e42c7eb228f93604d2b3086e
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 5e61448d8dbc6..103ce9dd22666 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_fw.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_fw.c
> @@ -141,6 +141,12 @@ void *mpi3mr_get_sensebuf_virt_addr(struct mpi3mr_ioc *mrioc,
>  	if (!phys_addr)
>  		return NULL;
>  
> +	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) ||
> +	    ((phys_addr - mrioc->sense_buf_dma) % MPI3MR_SENSE_BUF_SZ))

[Severity: Low]
Does this modulo operation cause a build failure on 32-bit platforms with
64-bit DMA addresses?

When building for a 32-bit architecture with CONFIG_ARCH_DMA_ADDR_T_64BIT
enabled (such as x86 PAE or ARM LPAE), dma_addr_t is 64-bit. Modulo
dividing a 64-bit integer by a runtime variable forces the compiler to
emit a call to __umoddi3, which is not exported in the kernel.

The issue was flagged in mpi3mr_get_reply_virt_addr() within the same file:

drivers/scsi/mpi3mr/mpi3mr_fw.c:mpi3mr_get_reply_virt_addr() {
    ...
	if ((phys_addr < mrioc->reply_buf_dma) ||
	    (phys_addr > mrioc->reply_buf_dma_max_address - mrioc->reply_sz) ||
	    ((phys_addr - mrioc->reply_buf_dma) % mrioc->reply_sz))
		return NULL;
    ...
}

[Severity: High]
This is a pre-existing issue, but does the reply queue processing miss
memory barriers?

Looking at the polling loops for both the operational and admin reply queues,
the phase bit is checked without a subsequent dma_rmb():

drivers/scsi/mpi3mr/mpi3mr_fw.c: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 similarly in mpi3mr_process_admin_reply_q():

drivers/scsi/mpi3mr/mpi3mr_fw.c:mpi3mr_process_admin_reply_q() {
    ...
	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;
	}
    ...
}

Under high concurrent I/O throughput on weakly-ordered architectures like
ARM64, when the hardware DMAs a reply descriptor, could the CPU speculatively
execute reads for the descriptor payload or sense buffer before the phase bit
condition is definitively synchronized?

If the phase bit changes, without a memory barrier, subsequent reads to other
descriptor fields might bypass the phase bit evaluation and read stale memory
data from the previous iteration.

> +		return NULL;
> +
>  	return mrioc->sense_buf + (phys_addr - mrioc->sense_buf_dma);
>  }

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

[PATCH 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>
---
 drivers/scsi/mpi3mr/mpi3mr_os.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
index 402d1f35d214..15207eed7edc 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)) {
@@ -2564,6 +2565,12 @@ static void mpi3mr_dev_rmhs_send_tm(struct mpi3mr_ioc *mrioc, u16 handle,
 	drv_cmd->callback = mpi3mr_dev_rmhs_complete_tm;
 	drv_cmd->dev_handle = handle;
 	drv_cmd->iou_rc = iou_rc;
+	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);
+		goto out_failed;
+	}
+
 	tm_req.dev_handle = cpu_to_le16(handle);
 	tm_req.task_type = MPI3_SCSITASKMGMT_TASKTYPE_TARGET_RESET;
 	tm_req.host_tag = cpu_to_le16(drv_cmd->host_tag);

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

[sashiko-bot]

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

New issues:
- [High] Placement of the newly added bounds check introduces a delayed command queue stall and async chain breakage.

Pre-existing issues:
- [Critical] Concurrent lockless modification of delayed lists from process and IRQ contexts.
- [High] TOCTOU race window during delayed command hardware dispatch allows double-free.
--

--- Patch [8]: [PATCH 08/17] mpi3mr: Fix out-of-bounds bitmap access during device removal ---
commit 498a49c2b9e406ac25107bbe9cd216b5c786f292
Author: Chandrakanth Patil <chandrakanth.patil@broadcom.com>

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

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.

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
> index f80a21ec161b6..7ae4b4fad3f4a 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 is there a race window here if the
hardware request completes extremely quickly?

mpi3mr_dev_rmhs_send_tm() sends the request to hardware. Only after it
returns does the code list_del() and kfree() the delayed_dev_rmhs node:

mpi3mr_dev_rmhs_complete_iou()
{
        ...
        mpi3mr_dev_rmhs_send_tm(mrioc, drv_cmd->dev_handle, drv_cmd,
            drv_cmd->iou_rc);
        list_del(&delayed_dev_rmhs->list);
        kfree(delayed_dev_rmhs);
        return;
}

If the completion executes concurrently and triggers another call to
mpi3mr_dev_rmhs_complete_iou(), could it dequeue and dispatch the exact
same node before the first thread has a chance to remove it, leading to
a double-free?

Similar patterns seem to exist in mpi3mr_complete_evt_ack().

[Severity: Critical]
This is also a pre-existing issue, but I noticed there might be unprotected
list operations on delayed_rmhs_list.

The delayed_rmhs_list is appended to in mpi3mr_dev_rmhs_send_tm():

        list_add_tail(&delayed_dev_rmhs->list,
            &mrioc->delayed_rmhs_list);

And it is removed from in mpi3mr_dev_rmhs_complete_iou():

        list_del(&delayed_dev_rmhs->list);

Are these list operations safe from concurrent modifications between process
and IRQ contexts, or should they be protected by a spinlock to prevent
list corruption? Similar patterns exist for delayed_evtack_cmds_list.

[ ... ]

> @@ -2564,6 +2565,12 @@ static void mpi3mr_dev_rmhs_send_tm(struct mpi3mr_ioc *mrioc, u16 handle,
>  	drv_cmd->callback = mpi3mr_dev_rmhs_complete_tm;
>  	drv_cmd->dev_handle = handle;
>  	drv_cmd->iou_rc = iou_rc;
> +	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);
> +		goto out_failed;
> +	}
> +

[Severity: High]
If handle >= mrioc->facts.max_devhandle, the code goes to out_failed.
This frees the command slot synchronously but does not invoke the
completion callback.

If this was called from mpi3mr_dev_rmhs_complete_iou() to process a delayed
command, would this synchronous failure prevent any subsequent delayed
items from being processed since the completion chain is broken?

Also, because this check is placed here, out-of-bounds handles can still be
appended to the delayed_rmhs_list earlier in this function when command
slots are full:

mpi3mr_dev_rmhs_send_tm()
{
        ...
        if (cmd_idx >= MPI3MR_NUM_DEVRMCMD) {
                delayed_dev_rmhs = kzalloc_obj(*delayed_dev_rmhs, GFP_ATOMIC);
                ...
                delayed_dev_rmhs->handle = handle;
                list_add_tail(&delayed_dev_rmhs->list, ...);
                return;
        }
        ...
}

Should the bounds check happen before allocating and adding to the delayed
list?

>  	tm_req.dev_handle = cpu_to_le16(handle);
>  	tm_req.task_type = MPI3_SCSITASKMGMT_TASKTYPE_TARGET_RESET;
>  	tm_req.host_tag = cpu_to_le16(drv_cmd->host_tag);

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

[PATCH 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>
---
 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 15207eed7edc..9af236e70624 100644
--- a/drivers/scsi/mpi3mr/mpi3mr_os.c
+++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
@@ -2522,6 +2522,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 {

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

[sashiko-bot]

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

Pre-existing issues:
- [High] Memory leak of tgt_dev reference in mpi3mr_target_alloc() error paths.
- [High] Memory leak of tgtdev reference in mpi3mr_get_port_id_by_rphy() when hba_port is NULL.
- [High] Memory leak of tgtdev reference in mpi3mr_sas_port_add() error paths.
--

--- Patch [9]: [PATCH 09/17] mpi3mr: Fix target device reference leak in device removal handshake ---
commit d4f3aa04629aa5ef9b7db8bd5ef68d81cfceef82
Author: Chandrakanth Patil <chandrakanth.patil@broadcom.com>

mpi3mr: Fix target device reference leak in device removal handshake

This commit fixes a target device reference leak in the device removal
handshake. It drops the reference acquired by the device lookup once the
state has been updated.

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
> index 7ae4b4fad3f4a..b18e7ea873030 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_os.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
> @@ -2522,6 +2522,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;

[Severity: High]
This isn't a bug introduced by this patch, but since this commit fixes a
target device reference leak, I noticed a similar pre-existing leak in
mpi3mr_target_alloc() in drivers/scsi/mpi3mr/mpi3mr_os.c.

If the validation checks fail, the function sets retval = -ENXIO and exits
without releasing the acquired tgt_dev reference:

drivers/scsi/mpi3mr/mpi3mr_os.c:mpi3mr_target_alloc() {
    ...
    tgt_dev = __mpi3mr_get_tgtdev_by_perst_id(mrioc, starget->id);
    if (tgt_dev && !tgt_dev->is_hidden && tgt_dev->non_stl) {
        ...
    } else {
        retval = -ENXIO;
    }
    ...
}

Does this cause a permanent memory leak of the target device structure?


[Severity: High]
This is a pre-existing issue, but there appears to be another leak in
mpi3mr_get_port_id_by_rphy() in drivers/scsi/mpi3mr/mpi3mr_transport.c.

The code retrieves the target device and takes a reference, but it only
puts the reference inside the condition block that checks if the device's
hba_port is non-NULL:

drivers/scsi/mpi3mr/mpi3mr_transport.c:mpi3mr_get_port_id_by_rphy() {
    ...
    tgtdev = __mpi3mr_get_tgtdev_by_addr_and_rphy(mrioc, ...);
    if (tgtdev && tgtdev->dev_spec.sas_sata_inf.hba_port) {
        ...
        mpi3mr_tgtdev_put(tgtdev);
    }
    ...
}

If a device is found but its hba_port is NULL, the function exits without
releasing the reference. Does this result in a persistent memory leak?


[Severity: High]
This is also a pre-existing issue, but does mpi3mr_sas_port_add() in
drivers/scsi/mpi3mr/mpi3mr_transport.c leak a reference during error paths?

When adding an end device, a reference to the target device is acquired. If
any subsequent setup or allocation fails, the code jumps to the out_fail
label. The cleanup path handles freeing the SAS port structure but seems to
bypass calling mpi3mr_tgtdev_put():

drivers/scsi/mpi3mr/mpi3mr_transport.c:mpi3mr_sas_port_add() {
    ...
    if (rphy->identify.device_type == SAS_END_DEVICE) {
        tgtdev = mpi3mr_get_tgtdev_by_addr(...);
    }
    ...
out_fail:
    ...
}

Could this leak the target device reference during port addition failures?

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

[PATCH 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>
---
 drivers/scsi/mpi3mr/mpi3mr_os.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
index 9af236e70624..94625e5f6af8 100644
--- a/drivers/scsi/mpi3mr/mpi3mr_os.c
+++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
@@ -2819,11 +2819,26 @@ 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;
 	struct mpi3mr_tgt_dev *tgtdev = NULL;
 	struct mpi3mr_stgt_priv_data *scsi_tgt_priv_data = NULL;
 
+	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);
+	if (topo_evt->num_entries > max_entries) {
+		ioc_err(mrioc, "SAS topology event: num_entries(%d) exceeds max(%d)\n",
+			topo_evt->num_entries, max_entries);
+		return;
+	}
+
 	for (i = 0; i < topo_evt->num_entries; i++) {
 		handle = le16_to_cpu(topo_evt->phy_entry[i].attached_dev_handle);
 		if (!handle)

Re: [PATCH 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:

Pre-existing issues:
- [High] Missing bounds checking in PCIe topology change event handlers.
- [High] Incomplete fix: early return in SAS top-half bypasses bottom-half cancellation.
- [High] Event data length is unbounded by host DMA buffer size.
--

--- Patch [10]: [PATCH 10/17] mpi3mr: Fix out-of-bounds read in SAS topology change events ---
commit bab975c5553e402733c8341f728b4c3f336df543
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.

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

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
> index b18e7ea873030..1291c60b4d98c 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_os.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_os.c

[ ... ]

> @@ -2819,11 +2819,26 @@ 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;
>  	struct mpi3mr_tgt_dev *tgtdev = NULL;
>  	struct mpi3mr_stgt_priv_data *scsi_tgt_priv_data = NULL;
>  
> +	avail_len = event_reply->event_data_length * 4;

[Severity: High]
This is a pre-existing issue, but does event_reply->event_data_length
need to be validated against the host DMA buffer size before being used?

Since event_data_length is provided by the firmware, if it exceeds the
hardware DMA reply frame size (mrioc->reply_sz), could this calculation
artificially inflate avail_len and defeat the bounds check?

Also, looking at mpi3mr_os_handle_events(), this unvalidated length
appears to be used for a memory copy:

mpi3mr_os_handle_events() {
    ...
    sz = event_reply->event_data_length * 4;
    fwevt = mpi3mr_alloc_fwevt(sz);
    ...
    memcpy(fwevt->event_data, event_reply->event_data, sz);
}

Could this result in an out-of-bounds read from the DMA reply frame pool?

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

[Severity: High]
This isn't a bug introduced by this patch, but does returning early here
leave the bottom-half handler scheduled with unvalidated data?

In mpi3mr_os_handle_events(), the bottom-half scheduling flag is set
before the top-half runs:

mpi3mr_os_handle_events() {
    ...
    process_evt_bh = 1;
    mpi3mr_sastopochg_evt_th(mrioc, event_reply);
}

If we hit this early return (or the one below), process_evt_bh remains 1.
When mpi3mr_sastopochg_evt_bh() subsequently runs, it will iterate over the
unvalidated event_data->num_entries, potentially reading out-of-bounds.

Should this function clear the bottom-half scheduling flag or return an error
code when validation fails?

> +	}
> +	max_entries = (avail_len -
> +		      offsetof(struct mpi3_event_data_sas_topology_change_list, phy_entry)) /
> +		      sizeof(struct mpi3_event_sas_topo_phy_entry);
> +	if (topo_evt->num_entries > max_entries) {
> +		ioc_err(mrioc, "SAS topology event: num_entries(%d) exceeds max(%d)\n",
> +			topo_evt->num_entries, max_entries);
> +		return;
> +	}
> +
>  	for (i = 0; i < topo_evt->num_entries; i++) {
>  		handle = le16_to_cpu(topo_evt->phy_entry[i].attached_dev_handle);
>  		if (!handle)

[Severity: High]
This is a pre-existing issue, but should a similar bounds check be applied
to the PCIe topology change events?

Looking at mpi3mr_pcietopochg_evt_th() and mpi3mr_pcietopochg_evt_bh(),
both handlers blindly iterate over the variable-length port_entry array
using the unvalidated num_entries provided by firmware:

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

Could a maliciously large num_entries provided by a device cause an
out-of-bounds memory read here as well?

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

[PATCH 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>
---
 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 94625e5f6af8..6d4ddc78bcd0 100644
--- a/drivers/scsi/mpi3mr/mpi3mr_os.c
+++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
@@ -3109,7 +3109,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;
 
@@ -3170,7 +3170,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;
@@ -3204,7 +3209,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,

Re: [PATCH 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] Bottom-half event handlers perform out-of-bounds heap reads on the `fwevt` buffer because they trust unvalidated loop bounds against the newly clamped allocation size.

Pre-existing issues:
- [High] Top-half event handlers perform out-of-bounds DMA reads on the reply buffer due to missing or flawed bounds checking for variable-length arrays.
- [High] `MPI3_EVENT_DEVICE_INFO_CHANGED` incorrectly falls through to `MPI3_EVENT_LOG_DATA`, silently dropping events and bypassing its required bottom-half processing.
- [High] Accessing a potentially freed work item during event cleanup via `cancel_work_sync`.
--

--- Patch [11]: [PATCH 11/17] mpi3mr: Fix out-of-bounds read of event data ---
commit 4872e2f7b284da8f062de528f26e6b6482b6a169
Author: Chandrakanth Patil <chandrakanth.patil@broadcom.com>
Subject: mpi3mr: Fix out-of-bounds read of event data

The commit clamps the event data length read from the reply to what the
frame can hold. This prevents out-of-bounds reads when caching log data
and when sizing the buffer allocated for the bottom-half event handlers.

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
> index 1291c60b4d98c..68b13c2ad2244 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_os.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
> @@ -3170,7 +3170,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 does MPI3_EVENT_DEVICE_INFO_CHANGED
intentionally fall through to MPI3_EVENT_LOG_DATA without setting
process_evt_bh = 1?

Because process_evt_bh is not set, the bottom-half handler is completely
bypassed, leaving mpi3mr_devinfochg_evt_bh() as effectively dead code:

mpi3mr_fwevt_bh() {
    ...
    case MPI3_EVENT_DEVICE_INFO_CHANGED:
    {
        dev_pg0 = (struct mpi3_device_page0 *)fwevt->event_data;
        perst_id = le16_to_cpu(dev_pg0->persistent_id);
        if (perst_id != MPI3_DEVICE0_PERSISTENTID_INVALID)
            mpi3mr_devinfochg_evt_bh(mrioc, dev_pg0);
    ...
}

Will this cause the driver to fail to process queue depth changes for target
devices?

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

[ ... ]

> @@ -3204,7 +3209,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);

[Severity: High]
Does clamping the allocation size here introduce an out-of-bounds heap read in
the bottom-half handlers?

The patch correctly limits the allocation of fwevt to sz, but the payload
copied into it can still contain an unmodified, untrusted num_entries from
the firmware.

When the bottom-half worker executes, won't it iterate up to num_entries,
reading past the end of the strictly clamped fwevt allocation?

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

Can this lead to an out-of-bounds read on the kernel heap?

[Severity: High]
This is a pre-existing issue, but does mpi3mr_pcietopochg_evt_th() also
lack proper bounds checking before looping over num_entries?

It iterates directly based on the untrusted num_entries:

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

Similarly, in mpi3mr_sastopochg_evt_th(), the bounds check relies on
the unclamped event_data_length instead of mrioc->reply_sz:

mpi3mr_sastopochg_evt_th() {
    avail_len = event_reply->event_data_length * 4;
    ...
    max_entries = (avail_len - offsetof(...)) / sizeof(...);
    if (topo_evt->num_entries > max_entries) return;
}

Will this result in out-of-bounds DMA reads during top-half event processing if
the firmware sends an erroneously large event_data_length?

[Severity: High]
This is a pre-existing issue, but is there a use-after-free risk during
driver cleanup?

In mpi3mr_cleanup_fwevt_list(), current_event is read locklessly and passed to
mpi3mr_cancel_work():

mpi3mr_cleanup_fwevt_list() {
    ...
    if (mrioc->current_event) {
        fwevt = mrioc->current_event;
        ...
        mpi3mr_cancel_work(fwevt);
    ...
}

Concurrently, the worker thread can drop the reference count and free fwevt
before clearing mrioc->current_event:

mpi3mr_fwevt_bh() {
    ...
out:
    mpi3mr_fwevt_put(fwevt);
    mrioc->current_event = NULL;
}

Could mpi3mr_cleanup_fwevt_list() pass a freed fwevt pointer to
cancel_work_sync(&fwevt->work)?

>  		if (!fwevt) {
>  			dprint_event_th(mrioc,

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

[PATCH 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>
---
 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);

Re: [PATCH 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] The firmware event handlers for SAS and PCIe topology changes iterate up to `num_entries` without ensuring it is strictly bounded by the event data length, leading to out-of-bounds reads.
- [High] This problem wasn't introduced by this patch, but the entirety of `mpi3mr_update_links` continues to access `mr_sas_node` and performs lockless list traversals on its structures after releasing `mrioc->sas_node_lock` on the happy path.
- [High] This problem wasn't introduced by this patch, but a shift-sign-extension bug exists when updating 64-bit `phy_mask` bitmasks using `1 << i`, leading to out-of-bounds array accesses when `i >= 31`.
--

--- Patch [12]: [PATCH 12/17] mpi3mr: Fix out-of-bounds phy array access on link change ---
commit 5a98eebaf3b1e059c60e29aa1529199221440703
Author: Chandrakanth Patil <chandrakanth.patil@broadcom.com>

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

This commit adds a bounds check to ensure the phy number associated with a
link change does not exceed the allocated number of phys for the node. If it
does, the event is logged and ignored.

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

[Severity: High]
This is a pre-existing issue, but does dropping the sas_node_lock here leave
mr_sas_node vulnerable to concurrent modification or use-after-free?

Looking at the remainder of mpi3mr_update_links(), mr_sas_node and its members
continue to be used after the lock is released:

    spin_unlock_irqrestore(&mrioc->sas_node_lock, flags);
    if (handle && (link_rate >= MPI3_SAS_NEG_LINK_RATE_1_5)) {
        mpi3mr_set_identify(mrioc, handle, &mr_sas_phy->remote_identify);
        mpi3mr_add_phy_to_an_existing_port(mrioc, mr_sas_node, mr_sas_phy, ...);

When mpi3mr_add_phy_to_an_existing_port() is called, it iterates over the
sas_port_list locklessly:

    list_for_each_entry(mr_sas_port, &mr_sas_node->sas_port_list, port_list) {

Since mrioc->sas_node_lock is no longer held, can other events concurrently add
or remove ports, corrupting the traversal? Additionally, mr_sas_node is fetched
without acquiring a reference, potentially allowing it to be concurrently freed.

[Severity: High]
This isn't a bug introduced by this patch, but could the firmware event
handlers for SAS and PCIe topology changes iterate up to num_entries without
bounds checking against the event data length?

In mpi3mr_sastopochg_evt_bh() and mpi3mr_pcietopochg_evt_bh() in mpi3mr_os.c,
the loop blindly iterates up to num_entries:

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

If the hardware or an attacker controlling the controller issues an event with
a num_entries field larger than what fits in the reply buffer, could this
lead to an out-of-bounds read? Even if the top-half handler attempts to bound
num_entries, returning early without dropping the event still allows the
bottom-half to iterate over the truncated buffer.

Additionally, mpi3mr_pcietopochg_evt_th() appears to completely lack any
bounds checking before iterating over topo_evt->port_entry.

[Severity: High]
This isn't a bug introduced by this patch, but is there a sign-extension
regression when updating the 64-bit phy_mask bitmask in
mpi3mr_add_phy_to_an_existing_port()?

    mr_sas_port->phy_mask |= (1 << i);

When i evaluates to 31, the expression 1 << 31 evaluates as a negative 32-bit
integer (0x80000000). When this is bitwise OR'd into the 64-bit phy_mask, it
sign-extends to 0xFFFFFFFF80000000ULL, erroneously setting bits 31 through 63.

Later, in mpi3mr_update_mr_sas_port(), this corrupted mask seems to be used
in a loop:

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

If phys_to_be_added inherits the sign-extended mask, won't this loop exceed
the allocated bounds of mrioc->sas_hba.phy (which may only have 32 elements)
and corrupt memory?

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

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

[PATCH 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>
---
 drivers/scsi/mpi3mr/mpi3mr_os.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
index 6d4ddc78bcd0..00bd654d7f24 100644
--- a/drivers/scsi/mpi3mr/mpi3mr_os.c
+++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
@@ -2753,11 +2753,26 @@ 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;
 	struct mpi3mr_tgt_dev *tgtdev = NULL;
 	struct mpi3mr_stgt_priv_data *scsi_tgt_priv_data = NULL;
 
+	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);
+	if (topo_evt->num_entries > max_entries) {
+		ioc_err(mrioc, "PCIe topology event: num_entries(%d) exceeds max(%d)\n",
+			topo_evt->num_entries, max_entries);
+		return;
+	}
+
 	for (i = 0; i < topo_evt->num_entries; i++) {
 		handle = le16_to_cpu(topo_evt->port_entry[i].attached_dev_handle);
 		if (!handle)

Re: [PATCH 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:

New issues:
- [High] The newly added bounds check reads `topo_evt->num_entries` directly from a DMA-mapped buffer, and the subsequent loop condition re-reads it, creating a Time-Of-Check to Time-Of-Use (TOCTOU) race condition.

Pre-existing issues:
- [High] The bounds check for PCIe and SAS topology change events fails to protect the bottom-half event handlers from out-of-bounds reads because early returns in the top-half do not prevent the malformed event from being queued.
- [High] The bounds check uses an untrusted `event_data_length` without verifying it against the actual physical dimensions of the hardware reply buffer, allowing an out-of-bounds read in the top-half handler.
- [High] Missing `dma_rmb()` after polling the phase bit in DMA-mapped reply descriptors, which can lead to processing stale descriptor data on weakly ordered architectures.
--

--- Patch [14]: [PATCH 14/17] mpi3mr: Fix out-of-bounds read in PCIe topology change events ---
commit 8553af34c1ff3c0cff0462ca66842979af8cb4d4
Author: Chandrakanth Patil <chandrakanth.patil@broadcom.com>

mpi3mr: Fix out-of-bounds read in PCIe topology change events
    
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.

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
> --- a/drivers/scsi/mpi3mr/mpi3mr_os.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
> @@ -2753,11 +2753,26 @@ 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;
>  	struct mpi3mr_tgt_dev *tgtdev = NULL;
>  	struct mpi3mr_stgt_priv_data *scsi_tgt_priv_data = NULL;
>  
> +	avail_len = event_reply->event_data_length * 4;

[Severity: High]
This isn't a bug introduced by this patch, but could this calculation
exceed the actual physical dimensions of the hardware reply buffer?

Because avail_len is derived strictly from the untrusted event_data_length,
a compromised or malicious device could provide a value larger than the
allocated DMA reply buffer (mrioc->reply_sz).

Should we verify event_data_length against mrioc->reply_sz to prevent
the bounds check from being artificially inflated, which would lead to an
out-of-bounds read? 

The SAS topology handler mpi3mr_sastopochg_evt_th() appears to share this
exact pattern.

> +	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);
> +	if (topo_evt->num_entries > max_entries) {
> +		ioc_err(mrioc, "PCIe topology event: num_entries(%d) exceeds max(%d)\n",
> +			topo_evt->num_entries, max_entries);
> +		return;
> +	}

[Severity: High]
This is a pre-existing issue, but does this early return actually prevent
the malformed event from being processed by the bottom-half handler?

When mpi3mr_pcietopochg_evt_th() returns early here, the process_evt_bh flag
remains set in mpi3mr_os_handle_events():

drivers/scsi/mpi3mr/mpi3mr_os.c:mpi3mr_os_handle_events() {
    ...
	case MPI3_EVENT_PCIE_TOPOLOGY_CHANGE_LIST:
	{
		process_evt_bh = 1;
		mpi3mr_pcietopochg_evt_th(mrioc, event_reply);
		break;
	}
    ...
}

Since process_evt_bh remains 1, the event is queued to the worker thread.
The bottom-half handler mpi3mr_pcietopochg_evt_bh() will then blindly
iterate up to the unvalidated event_data->num_entries, causing an
out-of-bounds read past the end of the allocated fwevt heap buffer.

Would it be safer to clear process_evt_bh or return a status from the
top-half handler to prevent queuing malformed events? The SAS topology
handler mpi3mr_sastopochg_evt_bh() is likely vulnerable to the exact
same incomplete fix flaw.

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

[Severity: High]
Is this loop vulnerable to a Time-Of-Check to Time-Of-Use (TOCTOU) race
condition?

The newly added bounds check reads topo_evt->num_entries directly from a
DMA-mapped buffer, and this loop condition continually re-evaluates it. 

Because the loop body contains functions that acquire spinlocks
(e.g., mpi3mr_get_tgtdev_by_handle()), which act as full compiler memory
barriers, the compiler is forced to re-fetch num_entries from DMA memory
on every iteration.

If a malicious device alters num_entries in DMA memory after the initial
max_entries check, could it extend the loop beyond bounds? 

Should we cache num_entries in a local variable before the bounds check
and use the local variable for the loop condition? The SAS topology handler
mpi3mr_sastopochg_evt_th() appears to have the identical issue.

[Severity: High]
This isn't a bug introduced by this patch, but is a dma_rmb() barrier needed
after polling the phase bit in the DMA-mapped reply descriptors?

Looking at mpi3mr_process_op_reply_q() in drivers/scsi/mpi3mr/mpi3mr_fw.c:

drivers/scsi/mpi3mr/mpi3mr_fw.c: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;
	}

	do {
		if (mrioc->unrecoverable || mrioc->io_admin_reset_sync)
			break;

		req_q_idx = le16_to_cpu(reply_desc->request_queue_id) - 1;
    ...
}

The driver polls the hardware DMA ring by checking the reply_flags phase mask.
Without a subsequent dma_rmb(), weakly ordered CPU architectures (such as
ARM64) can speculatively execute out-of-order reads and fetch the rest of
the descriptor payload (like request_queue_id) before the hardware has
actually committed the updated descriptor.

Could this result in processing corrupted or stale descriptor payloads?
There appears to be a similar polling loop for the admin reply queue in
mpi3mr_process_admin_reply_q() lacking a dma_rmb() before accessing
request_queue_ci.

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

[PATCH 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>
---
 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];

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

[sashiko-bot]

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

Pre-existing issues:
- [High] An integer overflow in the bounds check of `mpi3mr_bsg_upload_hdb` allows a malicious user to bypass the check and read arbitrary kernel memory.
--

--- Patch [15]: [PATCH 15/17] mpi3mr: zero out diagnostic buffer status memory ---
commit 76872b40f16cd1ab1c93f0a03cc6bd1fb6b3db94
Author: Chandrakanth Patil <chandrakanth.patil@broadcom.com>

mpi3mr: zero out diagnostic buffer status memory
    
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>

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_app.c b/drivers/scsi/mpi3mr/mpi3mr_app.c
> index f5b48c95cf055..0cdcb8d236d37 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];

[Severity: High]
This is a pre-existing issue, but while reviewing this file I noticed a
potential integer overflow in mpi3mr_bsg_upload_hdb():

drivers/scsi/mpi3mr/mpi3mr_app.c:mpi3mr_bsg_upload_hdb() {
    ...
	if ((upload_hdb.start_offset + upload_hdb.length) > diag_buffer->size) {
		dprint_bsg_err(mrioc,
		    "%s: invalid start offset %d, length %d for type %d\n",
		    __func__, upload_hdb.start_offset, upload_hdb.length,
		    upload_hdb.buf_type);
		return -EINVAL;
	}
    ...
}

Can upload_hdb.start_offset and upload_hdb.length wrap around during addition?

Since they are 32-bit unsigned integers controlled by user space via the BSG
interface, a large start_offset (e.g. 0xFFFFFFF0) and a small length
(e.g. 0x20) could wrap to a sum that passes the bounds check against
diag_buffer->size.

Would this allow a malicious user to bypass the check and read arbitrary kernel
memory when the subsequent copy occurs?

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

[PATCH 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>
---
 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 00bd654d7f24..c740a64164e5 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);

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

[sashiko-bot]

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

Pre-existing issues:
- [High] Use-After-Free of firmware event (`fwevt`) in `mpi3mr_cleanup_fwevt_list` during driver teardown.
--

--- Patch [16]: [PATCH 16/17] mpi3mr: Fix use-after-free of the firmware event workqueue ---
commit d1068d82a1a0b06d1a9647481bac5d43ff5f1ebc
Author: Chandrakanth Patil <chandrakanth.patil@broadcom.com>

mpi3mr: Fix use-after-free of the firmware event workqueue

This commit fixes a use-after-free issue regarding the firmware event
workqueue in the mpi3mr driver. It ensures the worker thread pointer is
checked while holding the event lock to prevent a race condition with the
workqueue being destroyed.

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
> index 510c990d42d1b..4a72b3d89c294 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 is a pre-existing issue, but while reviewing the firmware event
lifecycle, I noticed another potential use-after-free during driver teardown.

In mpi3mr_dequeue_fwevt(), the list reference is dropped before returning
the pointer:

drivers/scsi/mpi3mr/mpi3mr_os.c:mpi3mr_dequeue_fwevt() {
    ...
		fwevt = list_first_entry(&mrioc->fwevt_list,
		    struct mpi3mr_fwevt, list);
		list_del_init(&fwevt->list);
		/*
		 * Put fwevt reference count after
		 * removing it from fwevt_list
		 */
		mpi3mr_fwevt_put(fwevt);
	}
	spin_unlock_irqrestore(&mrioc->fwevt_lock, flags);

	return fwevt;
}

Then in mpi3mr_cleanup_fwevt_list(), we pass this unreferenced pointer to
cancel_work_sync():

drivers/scsi/mpi3mr/mpi3mr_os.c:mpi3mr_cleanup_fwevt_list() {
    ...
	while ((fwevt = mpi3mr_dequeue_fwevt(mrioc)))
		mpi3mr_cancel_work(fwevt);
    ...
}

Could the background worker thread (mpi3mr_fwevt_worker) concurrently finish
executing and free the fwevt structure, causing cancel_work_sync() inside
mpi3mr_cancel_work() to access freed memory?

Additionally, lockless access to mrioc->current_event in
mpi3mr_cleanup_fwevt_list() could similarly read a concurrently freed event if
the worker thread finishes in the middle of cleanup:

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

Does this code need to hold a reference to fwevt while calling
mpi3mr_cancel_work() to prevent the worker thread from freeing it?

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

[PATCH 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>
---
 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 c740a64164e5..6f1a9472675d 100644
--- a/drivers/scsi/mpi3mr/mpi3mr_os.c
+++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
@@ -5972,6 +5972,8 @@ mpi3mr_pcierr_error_detected(struct pci_dev *pdev, pci_channel_state_t state)
 			return PCI_ERS_RESULT_DISCONNECT;
 		}
 
+		mrioc->stop_drv_processing = 1;
+		mpi3mr_cleanup_fwevt_list(mrioc);
 		scsi_block_requests(mrioc->shost);
 		mpi3mr_stop_watchdog(mrioc);
 		mpi3mr_cleanup_resources(mrioc);
@@ -6069,6 +6071,7 @@ static void mpi3mr_pcierr_resume(struct pci_dev *pdev)
 
 	if (mrioc->block_on_pci_err) {
 		mrioc->block_on_pci_err = false;
+		mrioc->stop_drv_processing = 0;
 		scsi_unblock_requests(shost);
 		mpi3mr_start_watchdog(mrioc);
 	}

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

[sashiko-bot]

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

New issues:
- [High] Calling `mpi3mr_cleanup_fwevt_list` during PCI error recovery causes a deadlock due to a race with the fwevt worker thread calling SML functions.
- [High] Setting `stop_drv_processing = 1` during PCI channel freeze breaks transparent I/O recovery by permanently failing commands with `DID_NO_CONNECT`.

Pre-existing issues:
- [High] Unprotected and unreferenced access to `mrioc->current_event` and `fwevt` in `mpi3mr_cleanup_fwevt_list` leads to a Use-After-Free, exacerbated by the patch executing it asynchronously during PCI error recovery.
- [High] Skipping `cancel_work_sync` for events blocked in the SCSI mid-layer (SML) leaves the worker thread running, allowing access to unmapped registers on suspend.
- [High] Missing memory barriers between `stop_drv_processing` and `mrioc->current_event` allows the worker thread to bypass the abort check and access unmapped memory.
--

--- Patch [17]: [PATCH 17/17] mpi3mr: Fix NULL pointer dereference on PCI error recovery ---
commit 21853a66ae88d53ee80d24101e9ce79158e66a4e
Author: Chandrakanth Patil <chandrakanth.patil@broadcom.com>

mpi3mr: Fix NULL pointer dereference on PCI error recovery

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.

> --- a/drivers/scsi/mpi3mr/mpi3mr_os.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
> @@ -5972,6 +5972,8 @@ mpi3mr_pcierr_error_detected(struct pci_dev *pdev, pci_channel_state_t state)
>  			return PCI_ERS_RESULT_DISCONNECT;
>  		}
>  
> +		mrioc->stop_drv_processing = 1;

[Severity: High]
Does setting stop_drv_processing to 1 here break transparent I/O recovery?

If mrioc->stop_drv_processing evaluates to true in mpi3mr_qcmd(), it
preempts the block_on_pci_err check. Instead of returning
SCSI_MLQUEUE_HOST_BUSY to allow the midlayer to safely pause and retry
I/Os once the PCI channel recovers, it causes all incoming queued
read/write commands to immediately fail with DID_NO_CONNECT:

mpi3mr_qcmd() {
    ...
    if (mrioc->stop_drv_processing && !(mpi3mr_allow_scmd_to_fw(scmd))) {
        scmd->result = DID_NO_CONNECT << 16;
        scsi_done(scmd);
        goto out;
    }
    ...
}

Could this lead to immediate I/O errors and potential filesystem corruption
instead of transparently pausing I/O?

[Severity: High]
This is a pre-existing issue, but does the lack of explicit memory barriers
here allow a race condition?

The synchronization between writing stop_drv_processing here and reading
mrioc->current_event in mpi3mr_cleanup_fwevt_list() appears to be lockless.
Concurrently, mpi3mr_fwevt_bh() writes mrioc->current_event and reads
stop_drv_processing.

Without smp_mb(), could the CPU reorder these operations such that
mpi3mr_pcierr_error_detected() reads a NULL current_event while
mpi3mr_fwevt_bh() sees stop_drv_processing as 0? This would let the worker
continue and access PCI registers after they have been unmapped.

> +		mpi3mr_cleanup_fwevt_list(mrioc);

[Severity: High]
Can calling mpi3mr_cleanup_fwevt_list() during PCI error recovery cause a
deadlock?

During PCI error recovery, mrioc->block_on_pci_err is true. This cleanup
function reads fwevt->pending_at_sml locklessly. If the worker hasn't yet
set pending_at_sml to 1, the cleanup function sees 0 and calls
cancel_work_sync(), which blocks the PCI error handler.

The worker thread could then set pending_at_sml to 1 and call SCSI mid-layer
functions like scsi_remove_target(), issuing I/O commands like
SYNCHRONIZE_CACHE. Because mpi3mr_allow_scmd_to_fw() permits these bypass
commands, mpi3mr_qcmd() will hit the block_on_pci_err check and return
SCSI_MLQUEUE_HOST_BUSY.

Since the SCSI mid-layer retries indefinitely, the worker thread remains
permanently blocked, and cancel_work_sync() deadlocks the PCI error handler.

[Severity: High]
This is a pre-existing issue, but does the asynchronous call to
mpi3mr_cleanup_fwevt_list() introduced here exacerbate a use-after-free
risk?

In mpi3mr_cleanup_fwevt_list(), mrioc->current_event is read locklessly
into a local fwevt pointer. Concurrently, the firmware event worker thread
might finish its execution, drop all references via mpi3mr_fwevt_put(), and
free the mpi3mr_fwevt structure.

If mpi3mr_cleanup_fwevt_list() accesses fwevt->pending_at_sml or fwevt->work
after the last reference is dropped but before mrioc->current_event is
set to NULL, does this trigger a use-after-free?

[Severity: High]
This isn't a bug introduced by this patch, but does skipping
cancel_work_sync() in mpi3mr_cleanup_fwevt_list() when
fwevt->pending_at_sml is set leave the worker thread running?

If the event is pending at the SCSI mid-layer, the code sets
fwevt->discard = 1 and returns early without stopping the worker thread:

mpi3mr_cleanup_fwevt_list() {
    ...
    if (current_work() == &fwevt->work || fwevt->pending_at_sml) {
        fwevt->discard = 1;
        return;
    }
    ...
}

During operations like system suspend where pci_err_recovery is false, the
caller unmaps mrioc->sysif_regs. Once unblocked, the worker thread continues
to mpi3mr_process_event_ack() and mpi3mr_admin_request_post().

Since pci_err_recovery is false, it blindly dereferences the unmapped
mrioc->sysif_regs to post the request, causing a kernel panic.

>  		scsi_block_requests(mrioc->shost);
>  		mpi3mr_stop_watchdog(mrioc);
>  		mpi3mr_cleanup_resources(mrioc);

[ ... ]

> @@ -6069,6 +6071,7 @@ static void mpi3mr_pcierr_resume(struct pci_dev *pdev)
>  
>  	if (mrioc->block_on_pci_err) {
>  		mrioc->block_on_pci_err = false;
> +		mrioc->stop_drv_processing = 0;
>  		scsi_unblock_requests(shost);
>  		mpi3mr_start_watchdog(mrioc);
>  	}

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