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

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

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

commit ab35632c4adcdd391fb038b2fa77fa3b546f692a
Author: Chandrakanth Patil <[email protected]>

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

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

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

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

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

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

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


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

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

And later in the same function:

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

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

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

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2