Re: [PATCH 2/2] scsi: mvumi: quarantine a timed-out internal command's buffers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] scsi: mvumi: timed-out cmd buffers reused in
  mvumi_probe_devices()
It also noticed 2 related pre-existing issues this patch does not
introduce:
- [Critical] scsi: mvumi: DMA truncation and endian bug in
  mvumi_delete_internal_cmd() [1]
- [Critical] scsi: mvumi: double free in mvumi_issue_blocked_cmd() [2]
--

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

scsi: mvumi: quarantine a timed-out internal command's buffers

This commit marks internal commands that time out in
mvumi_issue_blocked_cmd() and places their DMA buffers on mhba->res_list in
mvumi_delete_internal_cmd() so they are not freed until driver unload.

> diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
> index 8d22d370d8608..dbbfa1cbae008 100644
> --- a/drivers/scsi/mvumi.c
> +++ b/drivers/scsi/mvumi.c

[ ... ]

> @@ -298,11 +321,19 @@ static void mvumi_delete_internal_cmd(struct mvumi_hba *mhba,
>  			phy_addr = (dma_addr_t) m_sg->baseaddr_l |
>  				(dma_addr_t) ((m_sg->baseaddr_h << 16) << 16);
>  
> -			dma_free_coherent(&mhba->pdev->dev, size, cmd->data_buf,
> -								phy_addr);
> +			if (cmd->timed_out)
> +				mvumi_quarantine_buf(mhba, cmd->data_buf,
> +						     phy_addr, size);
> +			else
> +				dma_free_coherent(&mhba->pdev->dev, size,
> +						  cmd->data_buf, phy_addr);

[Severity: Critical]
This problem wasn't introduced by this patch, but does reconstructing
phy_addr [1] truncate the upper 32 bits and miss endianness conversions?

m_sg->baseaddr_h is a u32, so (m_sg->baseaddr_h << 16) << 16 is evaluated in
32-bit arithmetic and zeroes the upper bits before the cast to dma_addr_t,
and neither baseaddr_l nor baseaddr_h is converted with le32_to_cpu().
Passing the corrupted phy_addr to dma_free_coherent() or
mvumi_quarantine_buf() causes IOMMU faults or frees the wrong DMA mapping on
64-bit or big-endian systems.

[ ... ]

> @@ -734,6 +765,7 @@ static int mvumi_issue_blocked_cmd(struct mvumi_hba *mhba,
>  	if (atomic_read(&cmd->sync_cmd)) {
>  		spin_lock_irqsave(&mhba->shost->host_lock, flags);
>  		atomic_dec(&cmd->sync_cmd);
> +		cmd->timed_out = 1;

[Severity: Medium]
Can mvumi_probe_devices() reuse cmd->frame and cmd->data_buf after a timeout
without quarantining them first?

mvumi_inquiry() only calls mvumi_delete_internal_cmd() when its caller passed
a NULL cmd, but its sole caller mvumi_probe_devices() allocates a single cmd
up front and reuses it across every target ID:

drivers/scsi/mvumi.c:mvumi_probe_devices() {
	cmd = mvumi_create_internal_cmd(mhba, 64);
	...
	for (id = 0; id < maxid; id++) {
		wwid = mvumi_inquiry(mhba, id, cmd);
		...
	}

	if (cmd)
		mvumi_delete_internal_cmd(mhba, cmd);
}

When mvumi_issue_blocked_cmd() times out on target id, the next loop
iteration zeroes cmd->data_buf, overwrites cmd->frame, pops the just-released
tag from mhba->tag_pool, and resubmits the same DMA buffers while the
controller may still DMA to or read from them for the timed-out command.
In addition, if mvumi_match_devices() returns -1 in that loop,
mvumi_probe_devices() returns early without calling
mvumi_delete_internal_cmd().

>  		if (mhba->tag_cmd[cmd->frame->tag]) {

[Severity: Critical]
This problem wasn't introduced by this patch, but can a completion arriving
just as wait_event_timeout() expires race with this timeout path [2]
and double-release cmd->frame->tag?

cmd->sync_cmd is checked before acquiring mhba->shost->host_lock and is not
rechecked under the lock:

CPU0                                  CPU1
mvumi_issue_blocked_cmd()
  wait_event_timeout(...)
  if (atomic_read(&cmd->sync_cmd))
                                      mvumi_isr_handler()
                                        spin_lock_irqsave(&host_lock, ...)
                                        ...
                                        atomic_dec(&cmd->sync_cmd)
                                        mhba->tag_cmd[tag] = NULL
                                        tag_release_one(..., tag)
                                        spin_unlock_irqrestore(&host_lock, ...)

                                      mvumi_queue_command()
                                        spin_lock_irqsave(&host_lock, ...)
                                        tag_get_one(...)  /* gets same tag */
                                        mhba->tag_cmd[tag] = new_cmd
                                        spin_unlock_irqrestore(&host_lock, ...)

    spin_lock_irqsave(&host_lock, ...)
    atomic_dec(&cmd->sync_cmd)
    if (mhba->tag_cmd[tag])
      mhba->tag_cmd[tag] = NULL
      tag_release_one(..., tag)  <-- frees new_cmd's active tag

Releasing an active tag back into mhba->tag_pool allows it to be handed out
to a third command while new_cmd is still in flight, corrupting tag_pool and
misdirecting command completions.

[1] https://sashiko.dev/#/bug/linux-76320be4-9016-4f09-ab08-37df27a6afc4
[2] https://sashiko.dev/#/bug/linux-23bd99c4-2d45-42f2-b477-ed58fe586f8f

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