Re: [PATCH v7 5/7] 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: newly added handshake bounds check breaks initialization on big-endian systems
--

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

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

This patch adds missing bounds checks in mvumi_hs_process_page() to reject
invalid hardware handshake sizes. It prevents buffer overflows when the
hardware reports excessively large list allocations, capability fields, or
device limits.

> 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
[ ... ]
> @@ -1054,30 +1067,115 @@ static int mvumi_hs_process_page(struct mvumi_hba *mhba,
[ ... ]
> +		/*
> +		 * The page is in coherent DMA memory, so take every field
> +		 * out of it once: a second fetch of a value that has already
> +		 * been checked would not be the value that was checked.
> +		 */
> +		in_sz = READ_ONCE(hs_page1->cl_in_max_entry_size);
> +		out_sz = READ_ONCE(hs_page1->cl_out_max_entry_size);
> +		list_depth = READ_ONCE(hs_page1->cl_inout_list_depth);
> +		new_max_io = READ_ONCE(hs_page1->max_io_support);
> +		new_max_target_id = READ_ONCE(hs_page1->max_devices_support);

[Severity: High]
Since hs_page1 is populated by the hardware via DMA, aren't these fields
inherently little-endian?

Reading hs_page1->max_devices_support directly into new_max_target_id without
le16_to_cpu() causes the bytes to be swapped on big-endian architectures.

> +		new_capability = READ_ONCE(hs_page1->capability);
> +		new_max_transfer_size = READ_ONCE(hs_page1->max_transfer_size);
> +
[ ... ]
> +		/*
> +		 * A target above MVUMI_MAX_TARGET_ID cannot be addressed: its
> +		 * id would land in the LUN byte of device_id and alias another
> +		 * device.  It is also what makes the loop counter in
> +		 * mvumi_flush_cache() wrap before it reaches the bound.
> +		 */
> +		if (new_max_target_id > MVUMI_MAX_TARGET_ID) {

[Severity: High]
Will this newly added bounds check incorrectly fail the probe on big-endian
systems?

A valid firmware-reported target count of 1 (0x0001) would be byte-swapped
to 256 (0x0100). Since MVUMI_MAX_TARGET_ID is 255, this check would evaluate
256 > 255 as true, falsely log "too many devices reported", and abort the
initialization by returning -1.

> +			dev_err(&mhba->pdev->dev,
> +				"too many devices reported: %u\n",
> +				new_max_target_id);
> +			return -1;
> +		}

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