[PATCH v5] drm/qxl: fix use-after-free in qxl_irq_handler on PCI

[PATCH v5] drm/qxl: fix use-after-free in qxl_irq_handler on PCI

[Óscar Megía López]

Executing below script:

i=0;\
while [ 1 -eq 1 ]; do\
  i=$((i+1)); echo 0000:00:01.0 > /sys/bus/pci/drivers/qxl/unbind;\
  if (($i%1000==0)); then\
      echo loops=$i; free;\
      grep nr_free_pages /proc/vmstat;\
      grep -E "VmallocUsed|Slab|Reclaimable|SUnreclaim" /proc/meminfo;\
      sync; echo 3 > /proc/sys/vm/drop_caches;\
      echo 1 > /proc/sys/vm/compact_memory;\
      udevadm settle;\
      free;\
      grep nr_free_pages /proc/vmstat;\
      grep -E "VmallocUsed|Slab|Reclaimable|SUnreclaim" /proc/meminfo;\
      uptime;\
  fi;\
  echo 0000:00:01.0 > /sys/bus/pci/drivers/qxl/bind;\
done

After a few seconds, it reports:

==================================================================
BUG: KASAN: slab-use-after-free in qxl_irq_handler+0x269/0x2b0
Read of size 8 at addr ffff888001c6cd48 by task swapper/0/0

CPU: 0 UID: 0 PID: 0 Comm: swapper/0 Not tainted
     7.1.0-10963-g1a3746ccbb0a #31 PREEMPT(lazy)
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009),
               BIOS Arch Linux 1.17.0-2-2 04/01/2014
Call Trace:
 <IRQ>
 dump_stack_lvl+0x4d/0x70
 print_report+0x14b/0x4b0
 ? __pfx__raw_spin_lock_irqsave+0x10/0x10
 ? profile_tick+0x56/0x90
 ? tick_nohz_handler+0x23c/0x5c0
 kasan_report+0x117/0x140
 ? qxl_irq_handler+0x269/0x2b0
 ? qxl_irq_handler+0x269/0x2b0
 ? __pfx_qxl_irq_handler+0x10/0x10
 qxl_irq_handler+0x269/0x2b0
 ? __pfx_qxl_irq_handler+0x10/0x10
 ? __pfx_qxl_irq_handler+0x10/0x10
 __handle_irq_event_percpu+0x116/0x450
 ? __pfx__raw_spin_lock+0x10/0x10
 handle_irq_event+0xa6/0x1c0
 handle_fasteoi_irq+0x271/0xb10
 ? __pfx_handle_fasteoi_irq+0x10/0x10
 __common_interrupt+0x60/0x130
 common_interrupt+0x7a/0x90
 </IRQ>
 <TASK>
  asm_common_interrupt+0x26/0x40
 RIP: 0010:pv_native_safe_halt+0xf/0x20
 Code: 42 de 00 c3 cc cc cc cc 0f 1f 00 90 90 90 90 90 90 90 90 90
       90 90 90 90 90 90 90 f3 0f 1e fa eb 07 0f 00 2d a3 cf 20 00
       fb f4 <c3> cc cc cc cc 66 2e 0f 1f 84 00 00 00 00 00 66 90
       90 90 90 90 90
 RSP: 0018:ffffffffb8207e48 EFLAGS: 00000206
 RAX: ffff8880b296f000 RBX: ffffffffb82146c0 RCX: 0000000000000001
 RDX: 0000000000000001 RSI: 0000000000000004 RDI: 0000000000067a04
 RBP: fffffbfff70428d8 R08: ffffffffb7247e1d R09: 1ffff1100d846202
 R10: ffffed100d846203 R11: ffffed100d846203 R12: 0000000000000000
 R13: 0000000000000000 R14: 1ffffffff7040fcd R15: dffffc0000000000
  ? ct_kernel_exit.constprop.0+0x9d/0xc0
  default_idle+0x9/0x10
  default_idle_call+0x37/0x60
  do_idle+0x3a8/0x5d0
  ? __pfx___schedule+0x10/0x10
  ? __pfx_do_idle+0x10/0x10
  cpu_startup_entry+0x4e/0x60
  rest_init+0x11a/0x120
  start_kernel+0x382/0x390
  x86_64_start_reservations+0x24/0x30
  x86_64_start_kernel+0xd6/0xe0
  common_startup_64+0x13e/0x158
  </TASK>

