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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] scsi: mvumi: unconditional 1us delay per completion for MV9580
- [High] scsi: mvumi: missing endianness conversion for PCI DMA structure fields
- [High] scsi: mvumi: tombstone tag 0xff collides with valid tags if max_io > 255
- [Medium] scsi: mvumi: missing dma_rmb() before reading DMA payload
--

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

scsi: mvumi: validate the response frame tag before using it

This commit fixes out-of-bounds array access and time-of-check to time-of-use
issues in the mvumi driver by ensuring the response frame tag and request ID
are fetched using READ_ONCE() before validation. It also adjusts tag bounds
checks to handle off-by-one errors and checks the tag against the array size it
indexes.

> diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
> index 8b8e5545747e8..2410d2aa147d9 100644
> --- a/drivers/scsi/mvumi.c
> +++ b/drivers/scsi/mvumi.c
> @@ -466,9 +466,9 @@ static char mvumi_check_ob_frame(struct mvumi_hba *mhba,
>  
>  	udelay(1);
>  	p_outb_frame = mhba->ob_list + cur_obf * mhba->ob_max_size;
> -	request_id = p_outb_frame->request_id;
> -	tag = p_outb_frame->tag;
> -	if (tag > mhba->tag_pool.size) {
> +	request_id = READ_ONCE(p_outb_frame->request_id);
> +	tag = READ_ONCE(p_outb_frame->tag);
> +	if (tag >= mhba->tag_pool.size) {

[Severity: High]
Since the hardware writes response frames to DMA memory in little-endian
format, does reading tag and request_id directly as native unsigned short
fields cause valid response frames to fail bounds checks on big-endian
architectures?

>  		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 = READ_ONCE(p_outb_frame->tag);
> +		if (unlikely(tag >= mhba->tag_pool.size ||
> +			     !mhba->tag_cmd[tag] ||
> +			     mhba->tag_cmd[tag]->request_id !=
> +			     READ_ONCE(p_outb_frame->request_id)))

[Severity: High]
For MV9580 controllers where request_id_enabled is 0, will this condition
almost always evaluate to true because the hardware does not reliably
populate request_id?

If so, this forces a fallback to mvumi_check_ob_frame() for every command
completion, causing an unconditional udelay(1) per command.

[Severity: Medium]
Does this code need a dma_rmb() before reading the DMA payload to ensure
weakly-ordered architectures don't speculatively execute the memcpy() payload
fetch before the preceding tag and request_id validity check completes?

Later in mvumi_receive_ob_list_entry(), the driver reads the payload:

        memcpy(ob_data->data, p_outb_frame, mhba->ob_max_size);

Without a memory barrier, fetching stale DMA payload data (which includes I/O
completion status and sense data) can result in incorrect I/O completion
handling or data corruption.

[Severity: High]
If the firmware negotiates a max_io greater than 255 (such as 512 or 1024),
could the hardcoded tombstone value of 0xff collide with a legitimate tag?

Further down in mvumi_receive_ob_list_entry(), the code does:

        p_outb_frame->tag = 0xff;

The hardware's copy pointer can advance before the DMA completes, meaning the
driver could read a stale tombstoned slot. If it sees tag 255, and tag 255 is
currently active with a matching request_id, the unlikely condition evaluates
to false. This bypasses the mvumi_check_ob_frame() delay loop entirely,
causing the driver to prematurely complete the active command with garbage
data.

>  			if (mvumi_check_ob_frame(mhba, cur_obf, p_outb_frame))
>  				continue;
>

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