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

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 <[email protected]>

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/[email protected]?part=1