* [PATCH 1/2] scsi: mvumi: stop the controller bus-mastering before freeing its buffers
2026-11-14 1:44 [PATCH 0/2] scsi: mvumi: do not free buffers a timed-out command left with the device Yehyeong Lee
@ 2026-11-14 1:44 ` Yehyeong Lee
2026-11-14 1:53 ` sashiko-bot
2026-11-14 1:44 ` [PATCH 2/2] scsi: mvumi: quarantine a timed-out internal command's buffers Yehyeong Lee
1 sibling, 1 reply; 5+ messages in thread
From: Yehyeong Lee @ 2026-11-14 1:44 UTC (permalink / raw)
To: mkp, James.Bottomley; +Cc: linux-scsi, linux-kernel
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
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH 2/2] scsi: mvumi: quarantine a timed-out internal command's buffers
2026-11-14 1:44 [PATCH 0/2] scsi: mvumi: do not free buffers a timed-out command left with the device Yehyeong Lee
2026-11-14 1:44 ` [PATCH 1/2] scsi: mvumi: stop the controller bus-mastering before freeing its buffers Yehyeong Lee
@ 2026-11-14 1:44 ` Yehyeong Lee
2026-11-14 2:05 ` sashiko-bot
1 sibling, 1 reply; 5+ messages in thread
From: Yehyeong Lee @ 2026-11-14 1:44 UTC (permalink / raw)
To: mkp, James.Bottomley; +Cc: linux-scsi, linux-kernel
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
^ permalink raw reply related [flat|nested] 5+ messages in thread