Re: [PATCH v8 5/8] scsi: mvumi: reject handshake sizes that overflow the list allocations

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: mvumi: missing firmware abort write causes infinite retry loop
--

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

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

mvumi_hs_process_page() takes max_io, the two command entry sizes and the
list depth from the firmware handshake page. The entry sizes are u8 fields
used as shift counts, and 1 is an int, so a count of 29 or more makes the
<< 2 applied to the shift overflow an int; a count of 32 or more is
undefined outright.

> diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
> index 18964e860ac8e..8b8e5545747e8 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) {
> +		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;

[Severity: High]
If these new boundary checks fail and return -1, will it cause an infinite
retry loop?

When mvumi_hs_process_page() or mvumi_init_data() returns -1,
mvumi_handshake() sets hs_fun to HS_S_ABORT but returns immediately without
writing the abort state to the doorbell registers:

	if (mvumi_hs_process_page(mhba, hs_header)) {
		HS_SET_STATE(hs_fun, HS_S_ABORT);
		return -1;
	}

Because the firmware never receives the abort, it waits for a response and
eventually retries by asserting DRBL_HANDSHAKE_ISR.

The caller mvumi_handshake_event() then processes the interrupt, calls
mvumi_handshake() again, and resets the timeout:

	if (isr_status & DRBL_HANDSHAKE_ISR) {
		mhba->instancet->clear_intr(mhba);
		mvumi_handshake(mhba);
		before = jiffies;
	}

Doesn't this result in the driver probe hanging indefinitely on bad firmware
instead of timing out or failing cleanly?

> +	}
> +
>  	tmp_size = mhba->ib_max_size * mhba->max_io;
>  	if (mhba->hba_capability & HS_CAPABILITY_SUPPORT_DYN_SRC)
>  		tmp_size += sizeof(struct mvumi_dyn_list_entry) * mhba->max_io;

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