Re: [PATCH v2] 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: endianness bug and OOB read via find_first_bit on 32-bit int
- [High] mvumi: firmware sizes overwrite allocated limits during host reset
--

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

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

This patch rejects entry sizes above 29 and list depths of 32 or more from the
firmware to prevent shift overflows and out-of-bounds array access. It also
calculates the total allocation size in a 64-bit variable, failing if it exceeds
UINT_MAX, to prevent 32-bit truncation before memory allocation.

Cc: stable@vger.kernel.org

> diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
> index e70d336b4ab39..e9ba072220331 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;
> +	u64 total;
>  
>  	if (mhba->fw_flag & MVUMI_FW_ALLOC)
>  		return 0;
>  
> +	/* These are firmware-supplied and size the allocations below. */
> +	total = (u64)mhba->ib_max_size * mhba->max_io +
> +		(u64)mhba->ob_max_size * mhba->max_io +
> +		sizeof(struct mvumi_dyn_list_entry) * (u64)mhba->max_io +
> +		160;
> +	if (total > UINT_MAX) {

[Severity: High]
Does this bypass the new bounds checks during a host reset?

During a host reset (mvumi_host_reset() -> mvumi_wait_for_outstanding() ->
mvumi_start()), a new firmware handshake is initiated. In
mvumi_hs_process_page(), structural limits like mhba->max_io and
mhba->ib_max_size are unconditionally overwritten with new firmware values.

When mvumi_init_data() is subsequently called, MVUMI_FW_ALLOC is already
set, causing an early return. This completely bypasses the new
total > UINT_MAX validation and does not reallocate buffers.

If the firmware supplies larger sizes during a reset, the driver will use
the larger bounds to index into the previously allocated, smaller buffers,
resulting in an out-of-bounds memory access.

While reviewing the changes to mhba->list_num_io, I noticed a related issue
in mvumi_hs_build_page():

drivers/scsi/mvumi.c:mvumi_hs_build_page() {
    ...
		if (mhba->hba_capability
			& HS_CAPABILITY_NEW_PAGE_IO_DEPTH_DEF) {
			hs_page4->ob_depth = find_first_bit((unsigned long *)
							    &mhba->list_num_io,
							    BITS_PER_LONG);
			hs_page4->ib_depth = find_first_bit((unsigned long *)
							    &mhba->list_num_io,
							    BITS_PER_LONG);
    ...
}

[Severity: High]
Does this code cause an out-of-bounds read and endianness bug on 64-bit systems?

Passing a pointer to a 32-bit unsigned int (mhba->list_num_io) cast to
unsigned long * to find_first_bit() reads 8 bytes on 64-bit architectures.

On big-endian systems, this will incorrectly scan the adjacent 32-bit member
(ib_max_size) instead of list_num_io, writing incorrect ob_depth and ib_depth
capabilities to the firmware.

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