[PATCH 0/2] scsi: mvumi: do not free buffers a timed-out command left with the device

[PATCH 0/2] scsi: mvumi: do not free buffers a timed-out command left with the device

[Yehyeong Lee]

This is the follow-up I promised when answering the review of v9 7/9,
https://lore.kernel.org/all/20261015055719.3992456-1-yhlee@isslab.korea.ac.kr/
and https://lore.kernel.org/all/sashiko-outbox-172292@kernel.org/

1/2 stops the controller bus-mastering before mvumi_detach_one() frees
the buffers it was given.  2/2 keeps a timed-out internal command's
buffers until then, rather than while the controller still has them.

Based on mainline; it conflicts textually with v9 7/9 of
https://lore.kernel.org/all/20261015045213.3975845-1-yhlee@isslab.korea.ac.kr/
in mvumi_delete_internal_cmd(), and I will rebase whichever goes in second.
I have no mvumi hardware; this was checked with a KUnit model.

Best regards,
Yehyeong Lee

Yehyeong Lee (2):
  scsi: mvumi: stop the controller bus-mastering before freeing its
    buffers
  scsi: mvumi: quarantine a timed-out internal command's buffers

 drivers/scsi/mvumi.c | 42 +++++++++++++++++++++++++++++++++++++-----
 drivers/scsi/mvumi.h |  1 +
 2 files changed, 38 insertions(+), 5 deletions(-)


base-commit: 97d73c8251226cf27cc6656b4c542869bb8c69fa
-- 
2.43.0

[PATCH 1/2] scsi: mvumi: stop the controller bus-mastering before freeing its buffers

[Yehyeong Lee]

mvumi_detach_one() frees the controller's buffers before stopping its DMA:

	mvumi_release_fw(mhba);		/* frees them */
	scsi_host_put(host);
	pci_disable_device(pdev);	/* stops the DMA */

mvumi_release_fw() frees everything on mhba->res_list and the handshake
page.  The controller is given those addresses during the handshake and
may read or write them for as long as it is bus-mastering, and only
pci_disable_device() stops that.

Disable the device first.  Nothing in mvumi_release_fw() needs it
enabled: it contains no ioread32() or iowrite32() at all.

I have no mvumi hardware.

Fixes: f0c568a478f0 ("[SCSI] mvumi: Add Marvell UMI driver")
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
Disabling the device before scsi_host_put() is the order myrb, myrs,
hptiop, wd719x, qla1280, dc395x, fnic, snic, atp870u and am53c974
already use.  Freeing first and disabling
afterwards is the more common order in drivers/scsi, because a driver with
nothing outstanding at teardown does not need the device stopped in order
to free safely.  mvumi cannot assume that, which is what 2/2 is about.

mvumi_probe_one()'s failure paths are left alone: they reach
pci_disable_device() from labels that have no Scsi_Host to put.
mvumi_shutdown(), mvumi_suspend() and mvumi_resume() call neither
pci_disable_device() nor scsi_host_put(), so nothing else moves.

 drivers/scsi/mvumi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
index 91c2db697c6913..8d22d370d8608f 100644
--- a/drivers/scsi/mvumi.c
+++ b/drivers/scsi/mvumi.c
@@ -2548,9 +2548,9 @@ static void mvumi_detach_one(struct pci_dev *pdev)
 
 	mhba->instancet->disable_intr(mhba);
 	free_irq(mhba->pdev->irq, mhba);
+	pci_disable_device(pdev);
 	mvumi_release_fw(mhba);
 	scsi_host_put(host);
-	pci_disable_device(pdev);
 	dev_dbg(&pdev->dev, "driver is removed!\n");
 }
 
-- 
2.43.0

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

[Yehyeong Lee]

mvumi_issue_blocked_cmd() waits 45 seconds for an internal command and,
if it has not completed, tidies up its own bookkeeping and returns 0, the
same value as success.  It writes nothing to the controller, so there is
no abort; the SCSI error handler is not involved either, because an
internal command has scmd == NULL.

The callers - mvumi_flush_cache(), mvumi_inquiry() and mvumi_get_event()
- cannot tell, and each goes on to mvumi_delete_internal_cmd(), which
frees the command frame and the data buffer.  The controller still has
both addresses: the frame as the source of a dynamic list entry on an
adapter with HS_CAPABILITY_SUPPORT_DYN_SRC, and the data buffer as the
target of the data-in transfer the frame's SGL describes.  One that
answers late writes into memory handed back to the page allocator.

