Re: [PATCH v2 08/17] mpi3mr: Fix out-of-bounds bitmap access during device removal

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

Pre-existing issues:
- [High] Failure to dispatch a Task Management request permanently strands the delayed device removal queue.
- [High] Silent failure during memory allocation drops device removal requests.
--

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

mpi3mr: Fix out-of-bounds bitmap access during device removal

This commit checks the device handle before indexing the remove pending
bitmap or building a task management request, preventing out-of-bounds
access if the controller reports an invalid handle.

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
> index f80a21ec161b6..2a35f146fe69b 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_os.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
> @@ -2401,7 +2401,8 @@ static void mpi3mr_dev_rmhs_complete_iou(struct mpi3mr_ioc *mrioc,
>  		ioc_info(mrioc,
>  		    "%s :dev removal handshake completed successfully: handle(0x%04x)\n",
>  		    __func__, drv_cmd->dev_handle);
> -		clear_bit(drv_cmd->dev_handle, mrioc->removepend_bitmap);
> +		if (drv_cmd->dev_handle < mrioc->facts.max_devhandle)
> +			clear_bit(drv_cmd->dev_handle, mrioc->removepend_bitmap);
>  	}
>  
>  	if (!list_empty(&mrioc->delayed_rmhs_list)) {

[Severity: High]
This is a pre-existing issue, but does a dispatch failure permanently strand
the delayed device removal queue here?

If mpi3mr_dev_rmhs_send_tm() is called to process a delayed TM request, but
mpi3mr_admin_request_post() fails during dispatch:

    retval = mpi3mr_admin_request_post(mrioc, &tm_req, sizeof(tm_req), 1);
    if (retval) {
        ...
        goto out_failed;
    }
    ...
out_failed:
    drv_cmd->state = MPI3MR_CMD_NOTUSED;
    clear_bit(cmd_idx, mrioc->devrem_bitmap);

The command is marked as unused and dropped. Because
mpi3mr_dev_rmhs_complete_iou() only dequeues one element from
delayed_rmhs_list and does not loop if the dispatch fails, the active
command slot becomes idle.

Does this break the chain of asynchronous completions, leaving any remaining
elements in the delayed list stranded indefinitely?

> @@ -2515,6 +2516,20 @@ static void mpi3mr_dev_rmhs_send_tm(struct mpi3mr_ioc *mrioc, u16 handle,
>  	struct mpi3mr_tgt_dev *tgtdev = NULL;
>  	unsigned long flags;
>  
> +	if (handle >= mrioc->facts.max_devhandle) {
> +		ioc_err(mrioc, "dev_remove_hs: handle(0x%04x) >= max_devhandle(0x%04x)\n",
> +			handle, mrioc->facts.max_devhandle);
> +		if (drv_cmd) {
> +			cmd_idx = drv_cmd->host_tag - MPI3MR_HOSTTAG_DEVRMCMD_MIN;
> +			drv_cmd->state = MPI3MR_CMD_NOTUSED;
> +			drv_cmd->callback = NULL;
> +			drv_cmd->dev_handle = MPI3MR_INVALID_DEV_HANDLE;
> +			drv_cmd->retry_count = 0;
> +			clear_bit(cmd_idx, mrioc->devrem_bitmap);
> +		}
> +		return;
> +	}
> +
>  	spin_lock_irqsave(&mrioc->tgtdev_lock, flags);

[Severity: High]
This is a pre-existing issue, but does a memory allocation failure silently
drop device removal requests here?

Later in mpi3mr_dev_rmhs_send_tm(), if no slots are available in
devrem_bitmap, the code falls back to adding the request to the delayed
list:

    if (cmd_idx >= MPI3MR_NUM_DEVRMCMD) {
        delayed_dev_rmhs = kzalloc_obj(*delayed_dev_rmhs, GFP_ATOMIC);
        if (!delayed_dev_rmhs)
            return;

If the atomic allocation fails, the function returns silently without
propagating an error. Can this result in the controller waiting
indefinitely for a handshake if the removal event is lost?

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