Re: [PATCH v6 5/5] 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:
- [Medium] scsi: mvumi: incomplete TOCTOU fix missing request_id re-validation
--

commit 603dc78073e88003a67484fc0cd705e2144c5c40
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 accesses and NULL pointer
dereferences in mvumi_handle_clob() caused by a double-fetch/TOCTOU
vulnerability. It re-validates the response frame tag and command pointer
from the copied local buffer.

> diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
> index 2871b4c96f1c8..85624efcc1725 100644
> --- a/drivers/scsi/mvumi.c
> +++ b/drivers/scsi/mvumi.c

[ ... ]

> @@ -1878,6 +1880,7 @@ static void mvumi_handle_clob(struct mvumi_hba *mhba)
>  	struct mvumi_rsp_frame *ob_frame;
>  	struct mvumi_cmd *cmd;
>  	struct mvumi_ob_data *pool;
> +	unsigned short tag;
>  
>  	while (!list_empty(&mhba->free_ob_list)) {
>  		pool = list_first_entry(&mhba->free_ob_list,
> @@ -1886,11 +1889,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;
> +		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;
> +		}

[Severity: Medium]
Is a re-validation of the request_id also needed here?

The patch aims to prevent a double-fetch vulnerability by re-checking the
tag and command after copying the frame to the local ob_data buffer.
However, the request_id does not appear to be re-verified against
cmd->request_id.

If the firmware modifies the tag in the outbound list after the initial
check in mvumi_receive_ob_list_entry() but before the copy:

1. The initial validation passes with a valid tag and matching request_id.
2. The firmware changes the tag to point to a different, valid command.
3. The memcpy() grabs the modified tag into the local ob_data buffer.
4. mvumi_handle_clob() validates that the modified tag is in bounds and
   that the associated command exists.

Without verifying that ob_frame->request_id matches cmd->request_id on
this copied frame, does this regression allow the driver to blindly complete
the swapped command with the payload from this response, potentially causing
data corruption or logical errors?

>  
>  		atomic_dec(&mhba->fw_outstanding);
> -		mhba->tag_cmd[ob_frame->tag] = NULL;
> -		tag_release_one(mhba, &mhba->tag_pool, ob_frame->tag);
> +		mhba->tag_cmd[tag] = NULL;
> +		tag_release_one(mhba, &mhba->tag_pool, tag);
>  		if (cmd->scmd)
>  			mvumi_complete_cmd(mhba, cmd, ob_frame);
>  		else

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