Re: [PATCH v5 5/6] scsi: mvumi: validate the response frame tag before using it

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] scsi: mvumi: missing READ_ONCE() on DMA tag allows TOCTOU OOB access
- [High] scsi: mvumi: missing endianness conversions in DMA shared structs
- [High] scsi: mvumi: missing dma_rmb() before reading DMA ring entry
--

commit 8741f5e18a328ba9b868f0711acd0a3e2b7e9ee4
Author: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
Subject: scsi: mvumi: validate the response frame tag before using it

This patch adds bounds and NULL command checks on the response frame tag
used in mvumi_handle_clob, mvumi_receive_ob_list_entry, and
mvumi_check_ob_frame. It ensures the bounds checks include the tag pool
size, and explicitly checks the array element dereference.

> diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
> index e63b3ba7eaabd..07cfac9276e7b 100644
> --- a/drivers/scsi/mvumi.c
> +++ b/drivers/scsi/mvumi.c
> @@ -468,7 +468,7 @@ static char mvumi_check_ob_frame(struct mvumi_hba *mhba,
>  	p_outb_frame = mhba->ob_list + cur_obf * mhba->ob_max_size;
>  	request_id = p_outb_frame->request_id;
>  	tag = p_outb_frame->tag;

[Severity: High]
Does this read from DMA shared memory require a READ_ONCE to prevent a
TOCTOU double-fetch?

Without it, the compiler could potentially optimize this into multiple
fetches directly from memory. If a compromised or malicious device changes
the tag after the bounds check below, could it lead to an out-of-bounds
access on tag_cmd?

[Severity: High]
Does this function also need a dma_rmb before validating and returning?

The udelay(1) called earlier in this function works around the DMA visibility
race but does not provide the required memory barrier to prevent reordering of
subsequent payload reads.

> -	if (tag > mhba->tag_pool.size) {
> +	if (tag >= mhba->tag_pool.size) {
>  		dev_err(&mhba->pdev->dev, "ob frame data error\n");
>  		return -1;
>  	}

[ ... ]

> @@ -546,10 +547,11 @@ static void mvumi_receive_ob_list_entry(struct mvumi_hba *mhba)
>  		/* Copy pointer may point to entry in outbound list
>  		*  before entry has valid data
>  		*/
> -		if (unlikely(p_outb_frame->tag > mhba->tag_pool.size ||
> -			mhba->tag_cmd[p_outb_frame->tag] == NULL ||
> -			p_outb_frame->request_id !=
> -				mhba->tag_cmd[p_outb_frame->tag]->request_id))
> +		tag = p_outb_frame->tag;

[Severity: High]
Like in mvumi_check_ob_frame, does this read from DMA shared memory also
require a READ_ONCE to prevent a TOCTOU double-fetch?

Without READ_ONCE, the compiler might optimize this into multiple fetches
directly from memory, which could lead to an out-of-bounds access on tag_cmd
if the tag is changed by the device after the bounds check.

[Severity: High]
Additionally, is a dma_rmb needed after observing a valid tag?

On weakly ordered architectures, the subsequent memcpy of the frame payload
(which happens further down in this function) could be speculatively
reordered before the tag is validated, potentially capturing stale data.

[Severity: High]
Also, tag is a 16-bit field in mvumi_rsp_frame. Since it is being read using
native host endianness, will this result in byteswapped values on big-endian
architectures? This could cause huge swapped tag values to falsely fail the
bounds check.

> +		if (unlikely(tag >= mhba->tag_pool.size ||
> +			     !mhba->tag_cmd[tag] ||
> +			     mhba->tag_cmd[tag]->request_id !=
> +			     p_outb_frame->request_id))
>  			if (mvumi_check_ob_frame(mhba, cur_obf, p_outb_frame))
>  				continue;

[ ... ]

> @@ -1856,11 +1859,26 @@ static void mvumi_handle_clob(struct mvumi_hba *mhba)
>  		list_add_tail(&pool->list, &mhba->ob_data_list);
>  
>  		ob_frame = (struct mvumi_rsp_frame *) &pool->data[0];
> -		cmd = mhba->tag_cmd[ob_frame->tag];
> +		/*
> +		 * The tag was checked against the frame in the outbound list.
> +		 * This is the copy, so check the value that is used.
> +		 */
> +		tag = ob_frame->tag;

[Severity: High]
Will reading this 16-bit tag from the local copy of the frame using native
endianness cause false bounds check failures on big-endian hosts? It appears
an endianness conversion is missing here as well.

> +		if (tag >= mhba->tag_pool.size) {
> +			dev_err(&mhba->pdev->dev,
> +				"ob frame tag[0x%x] out of range\n", tag);
> +			continue;
> +		}
> +		cmd = mhba->tag_cmd[tag];
> +		if (!cmd) {
> +			dev_err(&mhba->pdev->dev,
> +				"ob frame tag[0x%x] with no command\n", tag);
> +			continue;
> +		}

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