Re: [PATCH v6 4/5] 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 READ_ONCE on DMA fields causes TOCTOU
--

commit 63940e006b3b39c47bbaae8a7b209645d7e0d499
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 9a3dde53dea97..2871b4c96f1c8 100644
> --- a/drivers/scsi/mvumi.c
> +++ b/drivers/scsi/mvumi.c

[ ... ]

> @@ -1054,30 +1067,108 @@ static int mvumi_hs_process_page(struct mvumi_hba *mhba,
>  	}
>  
>  	switch (hs_header->page_code) {
> -	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;
> +		u8 in_sz, out_sz, list_depth;
> +
>  		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;
> +		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);
> +
> +		/* Firmware-supplied shift counts: (1U << n) << 2 must fit. */
> +		if (in_sz > 29 || out_sz > 29) {
> +			dev_err(&mhba->pdev->dev,
> +				"invalid handshake entry size\n");
> +			return -1;
> +		}
> +
> +		new_max_io = hs_page1->max_io_support;
> +		new_ib_max_size = (1U << in_sz) << 2;
> +		new_ob_max_size = (1U << out_sz) << 2;
> +		new_max_target_id = hs_page1->max_devices_support;
> +		new_capability = hs_page1->capability;

[Severity: High]
This patch correctly adds READ_ONCE() for the shift counts, but is it possible
the remaining DMA-mapped fields are vulnerable to a Time-Of-Check to
Time-Of-Use (TOCTOU) race?

Because `hs_page1` resides in coherent DMA memory, the hardware can modify it
concurrently. The compiler is permitted to optimize away local variables like
`new_max_target_id` and reload them directly from `hs_page1` later in the
function.

> +
> +		/*
> +		 * 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) {
> +			dev_err(&mhba->pdev->dev,
> +				"too many devices reported: %u\n",
> +				new_max_target_id);
> +			return -1;
> +		}
> +
> +		/* An entry has to hold the frame the driver puts in it. */

[ ... ]

> +		/* A host reset does not reallocate, so the sizes have to stay
> +		 * the ones the buffers were allocated with.  Only check that
> +		 * the firmware is not asking for more than they hold.
> +		 */
> +		if (mhba->fw_flag & MVUMI_FW_ALLOC) {
> +			if (new_max_io > mhba->max_io ||
> +			    new_ib_max_size > mhba->ib_max_size ||
> +			    new_ob_max_size > mhba->ob_max_size ||
> +			    new_list_num_io > mhba->list_num_io ||
> +			    new_max_target_id > mhba->max_target_id) {
> +				dev_err(&mhba->pdev->dev,
> +					"handshake sizes grew across reset\n");
> +				return -1;
> +			}
> +			break;
> +		}
> +
> +		mhba->max_io = new_max_io;
> +		mhba->list_num_io = new_list_num_io;
> +		mhba->max_transfer_size = hs_page1->max_transfer_size;
> +		mhba->max_target_id = new_max_target_id;
> +		mhba->hba_capability = new_capability;

If the compiler rematerializes `new_max_target_id` by doing a read from
`hs_page1->max_devices_support` here during the assignment, could a malicious
or malfunctioning device have changed it after the bounds check?

This would cause `mhba->max_target_id` to exceed 255, leading to the target
aliasing and loop wrap-around vulnerabilities this patch intended to prevent in
mvumi_flush_cache().

Should READ_ONCE() also be used when reading `max_io_support`,
`max_devices_support`, and `capability` from `hs_page1`?

> +		mhba->ib_max_size_setting = in_sz;
> +		mhba->ib_max_size = new_ib_max_size;
> +		mhba->ob_max_size_setting = out_sz;
> +		mhba->ob_max_size = new_ob_max_size;
>  
>  		if (mhba->hba_capability & HS_CAPABILITY_SUPPORT_COMPACT_SG)
>  			mhba->eot_flag = 22;
>  		else
>  			mhba->eot_flag = 27;
> -		if (mhba->hba_capability & HS_CAPABILITY_NEW_PAGE_IO_DEPTH_DEF)
> -			mhba->list_num_io = 1 << hs_page1->cl_inout_list_depth;
>  		break;
> +	}
>  	default:
>  		dev_err(&mhba->pdev->dev, "handshake: page code error\n");
>  		return -1;

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