Re: [PATCH v2 03/17] mpi3mr: Fix I/O block counter leak on admin request post failure

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

Pre-existing issues:
- [High] Use-after-free of `stgt_priv` during BSG command completion and the error path, due to a lack of reference counting on the SCSI target.
- [High] Out-of-bounds read when processing BSG buffer entries due to missing bounds check on `num_of_entries` against `job->request_len`.
--

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

mpi3mr: Fix I/O block counter leak on admin request post failure

The per device I/O block counter is raised before a task management
request is posted and only lowered once the request completes. If the
post itself fails the counter stays raised, so this commit lowers it
on the failure path as well.

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_app.c b/drivers/scsi/mpi3mr/mpi3mr_app.c
> index 1cd1c38dcd565..a4fe10a05cb3a 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_app.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_app.c
> @@ -2782,6 +2782,8 @@ static long mpi3mr_bsg_process_mpt_cmds(struct bsg_job *job)
>  		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);

[Severity: High]
This isn't a bug introduced by this patch, but does dropping the target
device reference earlier leave stgt_priv unprotected?

Earlier in this function, the tgtdev reference is dropped without pinning
the SCSI target:

	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 a concurrent device removal occurs, the SCSI midlayer could destroy the
target and free its hostdata via mpi3mr_target_destroy() while the driver
is processing the request or sleeping in wait_for_completion_timeout().

Could this lead to a use-after-free when atomic_dec(&stgt_priv->block_io)
is called on this error path and on the completion path?

>  		goto out_unlock;
>  	}

[Severity: High]
This is also a pre-existing issue, but does this function properly validate
the number of buffer entries against the allocated request payload size?

Earlier in mpi3mr_bsg_process_mpt_cmds():

	bufcnt = karg->buf_entry_list.num_of_entries;
	...
	for (count = 0; count < bufcnt; count++, buf_entries++, drv_buf_iter++) {
		switch (buf_entries->buf_type) {

If a userspace application supplies a small request_len but a large
num_of_entries, could the loop read past the end of the job->request payload?

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