[PATCH V2] scsi: mpi3mr: check the SMP passthrough reply status

[PATCH V2] scsi: mpi3mr: check the SMP passthrough reply status

[Ilya Khomyakov]

mpi3mr_transport_smp_handler() obtains the controller reply status from
mpi3mr_post_transport_req() but never examines it. The status is written to
the debug log and then discarded, after which the reply is copied to the
caller and bsg_job_done() is called with a success code.

mpi3mr_post_transport_req() returns zero once the request completes,
regardless of the reported ioc_status. Its return value only reports a
command already in use, a failure to submit the request, or a timeout. The
existing "if (rc)" test therefore never observes a request that the
controller refused.

mpi3mr_report_manufacture() issues its SMP request through the same helper
and tests ioc_status before using the reply. mpi3mr_transport_smp_handler()
is the only caller that passes reply data to user space, and it performs no
such test.

As a result a refused SMP request reaches user space as a successful
transfer of zero bytes. The BSG completion path derives the sg_io_v4 status
fields from job->result, so with a result of zero device_status,
transport_status and driver_status all read as zero and SG_INFO_CHECK is
not set. The only remaining indication is din_resid, which stays equal to
din_xfer_len. A caller that does not compare the two accepts the empty
buffer as an SMP response frame and parses it.

This was observed on a 9600-16e (SAS4116) running firmware 8.17.1.0 with
personality 0 and profile id 3, that is PerfIT SAS Only mode, with an
expander attached through an x8 wide port at 22.5 Gb/s. A user space daemon
on that system issued SMP passthrough requests that the controller refused.
Over roughly one hour the driver logged 109 of them, every one as

  mpi3mr0: sending SMP request
  mpi3mr0: SMP request completed with ioc_status(0x0001)
  mpi3mr0: SMP request - reply data transfer size(0)

ioc_status 0x0001 is MPI3_IOCSTATUS_INVALID_FUNCTION. All 109 were
reported to the caller as successful transfers.

The driver's own SMP requests to the same expander complete normally on the
same system, for example

  mpi3mr0: report manufacturer SMP request completed with ioc_status(0x0000)
  mpi3mr0: report manufacturer - reply data transfer size(60)

so a status other than success here describes an individual request that
the controller declined, not a controller that lacks the function. Besides
MPI3_IOCSTATUS_INVALID_FUNCTION the firmware can report BUSY,
INSUFFICIENT_RESOURCES, INVALID_STATE, FAILURE, SAS_SMP_REQUEST_FAILED and
SAS_SMP_DATA_OVERRUN on this path, and all of them are currently returned
to user space as a successful transfer.

Test the reply status and return -EIO when the controller did not complete
the request successfully. The new exit uses the existing unmap_in label, so
the DMA buffers are released exactly as before, and reslen is still zero at
that point, so the length reported to the block layer stays consistent with
the error code.

Move the reply copy above the new test. When a job completes with a
negative result the BSG completion path shortens job->reply_len to
sizeof(u32) and returns those four bytes to the caller. job->reply points
at a buffer that is allocated once per tag in bsg_init_rq() and is not
cleared between requests, so leaving it untouched on the new exit would
return four bytes left over from whatever command last used that tag.
mpi_reply is zeroed before the request is submitted, so copying it
unconditionally makes those bytes describe this request. The error paths
that precede the request submission have the same property, but they are
not reached by a completed request and are left alone here.

The change was tested with a patched out-of-tree build of the same driver
on the adapter described above. Before the change all 109 refused requests
in a 64 minute window reached the "reply data transfer size" trace and
completed successfully. After the change all 141 refused requests in a 73
minute window returned early: each was logged with ioc_status(0x0001) and
none reached that trace, which is the expected signature of the new exit
since it precedes the trace. The daemon that issued the requests behaved
the same before and after the change. No warnings were reported by the
driver or the block layer during either run.

This does not modify discovery, I/O or link management behavior. It changes
only the result reported for an SMP passthrough request that the controller
did not complete successfully.

Signed-off-by: Ilya Khomyakov <khomyakovilya@gmail.com>

---
 drivers/scsi/mpi3mr/mpi3mr_transport.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/mpi3mr/mpi3mr_transport.c b/drivers/scsi/mpi3mr/mpi3mr_transport.c