in v4, disable_irq() was introduced, but because qxl requests its IRQ with
IRQF_SHARED, calling disable_irq() masks the interrupt line globally at the
controller level (IO-APIC), potentially hanging other devices sharing the
same line (e.g., virtio devices). Furthermore, placing free_irq() too early
caused hardware teardown timeouts (e.g., waiting on qdev->release_event in
qxl_device_fini()).

Fix this by:
1. Masking interrupts specifically on the QXL hardware via ram_header->int_mask = 0
   and flushing pending work before calling free_irq().
2. Placing qxl_irq_fini() in qxl_pci_remove() after drm_atomic_helper_shutdown()
   so completion events are handled during atomic shutdown, but before releasing
   legacy VGA resources and device structures.

Assisted-by: OpenCode:1.17.13-Big Pickle/DeepSeek V4 Flash
Fixes: 48bd85808443 ("drm/qxl: Convert to Linux IRQ interfaces")
Signed-off-by: Óscar Megía López <megia.oscar@gmail.com>
---
Changes in v2:
  - Updated qxl_ttm_init to add ttm_device_fini on error in
    qxl_ttm_init_mem_type.

Changes in v3:
  - Added free_irq on qxl_probe unload.
  - Set to NULL after free on qxl_device_fini and
    added idr_destroy on release_idr and surf_id_idr.
  - Free client_monitors_config.

Changes in v4:
  - Delete all code unnecessary. Leave only code to fix the BUG.

Changes in v5:
  - Replace disable_irq() with device-specific interrupt masking via
    qdev->ram_header->int_mask = 0 to prevent global masking on IRQF_SHARED lines.
  - Reorder qxl_irq_fini() call in qxl_pci_remove() to ensure hardware teardown
    events (like release_event in qxl_device_fini) complete before free_irq().
---
 drivers/gpu/drm/qxl/qxl_drv.c |  2 ++
 drivers/gpu/drm/qxl/qxl_drv.h |  1 +
 drivers/gpu/drm/qxl/qxl_irq.c | 13 +++++++++++++
 3 files changed, 16 insertions(+)

diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
index 1e6a2392d7c6..3ec2f9395439 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.c
+++ b/drivers/gpu/drm/qxl/qxl_drv.c
@@ -154,10 +154,12 @@ static void
 qxl_pci_remove(struct pci_dev *pdev)
 {
 	struct drm_device *dev = pci_get_drvdata(pdev);
+	struct qxl_device *qdev = to_qxl(dev);
 
 	drm_kms_helper_poll_fini(dev);
 	drm_dev_unregister(dev);
 	drm_atomic_helper_shutdown(dev);
+	qxl_irq_fini(qdev);
 	if (pci_is_vga(pdev) && pdev->revision < 5)
 		vga_put(pdev, VGA_RSRC_LEGACY_IO);
 }
diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
index cc02b5f10ad9..e8598fdbe886 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.h
+++ b/drivers/gpu/drm/qxl/qxl_drv.h
@@ -424,6 +424,7 @@ void qxl_gem_prime_vunmap(struct drm_gem_object *obj,
 
 /* qxl_irq.c */
 int qxl_irq_init(struct qxl_device *qdev);
+void qxl_irq_fini(struct qxl_device *qdev);
 
 void qxl_debugfs_add_files(struct qxl_device *qdev,
 			   struct drm_info_list *files,
diff --git a/drivers/gpu/drm/qxl/qxl_irq.c b/drivers/gpu/drm/qxl/qxl_irq.c
index 4018bcf808e5..9d2311b15660 100644
--- a/drivers/gpu/drm/qxl/qxl_irq.c
+++ b/drivers/gpu/drm/qxl/qxl_irq.c
@@ -105,3 +105,16 @@ int qxl_irq_init(struct qxl_device *qdev)
 	}
 	return 0;
 }
+
+void qxl_irq_fini(struct qxl_device *qdev)
+{
+	struct drm_device *ddev = &qdev->ddev;
+	struct pci_dev *pdev = to_pci_dev(ddev->dev);
+
+	if (qdev->ram_header)
+		qdev->ram_header->int_mask = 0;
+
+	cancel_work_sync(&qdev->client_monitors_config_work);
+
+	free_irq(pdev->irq, ddev);
+}
-- 
2.55.0

Re: [PATCH v5] drm/qxl: fix use-after-free in qxl_irq_handler on PCI

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] drm/qxl: use-after-free on qdev in client_monitors_config_work
- [High] drm/qxl: IRQ storm on shared interrupt line due to race
- [Medium] drm/qxl: guaranteed 1-second timeout delay on device unbind
--

