Re: [PATCH v2 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:
- [High] Missing validation of `job->request_len` allows out-of-bounds slab memory reads when parsing the BSG request and buffer entries.
- [High] On 32-bit architectures, pointer wrap-around allows bypassing bounds checks for specific buffer types, leading to out-of-bounds heap access.
- [High] DMA coherent memory allocated for NVMe PRP lists is leaked on command posting failures or command timeouts.
- [High] Potential Use-After-Free of `stgt_priv` if the SCSI target is removed concurrently during a long-running BSG command.
- [High] Data race and use-after-timeout on `mrioc->bsg_cmds` state between timeout handler and interrupt handler.
--

commit 6909218adbd3af92548ad9194e535cec60aaccf1
Author: Chandrakanth Patil <[email protected]>

mpi3mr: Fix buffer overflow in BSG passthrough request copy

The size of an incoming BSG request is checked using a variable that is
narrower than the field it is read from, so large values wrap and pass
the check. The copy that follows then uses the full value and writes
past the request buffer.

Widen the variable and copy only the amount that was checked.

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 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;

[Severity: High]
This is a pre-existing issue, but does this lack validation of
job->request_len when parsing the BSG request and buffer entries?

drivers/scsi/mpi3mr/mpi3mr_app.c:mpi3mr_bsg_process_mpt_cmds() {
...
	bsg_req = job->request;
	karg = (struct mpi3mr_bsg_mptcmd *)&bsg_req->cmd.mptcmd;
...
	bufcnt = karg->buf_entry_list.num_of_entries;
...
	buf_entries = karg->buf_entry_list.buf_entry;
...
}

And similarly in mpi3mr_bsg_process_drv_cmds():

drivers/scsi/mpi3mr/mpi3mr_app.c:mpi3mr_bsg_process_drv_cmds() {
...
	bsg_req = job->request;
	drvrcmd = &bsg_req->cmd.drvrcmd;
	mrioc = mpi3mr_bsg_verify_adapter(drvrcmd->mrioc_id);
...
}

If a user intentionally specifies a request_len smaller than the size of
struct mpi3mr_bsg_packet, could this result in an out-of-bounds slab memory
read since the driver casts and accesses the buffer without checking the
length?

> @@ -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: High]
This is a pre-existing issue, but does this pointer arithmetic bypass
bounds checks on 32-bit architectures?

drivers/scsi/mpi3mr/mpi3mr_app.c:mpi3mr_bsg_process_mpt_cmds() {
...
		case MPI3MR_BSG_BUFTYPE_MPI_REPLY:
			sgl_iter = sgl_din_iter;
			sgl_din_iter += buf_entries->buf_len;
...
}

If a user submits a massive buf_len (e.g., 0xFFFFFFFF), could the addition
to sgl_din_iter (which is a u8 *) cause an integer wrap-around on 32-bit
systems?

Since a wrapped pointer is mathematically smaller, would it bypass the
safety check:

	if (sgl_din_iter > (din_buf + job->reply_payload.payload_len))

And allow an out-of-bounds heap write when the driver copies the reply:

	memcpy(drv_buf_iter->bsg_buf, bsg_reply_buf, tmplen);

[Severity: High]
This is a pre-existing issue, but does this code leak DMA coherent memory
on command posting failures or timeouts?

drivers/scsi/mpi3mr/mpi3mr_app.c:mpi3mr_build_nvme_prp() {
...
	mrioc->prp_list_virt = dma_alloc_coherent(&mrioc->pdev->dev,
	    dev_pgsz, &mrioc->prp_list_dma, GFP_KERNEL);
...
}

drivers/scsi/mpi3mr/mpi3mr_app.c:mpi3mr_bsg_process_mpt_cmds() {
...
		if (rval) {
			mrioc->bsg_cmds.is_waiting = 0;
			dprint_bsg_err(mrioc,
			    "%s: posting bsg request is failed\n", __func__);
			rval = -EAGAIN;
			if (block_io && stgt_priv)
				atomic_dec(&stgt_priv->block_io);
			goto out_unlock;
		}
...
}

If mpi3mr_admin_request_post() fails or if the command times out, the code
jumps to out_unlock. Does out_unlock fail to free mrioc->prp_list_virt,
leading to a permanent leak of DMA memory?

[Severity: High]
This is a pre-existing issue, but is there a potential Use-After-Free of
stgt_priv if the SCSI target is removed concurrently?

drivers/scsi/mpi3mr/mpi3mr_app.c:mpi3mr_bsg_process_mpt_cmds() {
...
		if (tgtdev) {
			if (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);
		}
...
		if (block_io && stgt_priv)
			atomic_dec(&stgt_priv->block_io);
...
}

The reference to the target device is dropped via mpi3mr_tgtdev_put(tgtdev)
before the long wait. If the physical SCSI device is hot-unplugged during
the wait for command completion, could the SCSI midlayer free the starget
and stgt_priv before atomic_dec() is called?

[Severity: High]
This is a pre-existing issue, but is there a data race on mrioc->bsg_cmds
state between the timeout handler and the interrupt handler?

drivers/scsi/mpi3mr/mpi3mr_app.c:mpi3mr_bsg_process_mpt_cmds() {
...
		if (!(mrioc->bsg_cmds.state & MPI3MR_CMD_COMPLETE)) {
			mrioc->bsg_cmds.is_waiting = 0;
			rval = -EAGAIN;
...
}

And in the interrupt handler:

drivers/scsi/mpi3mr/mpi3mr_fw.c:mpi3mr_process_admin_reply_desc() {
...
	cmdptr->state |= MPI3MR_CMD_COMPLETE;
...
	cmdptr->state &= ~MPI3MR_CMD_PENDING;
...
	if (cmdptr->is_waiting) {
		cmdptr->is_waiting = 0;
		complete(&cmdptr->done);
	}
...
}

If a BSG command times out, the timeout handler locklessly sets
is_waiting = 0 and aborts, allowing another thread to acquire the lock
and submit a new command using the same structure.

If a delayed hardware interrupt for the original command fires while the
new command is pending, could the non-atomic modifications to cmdptr->state
in mpi3mr_process_admin_reply_desc corrupt the state of the new command
and prematurely complete it?

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