--- a/drivers/scsi/mpi3mr/mpi3mr_transport.c
+++ b/drivers/scsi/mpi3mr/mpi3mr_transport.c
@@ -3320,12 +3320,18 @@
 	dprint_transport_info(mrioc,
 	    "SMP request completed with ioc_status(0x%04x)\n", ioc_status);
 
+	memcpy(job->reply, &mpi_reply, reply_sz);
+	job->reply_len = reply_sz;
+
+	if (ioc_status != MPI3_IOCSTATUS_SUCCESS) {
+		rc = -EIO;
+		goto unmap_in;
+	}
+
 	dprint_transport_info(mrioc,
 		    "SMP request - reply data transfer size(%d)\n",
 		    le16_to_cpu(mpi_reply.response_data_length));
 
-	memcpy(job->reply, &mpi_reply, reply_sz);
-	job->reply_len = reply_sz;
 	reslen = le16_to_cpu(mpi_reply.response_data_length);
 
 	if (addr_in)

Re: [PATCH V2] scsi: mpi3mr: check the SMP passthrough reply status

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing bounds check on user-provided payload lengths causes integer underflow, leading to massive out-of-bounds DMA.
- [High] mpi3mr_transport_phy_reset ignores the controller's reply status (ioc_status) when resetting HBA phys, falsely reporting success on failure.
- [Medium] Unbounded firmware response length triggers a WARN_ON in the generic BSG layer.
- [Low] Early error paths in BSG handlers leak 4 bytes of stale kernel heap memory to user space via the uninitialized `job->reply` buffer.
--

--- Patch [1]: [PATCH V2] scsi: mpi3mr: check the SMP passthrough reply status ---
Note: The format of this report has been altered to avoid quoting the
original patch code directly, due to recitation policy restrictions. The
findings are provided as a free-form summary below.

Commit: 8f30f09fa87333575dd13664d3ca77572ba9a47e
Author: Ilya Khomyakov <khomyakovilya@gmail.com>
Subject: scsi: mpi3mr: check the SMP passthrough reply status
Summary: The patch modifies mpi3mr_transport_smp_handler to check the
controller's reply status from mpi3mr_post_transport_req. It ensures refused
SMP requests correctly return an error code instead of falsely reporting
success.

[Severity: Low]
This isn't a bug introduced by this patch, but looking at the early error
paths in mpi3mr_transport_smp_handler (for example, when checking
mrioc->reset_in_progress), the function exits to the out label without
populating job->reply. Does this leak 4 bytes of stale kernel memory to
user space? Since job->reply is reused across requests without zeroing,
the bsg core completing the job will truncate the reply length to 4 bytes
and copy uninitialized or stale data to the user's response buffer.

[Severity: Medium]
This is a pre-existing issue, but further down in
mpi3mr_transport_smp_handler, the code assigns reslen directly from the
controller's mpi_reply.response_data_length. It then passes reslen to
bsg_job_done without bounding it against the user-allocated
job->reply_payload.payload_len. Could this trigger a WARN_ON in the
generic BSG layer? If the firmware returns a response length greater than
the allocated buffer, the BSG core's bsg_transport_sg_io_fn triggers a
WARN_ON for the mismatched lengths, which could act as a denial of service
vector if panic_on_warn is configured.

[Severity: High]
This is also a pre-existing issue, but I noticed a potential integer
underflow when setting up the scatter-gather elements in
mpi3mr_transport_smp_handler. The code subtracts 4 from dma_len_out and
dma_len_in when calling mpi3mr_add_sg_single. What happens if a user
passes a payload length of 1 to 3 bytes? Since there is no bounds check
ensuring the payload is at least 4 bytes, wouldn't this subtraction
underflow to a massive 32-bit unsigned integer and cause out-of-bounds DMA
operations?

[Severity: High]
This isn't a bug introduced by this patch, but I found a very similar
logic error to the one you just fixed, located nearby in
mpi3mr_transport_phy_reset. When resetting an HBA phy, it calls
mpi3mr_post_transport_req and logs the ioc_status, but then completely
ignores the status value and falls through to return success. Can this
disrupt SAS link recovery? If the controller actively refuses the reset,
falsely reporting success might cause the transport layer to incorrectly
assume the phy is operational.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260817143841.1856-1-khomyakovilya@gmail.com?part=1