Put both buffers on mhba->res_list instead.  The address and length are
taken where mvumi_delete_internal_cmd() takes them today, so nothing new
is trusted; mvumi_release_mem_resource() frees the list at unload, which
the preceding patch makes run after pci_disable_device().  The mark
cannot come from cmd_status, which is assigned from ob_frame->req_status
and can be left at REQ_STATUS_PENDING (0x80) by the controller.  The cost
is the frame and data buffer of each timed-out command, held until unload.

A KUnit test that writes to the buffers after the timeout, standing in
for the controller, gets a KASAN use-after-free in 5 of 5 runs without
this patch and none in 5 with it.  I have no mvumi hardware.

Fixes: f0c568a478f0 ("[SCSI] mvumi: Add Marvell UMI driver")
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
The faulting function below is the test, which performs the late write the
controller would make.

  ==================================================================
  BUG: KASAN: use-after-free in mvt_timeout_write+0x5f8/0x730
  Write of size 1 at addr ffff8880097b6000 by task kunit_try_catch/71

  CPU: 1 UID: 0 PID: 71 Comm: kunit_try_catch Tainted: G                 N  7.4.0-rc2-00563-g97d73c825122-dirty #2 PREEMPT(lazy) 
  Tainted: [N]=TEST
  Hardware name: QEMU Ubuntu 24.04 PC v2 (i440FX + PIIX, arch_caps fix, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
  Call Trace:
   <TASK>
   dump_stack_lvl+0x53/0x70
   print_report+0xd0/0x630
   ? __pfx__raw_spin_lock_irqsave+0x10/0x10
   ? mvt_timeout_write+0x5f8/0x730
   kasan_report+0xe5/0x120
   ? mvt_timeout_write+0x5f8/0x730
   mvt_timeout_write+0x5f8/0x730
   ? __pfx_mvt_timeout_write+0x10/0x10
   ? __set_cpus_allowed_ptr+0x178/0x230
   ? __pfx___set_cpus_allowed_ptr+0x10/0x10
   kunit_try_run_case+0x1ad/0x490
   ? __pfx_kunit_try_run_case+0x10/0x10
   ? kthread_affine_node+0x15f/0x1e0
   ? __pfx_kthread_affine_node+0x10/0x10
   ? __pfx_kunit_try_run_case+0x10/0x10
   ? __pfx_kunit_generic_run_threadfn_adapter+0x10/0x10
   kunit_generic_run_threadfn_adapter+0x7b/0xe0
   kthread+0x2c8/0x3b0
   ? recalc_sigpending+0x15c/0x1e0
   ? __pfx_kthread+0x10/0x10
   ret_from_fork+0x36e/0x5a0
   ? __pfx_ret_from_fork+0x10/0x10
   ? __switch_to+0xac3/0xdc0
   ? __switch_to_asm+0x33/0x70
   ? __pfx_kthread+0x10/0x10
   ret_from_fork_asm+0x1a/0x30
   </TASK>

  The buggy address belongs to the physical page:
  page: refcount:0 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x97b6
  flags: 0x100000000000000(node=0|zone=1)
  raw: 0100000000000000 ffffea000025edc8 ffffea000025ed48 0000000000000000
  raw: 0000000000000000 0000000000000000 00000000ffffffff 0000000000000000
  page dumped because: kasan: bad access detected

  Memory state around the buggy address:
   ffff8880097b5f00: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
   ffff8880097b5f80: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
  >ffff8880097b6000: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
                     ^
   ffff8880097b6080: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
   ffff8880097b6100: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
  ==================================================================

A late completion is already dropped: the timeout path clears tag_cmd[], and
both mvumi_receive_ob_list_entry() and mvumi_check_ob_frame() refuse a
response frame whose tag has no command.

The same kind of exposure remains for a normal SCSI command.
mvumi_timed_out() unmaps the scatterlist and returns the command without
stopping the device either, and this series does not address that.

mvumi_detach_one() does not flush the event work, so an event command
still running at unload is a separate, pre-existing problem.

 drivers/scsi/mvumi.c | 40 ++++++++++++++++++++++++++++++++++++----
 drivers/scsi/mvumi.h |  1 +
 2 files changed, 37 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
index 8d22d370d8608f..dbbfa1cbae0086 100644
--- a/drivers/scsi/mvumi.c
+++ b/drivers/scsi/mvumi.c
@@ -177,6 +177,29 @@ static void mvumi_release_mem_resource(struct mvumi_hba *mhba)
 	mhba->fw_flag &= ~MVUMI_FW_ALLOC;
 }
 
