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

[PATCH v6] 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>

qxl_device_fini() tears down GEM, buffer objects and command rings,
but it never releases the IRQ resources allocated during device
initialization. This leaves the IRQ handler and its associated state
alive after the device has been finalized, which can lead to use-after-
free if an interrupt fires during or after teardown.

Add the missing qxl_irq_fini() call to the finalization path, placed
before qxl_gem_fini() so that no IRQ can be delivered while GEM and
buffer object resources are being freed.

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().

Changes in v6:
  - qxl_irq_fini() call in qxl_pci_remove() do not free all resources
    if your run unbind after end xorg. Place qxl_irq_fini() in
    qxl_device_fini() before GEM and buffer object resources are being
    freed is the correct place to release IRQ resources.
---
 drivers/gpu/drm/qxl/qxl_drv.h |  1 +
 drivers/gpu/drm/qxl/qxl_irq.c | 15 +++++++++++++++
 drivers/gpu/drm/qxl/qxl_kms.c |  1 +
 3 files changed, 17 insertions(+)

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..3698428b50ef 100644
--- a/drivers/gpu/drm/qxl/qxl_irq.c
+++ b/drivers/gpu/drm/qxl/qxl_irq.c
@@ -105,3 +105,18 @@ 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;
+		outb(0, qdev->io_base + QXL_IO_UPDATE_IRQ);
+	}
+
+	cancel_work_sync(&qdev->client_monitors_config_work);
+
+	free_irq(pdev->irq, ddev);
+}
diff --git a/drivers/gpu/drm/qxl/qxl_kms.c b/drivers/gpu/drm/qxl/qxl_kms.c
index 461b7ab9ad5c..fc2502752ae2 100644
--- a/drivers/gpu/drm/qxl/qxl_kms.c
+++ b/drivers/gpu/drm/qxl/qxl_kms.c
@@ -310,6 +310,7 @@ void qxl_device_fini(struct qxl_device *qdev)
 	qxl_surf_evict(qdev);
 	qxl_vram_evict(qdev);
 
+	qxl_irq_fini(qdev);
 	qxl_gem_fini(qdev);
 	qxl_bo_fini(qdev);
 	qxl_ring_free(qdev->command_ring);
-- 
2.56.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] drm/qxl: Race in qxl_irq_fini() leaves hardware interrupts unmasked
- [High] drm/qxl: UAF of qdev due to cancel_work_sync() called before free_irq()
- [High] drm/qxl: UAF of qdev via gc_work due to missing cancel_work_sync()
- [High] drm/qxl: double execution of qxl_irq_fini on probe error path causes UAF
--

commit b941d125ca70f91e048fd38f98c81dd403aa9b7d
Author: Óscar Megía López <megia.oscar@gmail.com>
Subject: drm/qxl: fix use-after-free in qxl_irq_handler on PCI

This patch attempts to fix a use-after-free in qxl_irq_handler by tearing
down the IRQ resources during qxl_device_fini(). It adds qxl_irq_fini() to
mask the interrupt, cancel the pending monitor configuration work, and free
the IRQ.

[ ... ]
> --- a/drivers/gpu/drm/qxl/qxl_irq.c
> +++ b/drivers/gpu/drm/qxl/qxl_irq.c
[ ... ]
> @@ -105,3 +105,18 @@ 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;
> +		outb(0, qdev->io_base + QXL_IO_UPDATE_IRQ);
> +	}

[Severity: High]
Can this masking be overwritten by a concurrent interrupt handler?

If qxl_irq_handler() executes on another CPU concurrently, it
unconditionally writes QXL_INTERRUPT_MASK to int_mask and
QXL_IO_UPDATE_IRQ before returning. If that happens after this block
executes, the hardware will remain armed even after free_irq() unregisters
the handler, which might cause an unhandled IRQ storm.

> +
> +	cancel_work_sync(&qdev->client_monitors_config_work);
> +
> +	free_irq(pdev->irq, ddev);

[Severity: High]
Is it safe to cancel the work before freeing the IRQ?

Because free_irq() hasn't executed yet, a concurrent interrupt could
trigger qxl_irq_handler(). The handler might call:
schedule_work(&qdev->client_monitors_config_work)

If this happens after cancel_work_sync() has already returned, it would
leave the work queued after qdev is freed, resulting in a use-after-free.

> +}
> --- a/drivers/gpu/drm/qxl/qxl_kms.c
> +++ b/drivers/gpu/drm/qxl/qxl_kms.c
> @@ -310,6 +310,7 @@ void qxl_device_fini(struct qxl_device *qdev)
>  	qxl_surf_evict(qdev);
>  	qxl_vram_evict(qdev);
>  
> +	qxl_irq_fini(qdev);

[Severity: High]
Is there a risk of a page fault here during probe failure?

If qxl_pci_probe() fails, the error path manually calls qxl_device_fini().
Then, devres cleanup triggers qxl_drm_release() which executes
qxl_device_fini() a second time.

The first execution unmaps qdev->ram_header but doesn't set it to NULL.
When the second execution calls qxl_irq_fini(), won't it dereference the
unmapped qdev->ram_header?

[Severity: High]
Will qdev->gc_work remain pending after device teardown?

Earlier in qxl_device_fini(), flush_work(&qdev->gc_work) is called,
followed by qxl_surf_evict() and qxl_vram_evict(). These evictions can
push commands to the hardware, resulting in new interrupts.

Since qxl_irq_fini() hasn't been called yet, qxl_irq_handler() processes
these interrupts and calls qxl_queue_garbage_collect(), which re-schedules
gc_work. Since cancel_work_sync(&qdev->gc_work) is never called, couldn't
this lead to a use-after-free when the pending work executes later?

>  	qxl_gem_fini(qdev);
>  	qxl_bo_fini(qdev);
>  	qxl_ring_free(qdev->command_ring);

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