Re: [PATCH v3] scsi: mvumi: reject handshake sizes that overflow the list allocations

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] scsi: mvumi: state corruption on error path bypasses check
- [Medium] scsi: mvumi: missing reset bounds check for max_target_id causes OOB read
--

commit 0bf09b555b23de542df2211e673ba896cdb17d70
Author: Yehyeong Lee <yhlee@isslab.korea.ac.kr>

scsi: mvumi: reject handshake sizes that overflow the list allocations

This patch introduces checks to reject invalid handshake entry sizes and
list depths from the firmware to prevent allocation overflows. It bounds
the worst-case allocation sizes and rejects handshakes that exceed them,
particularly when buffer sizes grow across a host reset.

> diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
> index e70d336b4ab39..72a1516a804bb 100644
> --- a/drivers/scsi/mvumi.c
> +++ b/drivers/scsi/mvumi.c

[ ... ]

> @@ -1048,19 +1061,32 @@ static int mvumi_hs_process_page(struct mvumi_hba *mhba,
>  	}
>  
>  	switch (hs_header->page_code) {
> -	case HS_PAGE_FIRM_CAP:
> +	case HS_PAGE_FIRM_CAP: {
> +		unsigned int old_max_io = mhba->max_io;
> +		unsigned int old_ib = mhba->ib_max_size;
> +		unsigned int old_ob = mhba->ob_max_size;
> +		unsigned int old_list = mhba->list_num_io;
> +
>  		hs_page1 = (struct mvumi_hs_page1 *) hs_header;

[ ... ]

>  		mhba->max_io = hs_page1->max_io_support;
>  		mhba->list_num_io = hs_page1->cl_inout_list_depth;
>  		mhba->max_transfer_size = hs_page1->max_transfer_size;
>  		mhba->max_target_id = hs_page1->max_devices_support;

[Severity: Medium]
Is it safe to unconditionally update mhba->max_target_id here?

If a firmware handshake during a host reset returns a max_devices_support
value larger than the initial one, the target_map buffer is not reallocated
because MVUMI_FW_ALLOC is set. During a subsequent mvumi_flush_cache() call,
could the loop using this updated max_target_id read past the end of the
target_map allocation?

[ ... ]

> +		/* On reset the buffers are not reallocated; reject sizes that
> +		 * outgrow them.
> +		 */
> +		if ((mhba->fw_flag & MVUMI_FW_ALLOC) &&
> +		    (mhba->max_io > old_max_io || mhba->ib_max_size > old_ib ||
> +		     mhba->ob_max_size > old_ob ||
> +		     mhba->list_num_io > old_list)) {
> +			dev_err(&mhba->pdev->dev,
> +				"handshake sizes grew across reset\n");
> +			return -1;
> +		}

[Severity: High]
Does returning -1 here permanently corrupt the state in the mhba structure?

Because mhba->max_io and the other size fields were already updated with
the newly reported firmware values earlier in this block, returning
without restoring them leaves the oversized values stored in the mhba
structure. If the host reset is retried, old_max_io and the other
variables will cache these newly corrupted values as the baseline.

Would this cause the growth check to evaluate to false (since new == old),
successfully bypassing the protection entirely and allowing the
out-of-bounds array accesses the patch seeks to prevent?

>  		break;
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014021919.3486362-1-yhlee@isslab.korea.ac.kr?part=1