+/* The controller may still DMA here; free it once the device is disabled. */
+static void mvumi_quarantine_buf(struct mvumi_hba *mhba, void *virt_addr,
+				 dma_addr_t bus_addr, unsigned int size)
+{
+	struct mvumi_res *res = kzalloc_obj(*res);
+	unsigned long flags;
+
+	if (!res) {
+		dev_err(&mhba->pdev->dev,
+			"leaking %u bytes the controller may still write to\n",
+			size);
+		return;
+	}
+	res->type = RESOURCE_UNCACHED_MEMORY;
+	res->virt_addr = virt_addr;
+	res->bus_addr = bus_addr;
+	res->size = size;
+	INIT_LIST_HEAD(&res->entry);
+	spin_lock_irqsave(&mhba->shost->host_lock, flags);
+	list_add_tail(&res->entry, &mhba->res_list);
+	spin_unlock_irqrestore(&mhba->shost->host_lock, flags);
+}
+
 /**
  * mvumi_make_sgl -	Prepares  SGL
  * @mhba:		Adapter soft state
@@ -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);
 		}
-		dma_free_coherent(&mhba->pdev->dev, mhba->ib_max_size,
-				cmd->frame, cmd->frame_phys);
+		if (cmd->timed_out)
+			mvumi_quarantine_buf(mhba, cmd->frame, cmd->frame_phys,
+					     mhba->ib_max_size);
+		else
+			dma_free_coherent(&mhba->pdev->dev, mhba->ib_max_size,
+					  cmd->frame, cmd->frame_phys);
 		kfree(cmd);
 	}
 }
@@ -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;
 		if (mhba->tag_cmd[cmd->frame->tag]) {
 			mhba->tag_cmd[cmd->frame->tag] = NULL;
 			dev_warn(&mhba->pdev->dev, "TIMEOUT:release tag [%d]\n",
diff --git a/drivers/scsi/mvumi.h b/drivers/scsi/mvumi.h
index 1306a4abf19ad2..e248dfc1dd1bb7 100644
--- a/drivers/scsi/mvumi.h
+++ b/drivers/scsi/mvumi.h
@@ -252,6 +252,7 @@ struct mvumi_cmd {
 	void *data_buf;
 	unsigned short request_id;
 	unsigned char cmd_status;
+	unsigned char timed_out;
 };
 
 struct mvumi_cmd_priv {
-- 
2.43.0

Re: [PATCH 1/2] scsi: mvumi: stop the controller bus-mastering before freeing its buffers

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Critical] scsi: mvumi: use-after-free in mvumi_scan_events() [1]
--

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

scsi: mvumi: stop the controller bus-mastering before freeing its buffers

This commit moves pci_disable_device() ahead of mvumi_release_fw() and
scsi_host_put() in mvumi_detach_one() so bus-mastering is disabled before
freeing the controller's buffers.

> diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
> index 91c2db697c691..8d22d370d8608 100644
> --- a/drivers/scsi/mvumi.c
> +++ b/drivers/scsi/mvumi.c
> @@ -2548,9 +2548,9 @@ static void mvumi_detach_one(struct pci_dev *pdev)
>  
>  	mhba->instancet->disable_intr(mhba);
>  	free_irq(mhba->pdev->irq, mhba);
> +	pci_disable_device(pdev);
>  	mvumi_release_fw(mhba);
>  	scsi_host_put(host);
> -	pci_disable_device(pdev);

[Severity: Critical]
This problem wasn't introduced by this patch, but can mvumi_scan_events()
race with mvumi_detach_one() [1] and cause a use-after-free on mhba?

When an interrupt fires, mvumi_launch_events() allocates an mvumi_events
work item and schedules mvumi_scan_events() on the system workqueue.
mvumi_detach_one() frees the IRQ and drops the last reference to host
(which contains mhba) via scsi_host_put() without flushing or cancelling
pending event work items:

CPU0                              CPU1 (kworker)
mvumi_detach_one()
  free_irq(mhba->pdev->irq, mhba)
  mvumi_release_fw(mhba)
  scsi_host_put(host)
                                  mvumi_scan_events()
                                    mvumi_get_event(mu_ev->mhba, ...)
                                      <-- use-after-free

Flushing the scheduled event work after free_irq() and before tearing
down mhba and calling scsi_host_put() would avoid this.

[1] https://sashiko.dev/#/bug/linux-dec460b7-008c-478a-8da9-a7f1503e9e30

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

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

[sashiko-bot]

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