Re: [PATCH v2 04/17] mpi3mr: Fix target device reference leak in BSG task management

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

Pre-existing issues:
- [High] Time-of-check to time-of-use (TOCTOU) race condition leading to a Use-After-Free on `stgt_priv`.
- [High] Data race between BSG timeout handling and admin reply ISR causing a Use-After-Free on `sense_buff_k`.
- [Medium] Missing endianness conversion when reading `dev_handle` from `mpi3_scsi_task_mgmt_request`.
--

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

mpi3mr: Fix target device reference leak in BSG task management

This commit fixes a reference leak for the target device in the BSG task
management path. It ensures the reference is dropped whenever the device
lookup succeeds, regardless of whether the SCSI target data is fully set up.

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_app.c b/drivers/scsi/mpi3mr/mpi3mr_app.c
> index a4fe10a05cb3a..94b992acb2330 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_app.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_app.c
> @@ -2745,10 +2745,12 @@ static long mpi3mr_bsg_process_mpt_cmds(struct bsg_job *job)
>  	}
>  	if (block_io) {
>  		tgtdev = mpi3mr_get_tgtdev_by_handle(mrioc, dev_handle);

[Severity: Medium]
This isn't a bug introduced by this patch, but is dev_handle missing an
endianness conversion here?

It appears dev_handle is read natively from the request structure:

mpi3mr_bsg_process_mpt_cmds() {
    ...
    dev_handle = tm_req->dev_handle;
    ...
}

Since dev_handle is defined as little-endian (__le16) in
struct mpi3_scsi_task_mgmt_request, could Big-Endian systems read a
byte-swapped value, causing mpi3mr_get_tgtdev_by_handle() to look up the
wrong device?

> -		if (tgtdev && tgtdev->starget && tgtdev->starget->hostdata) {
> -			stgt_priv = (struct mpi3mr_stgt_priv_data *)
> -			    tgtdev->starget->hostdata;
> -			atomic_inc(&stgt_priv->block_io);
> +		if (tgtdev) {
> +			if (tgtdev->starget && tgtdev->starget->hostdata) {
> +				stgt_priv = (struct mpi3mr_stgt_priv_data *)
> +				    tgtdev->starget->hostdata;
> +				atomic_inc(&stgt_priv->block_io);

[Severity: High]
This is a pre-existing issue, but does caching stgt_priv here create a
use-after-free risk later in the function?

When a user-space process issues a blocking command, the thread will wait
for completion:

mpi3mr_bsg_process_mpt_cmds() {
    ...
    wait_for_completion_timeout(&mrioc->bsg_cmds.done, ...);
    if (block_io && stgt_priv)
        atomic_dec(&stgt_priv->block_io);
    ...
}

If the target device and its hostdata are concurrently destroyed while the
thread is blocked, tgt_dev->starget is cleared:

mpi3mr_target_destroy() {
    ...
    tgt_dev->starget = NULL;
    ...
}

If this race occurs, wouldn't the BSG thread wake up and dereference the
freed stgt_priv pointer when calling atomic_dec()?

> +			}
>  			mpi3mr_tgtdev_put(tgtdev);
>  		}

[Severity: High]
This is also a pre-existing issue, but is there a data race in the BSG
timeout handling that can cause a use-after-free on the sense buffer?

In the timeout path, the user thread locklessly clears the pointer and
frees the memory:

mpi3mr_bsg_process_mpt_cmds() {
    ...
    mrioc->bsg_cmds.sensebuf = NULL;
    mrioc->bsg_cmds.state = MPI3MR_CMD_NOTUSED;
    ...
    kfree(sense_buff_k);
    ...
}

If the admin reply ISR executes concurrently and checks the cmdptr->sensebuf
condition before it is cleared:

mpi3mr_fw.c:mpi3mr_process_admin_reply_desc() {
    if (sense_buf && cmdptr->sensebuf) {
        cmdptr->is_sense = 1;
        memcpy(cmdptr->sensebuf, sense_buf, MPI3MR_SENSE_BUF_SZ);
    }
    ...
}

Could the ISR execute memcpy() into the freed sense_buff_k memory block?

>  	}

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