commit 4e30a9d9b42421b43ae1402ef7242873b17ea62b
Author: Óscar Megía López <megia.oscar@gmail.com>

drm/qxl: fix use-after-free in qxl_irq_handler on PCI

This commit aims to fix a slab-use-after-free in the qxl_irq_handler during
unbind. It masks interrupts on the QXL hardware and reorders the IRQ cleanup
logic to be handled in qxl_pci_remove() before legacy VGA resources are
released.

> diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
> --- a/drivers/gpu/drm/qxl/qxl_drv.c
> +++ b/drivers/gpu/drm/qxl/qxl_drv.c
> @@ -159,10 +159,12 @@ static void
>  qxl_pci_remove(struct pci_dev *pdev)
>  {
>  	struct drm_device *dev = pci_get_drvdata(pdev);
> +	struct qxl_device *qdev = to_qxl(dev);
>  
>  	drm_kms_helper_poll_fini(dev);
>  	drm_dev_unregister(dev);
>  	drm_atomic_helper_shutdown(dev);
> +	qxl_irq_fini(qdev);

[Severity: Medium]
Does placing qxl_irq_fini() here cause a guaranteed timeout during unbind?

Later in the teardown process, the DRM release callback qxl_drm_release()
calls qxl_device_fini(), which invokes qxl_io_notify_oom() and waits for
the hardware to signal completion via an interrupt:

drivers/gpu/drm/qxl/qxl_kms.c:qxl_device_fini() {
    ...
    qxl_io_notify_oom(qdev);
    wait_event_timeout(qdev->release_event,
                       atomic_read(&qdev->release_count) == 0,
                       HZ);
    ...
}

Since the interrupt handler is permanently unregistered at this point,
the interrupt is never processed. Will wait_event_timeout() unconditionally
hit its 1-second timeout on every unbind?

>  	if (pci_is_vga(pdev) && pdev->revision < 5)
>  		vga_put(pdev, VGA_RSRC_LEGACY_IO);
>  }

[ ... ]

> diff --git a/drivers/gpu/drm/qxl/qxl_irq.c b/drivers/gpu/drm/qxl/qxl_irq.c
> --- a/drivers/gpu/drm/qxl/qxl_irq.c
> +++ b/drivers/gpu/drm/qxl/qxl_irq.c
> @@ -105,3 +105,16 @@ int qxl_irq_init(struct qxl_device *qdev)
>  	}
>  	return 0;
>  }
> +
> +void qxl_irq_fini(struct qxl_device *qdev)
> +{
> +	struct drm_device *ddev = &qdev->ddev;
> +	struct pci_dev *pdev = to_pci_dev(ddev->dev);
> +
> +	if (qdev->ram_header)
> +		qdev->ram_header->int_mask = 0;

[Severity: High]
Can this race with the interrupt handler and cause an IRQ storm?

While this clears the interrupt mask, it does not notify the hardware via an
I/O update. If an interrupt arrives right before this, qxl_irq_handler()
runs concurrently:

drivers/gpu/drm/qxl/qxl_irq.c:qxl_irq_handler() {
    ...
    qdev->ram_header->int_mask = QXL_INTERRUPT_MASK;
    outb(0, qdev->io_base + QXL_IO_UPDATE_IRQ);
    ...
}

The handler will unconditionally set the mask back to QXL_INTERRUPT_MASK
and re-enable interrupts at the hardware level.

When free_irq() subsequently removes the handler, could the hardware be left
actively asserting interrupts on a shared PCI line with no handler registered
to clear them, potentially breaking other devices on the same line?

> +
> +	cancel_work_sync(&qdev->client_monitors_config_work);

[Severity: High]
Does cancelling the work before freeing the IRQ create a race condition that
leads to a use-after-free?

Because the IRQ handler is still active when cancel_work_sync() is called,
an arriving interrupt can trigger qxl_irq_handler() which schedules the work
again:

drivers/gpu/drm/qxl/qxl_irq.c:qxl_irq_handler() {
    ...
    if (pending & QXL_INTERRUPT_CLIENT_MONITORS_CONFIG) {
        schedule_work(&qdev->client_monitors_config_work);
    }
    ...
}

If the work is re-queued after cancel_work_sync() returns, qdev will
eventually be freed via devres during device removal. When the workqueue
later processes the dangling work item, will it access the freed memory?

> +
> +	free_irq(pdev->irq, ddev);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001184321.80615-1-megia.oscar@gmail.com?part=1