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

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 <[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.

Fixes: 506bc1a0d6ba ("scsi: mpi3mr: Add support for MPT commands")
Signed-off-by: Chandrakanth Patil <[email protected]>

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