Re: [PATCH v4] 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: infinite loop in mvumi_flush_cache()
- [High] scsi: mvumi: infinite loop and OOB read due to unvalidated frame_length
--

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

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

This commit adds bounds checks for firmware-supplied parameters to prevent
overflows and out-of-bounds accesses. It rejects invalid sizes for the entry
size, list depth, and allocation size, and limits sizes across host resets.

> diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
> index e70d336b4ab39..4fc0cf819c271 100644
> --- a/drivers/scsi/mvumi.c
> +++ b/drivers/scsi/mvumi.c
[ ... ]
> @@ -1048,30 +1061,80 @@ static int mvumi_hs_process_page(struct mvumi_hba *mhba,
>  	}
>  
>  	switch (hs_header->page_code) {

[Severity: High]
Does this code allow an infinite loop and out-of-bounds read if the firmware
provides a frame_length >= 256?

In mvumi_hs_process_page(), hs_header->frame_length is read directly from the
firmware DMA buffer and passed to mvumi_calculate_checksum() without
validation:

mvumi_hs_process_page() {
        ...
        page_checksum = mvumi_calculate_checksum(hs_header,
                                                hs_header->frame_length);
        ...
}

Looking at mvumi_calculate_checksum(), the loop counter i is an 8-bit
unsigned char:

mvumi_calculate_checksum() {
        unsigned char i;
        ...
        for (i = 0; i < len; i++) {
                ret ^= *ptr;
                ptr++;
        }
}

If frame_length >= 256, the condition i < len will always evaluate to true
since i wraps around to 0, causing the kernel to hang and read out of
bounds until a page fault occurs.

> -	case HS_PAGE_FIRM_CAP:
> +	case HS_PAGE_FIRM_CAP: {
> +		unsigned int new_max_io, new_ib_max_size, new_ob_max_size;
> +		unsigned int new_list_num_io;
> +		unsigned short new_max_target_id, new_capability;
>  
>  		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;
> -		mhba->hba_capability = hs_page1->capability;
> -		mhba->ib_max_size_setting = hs_page1->cl_in_max_entry_size;
> -		mhba->ib_max_size = (1 << hs_page1->cl_in_max_entry_size) << 2;
> +		/* Firmware-supplied shift counts: (1U << n) << 2 must fit. */
> +		if (hs_page1->cl_in_max_entry_size > 29 ||
> +		    hs_page1->cl_out_max_entry_size > 29) {
> +			dev_err(&mhba->pdev->dev,
> +				"invalid handshake entry size\n");
> +			return -1;
> +		}
>  
> -		mhba->ob_max_size_setting = hs_page1->cl_out_max_entry_size;
> -		mhba->ob_max_size = (1 << hs_page1->cl_out_max_entry_size) << 2;
> +		new_max_io = hs_page1->max_io_support;
> +		new_ib_max_size = (1U << hs_page1->cl_in_max_entry_size) << 2;
> +		new_ob_max_size = (1U << hs_page1->cl_out_max_entry_size) << 2;
> +		new_max_target_id = hs_page1->max_devices_support;
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Does this assignment lead to an infinite loop in mvumi_flush_cache() if
max_devices_support is greater than 255?

The new_max_target_id is derived from max_devices_support (which can be up to
65535) and is later assigned to mhba->max_target_id.

However, in mvumi_flush_cache(), the loop counter device_id is declared
as an 8-bit unsigned char:

mvumi_flush_cache() {
        unsigned char device_id;
        ...
        for (device_id = 0; device_id < mhba->max_target_id; device_id++) {
                if (!(mhba->target_map[device_id / bitcount] &
                                (1 << (device_id % bitcount))))
                        continue;
        ...
}

If max_devices_support is greater than 255, the condition
device_id < mhba->max_target_id will always be true, locking up the kernel
in an infinite loop.

> +		new_capability = hs_page1->capability;

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