Re: [PATCH] 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: size_t truncation bypasses buffer overflow check on 32-bit architectures
- [High] scsi: mvumi: Out-of-bounds access due to unvalidated list_num_io
--

commit b87e745d0db555a98674b0d478b3e35f7a31fefb
Author: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
scsi: mvumi: reject handshake sizes that overflow the list allocations

This commit adds firmware handshake boundary checks to reject sizes that
could overflow integer capacities. It restricts shift counts to avoid
integer overflow and introduces a check on the total required allocation
size.

> diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
> index e70d336b4ab39..b86d954f3576b 100644
> --- a/drivers/scsi/mvumi.c
> +++ b/drivers/scsi/mvumi.c
> @@ -912,10 +912,23 @@ static int mvumi_init_data(struct mvumi_hba *mhba)
>  	unsigned int tmp_size, offset, i;
>  	void *virmem, *v;
>  	dma_addr_t p;
> +	size_t total;
>  
>  	if (mhba->fw_flag & MVUMI_FW_ALLOC)
>  		return 0;
>  
> +	/* These are firmware-supplied and size the allocations below. */
> +	total = (size_t)mhba->ib_max_size * mhba->max_io +
> +		(size_t)mhba->ob_max_size * mhba->max_io +
> +		sizeof(struct mvumi_dyn_list_entry) * (size_t)mhba->max_io +
> +		160;
> +	if (total > UINT_MAX) {

[Severity: High]
Does this truncation bypass the buffer overflow check on 32-bit architectures?

The calculation casts the factors to size_t. On 32-bit architectures,
size_t is exactly 32 bits. If the firmware provides large values, the
product can silently wrap to 0 during the arithmetic.

Because total is of type size_t, it mathematically can never exceed UINT_MAX
on 32-bit systems. The check total > UINT_MAX becomes tautologically false
and is bypassed, which allows the driver to proceed and allocate undersized
buffers using the wrapped 32-bit values.

> +		dev_err(&mhba->pdev->dev,
> +			"handshake sizes too large: io %u, in %u, out %u\n",
> +			mhba->max_io, mhba->ib_max_size, mhba->ob_max_size);
> +		return -1;
> +	}

[ ... ]

> @@ -1051,16 +1064,24 @@ static int mvumi_hs_process_page(struct mvumi_hba *mhba,
>  	case HS_PAGE_FIRM_CAP:
>  		hs_page1 = (struct mvumi_hs_page1 *) hs_header;

[ ... ]

> -		if (mhba->hba_capability & HS_CAPABILITY_NEW_PAGE_IO_DEPTH_DEF)
> -			mhba->list_num_io = 1 << hs_page1->cl_inout_list_depth;
> +		if (mhba->hba_capability & HS_CAPABILITY_NEW_PAGE_IO_DEPTH_DEF) {
> +			if (hs_page1->cl_inout_list_depth >= 32) {
> +				dev_err(&mhba->pdev->dev,
> +					"invalid handshake list depth\n");
> +				return -1;
> +			}
> +			mhba->list_num_io = 1U << hs_page1->cl_inout_list_depth;

[Severity: High]
Can this lead to an out-of-bounds access if list_num_io exceeds max_io?

The patch caps cl_inout_list_depth to 31, allowing list_num_io to be up
to 2^31, but fails to check whether list_num_io exceeds max_io.

The ib_list and ob_list arrays are explicitly sized and allocated based
on max_io in mvumi_init_data(). The array indices unconditionally increment
with each IO operation and only wrap around when they reach list_num_io.

If list_num_io > max_io, the driver can sequentially advance the index
beyond the allocated array size after processing max_io commands, causing
out-of-bounds heap reads and writes.

> +		}
>  		break;
>  	default:

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