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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: mvumi: missing dma_rmb() causes speculative payload reads
--

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

    scsi: mvumi: validate the response frame tag before using it
    
    This commit fixes bounds checking and validates the response frame tag
    used in the outbound list. It also ensures the tag and request ID are
    retrieved using READ_ONCE() to prevent compiler optimizations that could
    lead to multiple fetches.

> 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
[ ... ]
> @@ -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)))
>  			if (mvumi_check_ob_frame(mhba, cur_obf, p_outb_frame))
>  				continue;
>  

[Severity: High]
Does this code need a dma_rmb() here?

The driver polls the completion tokens (tag and request_id) in DMA memory
here. A few lines below, the outbound DMA response frame is copied:

mvumi_receive_ob_list_entry() {
    ...
		tag = READ_ONCE(p_outb_frame->tag);
		if (unlikely(...))
			...
    ...
		memcpy(ob_data->data, p_outb_frame, mhba->ob_max_size);
    ...
}

Without a dma_rmb() after the completion tokens are validated, can the CPU
speculatively execute the memcpy() of the payload before reading the
completion token?

On weakly ordered architectures, the CPU may read stale values for
req_status or payload during the memcpy(). This could lead to silent data
corruption if a failed disk IO is incorrectly seen as successful.

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