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

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

[Ilya Khomyakov]

Hello. 

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. In that
configuration the controller refuses SMP passthrough requests submitted
through the BSG interface. Over roughly one hour the driver logged 109
such requests, every one of them 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.

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.

On an error path the BSG completion path replaces job->reply_len with
sizeof(u32), so the MPI reply structure that today carries ioc_status to
user space is no longer copied out. That structure is driver specific and
is not examined by smp_utils; returning a proper error code is the more
useful result.

The change was tested with a mpi3mr 8.17.1.0.0 build on the
adapter described above. Before the change every refused request reached
the "reply data transfer size" trace and completed successfully. After the
change 21 refused requests over a four minute window all 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.
During the same window mpi3mr_report_manufacture() issued one SMP request
that the controller completed with ioc_status(0x0090),
MPI3_IOCSTATUS_SAS_SMP_REQUEST_FAILED; that path already tests the status
and was unaffected.

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 | 5 +++++
 1 file changed, 5 insertions(+)

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,6 +3320,11 @@ mpi3mr_transport_smp_handler(struct bsg_job *job, struct Scsi_Host *shost,
 	dprint_transport_info(mrioc,
 	    "SMP request completed with ioc_status(0x%04x)\n", ioc_status);
 
+	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));

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

[sashiko-bot]

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

Pre-existing issues:
- [High] Subtracting 4 from DMA payload lengths can underflow, leading to a massive out-of-bounds DMA transfer.
- [Medium] Bypassing the reply buffer initialization on the error path leaks 4 bytes of stale kernel memory to user space.
- [Medium] `mpi3mr_transport_phy_reset()` does not verify `ioc_status` when issuing a phy reset command for HBA phys, causing a silent failure.
--

--- Patch [1]: [PATCH] scsi: mpi3mr: check the SMP passthrough reply status ---
commit e3ae5fbfa11b135aa2d03a8b686ed5c492bc6d86
Author: Ilya Khomyakov <khomyakovilya@gmail.com>

scsi: mpi3mr: check the SMP passthrough reply status

This commit adds a check for ioc_status in mpi3mr_transport_smp_handler().
If the controller did not complete the SMP passthrough request successfully,
it now returns -EIO and skips the reply transfer, preventing the block layer
from seeing a successful zero-byte transfer.

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_transport.c b/drivers/scsi/mpi3mr/mpi3mr_transport.c
> index 240f67a8e2e3b..6054d68ea112b 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_transport.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_transport.c
> @@ -3320,6 +3320,11 @@ mpi3mr_transport_smp_handler(struct bsg_job *job, struct Scsi_Host *shost,
>  	dprint_transport_info(mrioc,
>  	    "SMP request completed with ioc_status(0x%04x)\n", ioc_status);
>  
> +	if (ioc_status != MPI3_IOCSTATUS_SUCCESS) {
> +		rc = -EIO;
> +		goto unmap_in;
> +	}
> +

[Severity: Medium]
This is a pre-existing issue, but does skipping the reply initialization leak
stale kernel memory to user space?

When taking the new error path, we skip copying mpi_reply to job->reply,
leaving job->reply with uninitialized data from a previous request.

The block layer's bsg_transport_sg_io_fn() will intercept the negative result,
set job->reply_len = sizeof(u32), and copy those uninitialized 4 bytes to 
user space:

block/bsg-lib.c:bsg_transport_sg_io_fn() {
    ...
    if (job->result < 0) {
        job->reply_len = sizeof(u32);
        ret = job->result;
    }

    if (job->reply_len && hdr->response) {
        int len = min(hdr->max_response_len, job->reply_len);

        if (copy_to_user(uptr64(hdr->response), job->reply, len))
    ...
}

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but can the DMA length calculation
underflow if the user-supplied payload length is less than 4 bytes?

Looking at mpi3mr_transport_smp_handler(), the driver subtracts 4 from the
payload lengths without ensuring a minimum length:

drivers/scsi/mpi3mr/mpi3mr_transport.c:mpi3mr_transport_smp_handler() {
    ...
    psge = &mpi_request.request_sge;
    mpi3mr_add_sg_single(psge, sgl_flags, dma_len_out - 4, dma_addr_out);

    psge = &mpi_request.response_sge;
    mpi3mr_add_sg_single(psge, sgl_flags, dma_len_in - 4, dma_addr_in);
    ...
}

If a local user with BSG access (CAP_SYS_RAWIO) submits a request with a
payload length of 1, 2, or 3 bytes, this subtraction could underflow to a huge
32-bit value, causing an out-of-bounds DMA transfer to the firmware.


[Severity: Medium]
This is a pre-existing issue, but does mpi3mr_transport_phy_reset() fail to
propagate firmware errors?

The function successfully issues the reset request but ignores the controller's
completion status:

drivers/scsi/mpi3mr/mpi3mr_transport.c:mpi3mr_transport_phy_reset() {
    ...
    if (mpi3mr_post_transport_req(mrioc, &mpi_request, request_sz,
        &mpi_reply, reply_sz, MPI3MR_INTADMCMD_TIMEOUT, &ioc_status)) {
        rc = -EAGAIN;
        goto out;
    }

    dprint_transport_info(mrioc,
        "phy reset request completed with ioc_status(0x%04x)\n",
        ioc_status);
out:
    return rc;
}

Since rc remains 0 even if ioc_status indicates an error, does this break the
SAS error recovery state machine by silently reporting success?

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