Re: [PATCH 01/17] mpi3mr: Fix buffer overflow in BSG passthrough request copy

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

Pre-existing issues:
- [Critical] Pointer arithmetic wrap-around on 32-bit architectures allows bypassing bounds checks, leading to arbitrary kernel memory read/write via DMA.
- [High] DMA coherent memory allocated for NVMe PRPs is leaked on BSG command timeout or submission failure.
- [High] Target device `block_io` atomic counter is leaked if BSG command submission fails.
- [High] Lockless TOCTOU check of `MPI3MR_CMD_COMPLETE` causes a Use-After-Free of `sense_buff_k` and data races on `reply` buffers.
- [High] Missing validation of the BSG request length (`job->request_len`) allows a user to send an undersized request buffer, leading to out-of-bounds memory reads and kernel panics.
--

--- Patch [1]: [PATCH 01/17] mpi3mr: Fix buffer overflow in BSG passthrough request copy ---
commit ed6513158d24e842e223917348ef598d4d161609
Author: Chandrakanth Patil <[email protected]>
Subject: mpi3mr: Fix buffer overflow in BSG passthrough request copy

This commit fixes a buffer overflow in the BSG passthrough request copy.
The check for an incoming BSG request size used a narrower variable than
its source field, allowing large values to wrap and pass the check.
The variable is widened to ensure the full size is validated properly.

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_app.c b/drivers/scsi/mpi3mr/mpi3mr_app.c
> index 1353a8ff9c85d..8e5d24793efd0 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_app.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_app.c
> @@ -2384,7 +2384,7 @@ static long mpi3mr_bsg_process_mpt_cmds(struct bsg_job *job)
>  	long rval = -EINVAL;
>  	struct mpi3mr_ioc *mrioc = NULL;
>  	u8 *mpi_req = NULL, *sense_buff_k = NULL;
> -	u8 mpi_msg_size = 0;
> +	u32 mpi_msg_size = 0;
>  	struct mpi3mr_bsg_packet *bsg_req = NULL;
>  	struct mpi3mr_bsg_mptcmd *karg;
>  	struct mpi3mr_buf_entry *buf_entries = NULL;
> @@ -2538,7 +2538,7 @@ static long mpi3mr_bsg_process_mpt_cmds(struct bsg_job *job)
>  				rval = -EINVAL;
>  				goto out;
>  			}
> -			memcpy(mpi_req, sgl_iter, buf_entries->buf_len);
> +			memcpy(mpi_req, sgl_iter, mpi_msg_size);
>  			break;
>  		default:
>  			invalid_be = 1;

[Severity: Critical]
This is a pre-existing issue, but can the pointer arithmetic nearby lead to
an integer wrap-around on 32-bit architectures?

When iterating over buffer entries in the same function:

	case MPI3MR_BSG_BUFTYPE_DATA_OUT:
		sgl_iter = sgl_dout_iter;
		sgl_dout_iter += buf_entries->buf_len;

If a user submits a BSG request with a crafted buf_len, does the u32 value wrap
the sgl_dout_iter pointer on 32-bit systems? This wrapped pointer could then
bypass the later bounds check:

	if (sgl_dout_iter > (dout_buf + job->request_payload.payload_len)) {

and allow arbitrary kernel memory access during subsequent payload processing.

While reviewing this patch, I noticed a few other pre-existing issues.

> drivers/scsi/mpi3mr/mpi3mr_app.c:mpi3mr_bsg_request()
> {
> 	long rval = -EINVAL;
> 	unsigned int reply_payload_rcv_len = 0;
> 
> 	struct mpi3mr_bsg_packet *bsg_req = job->request;
> 
> 	switch (bsg_req->cmd_type) {

[Severity: High]
This isn't a bug introduced by this patch, but does this function properly
validate job->request_len before dereferencing bsg_req->cmd_type?

If userspace submits a BSG request with a request_len of zero, could this
trigger a zero size pointer dereference and cause a kernel panic? Similarly,
if the length is too small, would this read uninitialized heap memory?

> drivers/scsi/mpi3mr/mpi3mr_app.c:mpi3mr_bsg_process_mpt_cmds()
> {
[ ... ]
> 	if (block_io) {
> 		tgtdev = mpi3mr_get_tgtdev_by_handle(mrioc, dev_handle);
> 		if (tgtdev && tgtdev->starget && tgtdev->starget->hostdata) {
> 			stgt_priv = (struct mpi3mr_stgt_priv_data *)
> 			    tgtdev->starget->hostdata;
> 			atomic_inc(&stgt_priv->block_io);
> 			mpi3mr_tgtdev_put(tgtdev);
> 		}
> 	}
[ ... ]
> 	rval = mpi3mr_admin_request_post(mrioc, mpi_req,
> 	    MPI3MR_ADMIN_REQ_FRAME_SZ, 0);
> 
> 	if (rval) {
> 		mrioc->bsg_cmds.is_waiting = 0;
> 		dprint_bsg_err(mrioc,
> 		    "%s: posting bsg request is failed\n", __func__);
> 		rval = -EAGAIN;
> 		goto out_unlock;
> 	}

[Severity: High]
This is a pre-existing issue, but does this error path permanently leak the
block_io atomic counter?

If mpi3mr_admin_request_post() fails, the goto out_unlock bypasses the
atomic_dec(&stgt_priv->block_io) that occurs after the completion timeout
logic. Does this leave the SCSI target device blocked indefinitely?

[Severity: High]
Similarly, this is a pre-existing issue, but does the same goto out_unlock
skip freeing the DMA memory allocated for NVMe PRPs?

If mpi3mr_build_nvme_prp() successfully allocated DMA memory in
mrioc->prp_list_virt earlier in this function, the jump to out_unlock skips
the success-path cleanup block:

	if (mrioc->prp_list_virt) {
		dma_free_coherent(&mrioc->pdev->dev, mrioc->prp_sz,
		    mrioc->prp_list_virt, mrioc->prp_list_dma);

Would subsequent commands overwrite mrioc->prp_list_virt and permanently
leak the previous DMA coherent allocation?

> drivers/scsi/mpi3mr/mpi3mr_fw.c:mpi3mr_process_admin_reply_desc()
> {
[ ... ]
> 	cmdptr = mpi3mr_get_drv_cmd(mrioc, host_tag, def_reply);
> 	if (cmdptr) {
> 		if (cmdptr->state & MPI3MR_CMD_PENDING) {
> 			cmdptr->state |= MPI3MR_CMD_COMPLETE;
> 			cmdptr->ioc_loginfo = ioc_loginfo;
[ ... ]
> 			if (sense_buf && cmdptr->sensebuf) {
> 				cmdptr->is_sense = 1;
> 				memcpy(cmdptr->sensebuf, sense_buf,
> 				       MPI3MR_SENSE_BUF_SZ);
> 			}

[Severity: High]
This isn't a bug introduced by this patch, but does setting the
MPI3MR_CMD_COMPLETE flag before the memory copy create a race condition?

If the application thread in mpi3mr_bsg_process_mpt_cmds() wakes up from a
natural timeout exactly when this flag is set, it performs a lockless check:

	if (!(mrioc->bsg_cmds.state & MPI3MR_CMD_COMPLETE)) {

Could the application thread assume the handler has fully finished, proceed
to read uninitialized reply memory, and free sense_buff_k before the interrupt
handler finishes copying into it? Would this result in a use-after-free when
the interrupt handler eventually executes the memcpy?

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