[PATCH] hw/ufs: avoid double unref of wrapped scsi-hd

Jia Jia posted 1 patch 1 week, 1 day ago
Failed in applying to current master (apply log)
hw/ufs/lu.c            |  5 +----
tests/qtest/ufs-test.c | 16 ++++++++++++++++
2 files changed, 17 insertions(+), 4 deletions(-)
[PATCH] hw/ufs: avoid double unref of wrapped scsi-hd
Posted by Jia Jia 1 week, 1 day ago
ufs_init_scsi_device() creates an internal scsi-hd and adds it as a
child of lu->bus. qdev_realize_and_unref() then drops the construction
reference, leaving the bus child ownership to tear it down.

ufs_lu_unrealize() still unrefs lu->scsi_dev directly. If the UFS
controller is ejected through ACPI PCI hotplug, the scsi-hd object can be
finalized there and then the bus child removal RCU callback later unrefs
the same object again.

Keep lu->scsi_dev as a borrowed pointer and clear it during unrealize
without unreffing it.

Add a qtest that ejects the UFS controller through the x86 ACPI PCI
hotplug eject register. On an ASAN build, the test reproduces the UAF
before the fix.

Fixes: 096434fea13a ("hw/ufs: Modify lu.c to share codes with SCSI subsystem")
Cc: qemu-stable@nongnu.org
Signed-off-by: Jia Jia <physicalmtea@gmail.com>
---
 hw/ufs/lu.c            |  5 +----
 tests/qtest/ufs-test.c | 16 ++++++++++++++++
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/hw/ufs/lu.c b/hw/ufs/lu.c
index f13fc6e342..41593a7117 100644
--- a/hw/ufs/lu.c
+++ b/hw/ufs/lu.c
@@ -516,10 +516,7 @@ static void ufs_lu_unrealize(DeviceState *dev)
 {
     UfsLu *lu = DO_UPCAST(UfsLu, qdev, dev);
 
-    if (lu->scsi_dev) {
-        object_unref(OBJECT(lu->scsi_dev));
-        lu->scsi_dev = NULL;
-    }
+    lu->scsi_dev = NULL;
 }
 
 static void ufs_lu_class_init(ObjectClass *oc, const void *data)
diff --git a/tests/qtest/ufs-test.c b/tests/qtest/ufs-test.c
index f677896db0..0ae03c3433 100644
--- a/tests/qtest/ufs-test.c
+++ b/tests/qtest/ufs-test.c
@@ -34,6 +34,8 @@
 #define TEST_QID 0
 #define QUEUE_SIZE 32
 #define UFS_MCQ_MAX_QNUM 32
+#define ACPI_PCIHP_ADDR 0xae00
+#define PCI_EJ_BASE 0x0008
 
 typedef struct QUfs QUfs;
 
@@ -635,6 +637,17 @@ static void ufstest_reg_read(void *obj, void *data, QGuestAllocator *alloc)
     qpci_iounmap(&ufs->dev, ufs->bar);
 }
 
+static void ufstest_acpi_eject(void *obj, void *data, QGuestAllocator *alloc)
+{
+    QUfs *ufs = obj;
+    QTestState *qts = ufs->dev.bus->qts;
+
+    qtest_outl(qts, ACPI_PCIHP_ADDR + PCI_EJ_BASE, 1 << 4);
+    qtest_qmp_assert_success(qts, "{ 'execute': 'query-status' }");
+    g_usleep(3 * G_USEC_PER_SEC);
+    qtest_qmp_assert_success(qts, "{ 'execute': 'query-status' }");
+}
+
 static void ufstest_init(void *obj, void *data, QGuestAllocator *alloc)
 {
     QUfs *ufs = obj;
@@ -1426,6 +1439,9 @@ static void ufs_register_nodes(void)
         g_test_message("Skipping ufs io tests for ppc64");
         return;
     }
+    if (!strcmp(arch, "i386") || !strcmp(arch, "x86_64")) {
+        qos_add_test("acpi-eject", "ufs", ufstest_acpi_eject, NULL);
+    }
     qos_add_test("init", "ufs", ufstest_init, NULL);
     qos_add_test("legacy-read-write", "ufs", ufstest_read_write, &io_test_opts);
     qos_add_test("mcq-read-write", "ufs", ufstest_read_write, &mcq_test_opts);
Re: [PATCH] hw/ufs: avoid double unref of wrapped scsi-hd
Posted by Jeuk Kim 4 days, 2 hours ago
On Sun, 31 May 2026 09:34:52 +0800, Jia Jia <physicalmtea@gmail.com> wrote:

Thanks for fixing the bug. The hw/ufs/lu.c change looks good to me.

One issue is in the new qtest. acpi-eject removes the UFS PCI device, but
qos-test may reuse the same QEMU instance for the next test when the command
line is unchanged. This makes the following UFS test run after the device has
already been ejected.

I can reproduce it with:

  QTEST_QEMU_BINARY=./build/qemu-system-x86_64 \
    build/tests/qtest/qos-test \
    -p /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/ufs/ufs-tests

acpi-eject passes, then init fails.

>
> diff --git a/tests/qtest/ufs-test.c b/tests/qtest/ufs-test.c
> index f677896db0..0ae03c3433 100644
> --- a/tests/qtest/ufs-test.c
> +++ b/tests/qtest/ufs-test.c
> @@ -1426,6 +1439,9 @@ static void ufs_register_nodes(void)
>          g_test_message("Skipping ufs io tests for ppc64");
>          return;
>      }
> +    if (!strcmp(arch, "i386") || !strcmp(arch, "x86_64")) {
> +        qos_add_test("acpi-eject", "ufs", ufstest_acpi_eject, NULL);

I think acpi-eject should be isolated, e.g. by registering it as a subprocess:

  QOSGraphTestOptions acpi_eject_test_opts = { .subprocess = true };

  qos_add_test("acpi-eject", "ufs", ufstest_acpi_eject,
               &acpi_eject_test_opts);

-- 
Jeuk Kim <jeuk20.kim@samsung.com>