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

[PATCH v7] 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.

Masking after free_irq to avoid be overwritten by a concurrent
interrupt handler.

Cancel the work after freeing the IRQ to avoid a use-after-free.

qdev->ram_header set to NULL after iounmap to avoid dereference
the unmapped qdev->ram_header.

Added cancel_work_sync(&qdev->gc_work) after qxl_irq_fini() to
avoid a use-after-free when the pending work executes later.

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.

Changes in v7:
  - Fixed four issues reported by sashiko-bot@kernel.org:
    - Masking after free_irq to avoid be overwritten by a concurrent
      interrupt handler.
    - Cancel the work after freeing the IRQ to avoid a use-after-free.
    - qdev->ram_header set to NULL after iounmap to avoid dereference
      the unmapped qdev->ram_header.
    - Added cancel_work_sync(&qdev->gc_work) after qxl_irq_fini() to
      avoid a use-after-free when the pending work executes later.
---
 drivers/gpu/drm/qxl/qxl_drv.h |  1 +
 drivers/gpu/drm/qxl/qxl_irq.c | 15 +++++++++++++++
 drivers/gpu/drm/qxl/qxl_kms.c |  3 +++
 3 files changed, 19 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..7e3e80134562 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);
+
+	free_irq(pdev->irq, ddev);
+
+	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);
+}
diff --git a/drivers/gpu/drm/qxl/qxl_kms.c b/drivers/gpu/drm/qxl/qxl_kms.c
index 461b7ab9ad5c..872939414af4 100644
--- a/drivers/gpu/drm/qxl/qxl_kms.c
+++ b/drivers/gpu/drm/qxl/qxl_kms.c
@@ -310,6 +310,8 @@ void qxl_device_fini(struct qxl_device *qdev)
 	qxl_surf_evict(qdev);
 	qxl_vram_evict(qdev);
 
+	qxl_irq_fini(qdev);
+	cancel_work_sync(&qdev->gc_work);
 	qxl_gem_fini(qdev);
 	qxl_bo_fini(qdev);
 	qxl_ring_free(qdev->command_ring);
@@ -318,6 +320,7 @@ void qxl_device_fini(struct qxl_device *qdev)
 	io_mapping_free(qdev->surface_mapping);
 	io_mapping_free(qdev->vram_mapping);
 	iounmap(qdev->ram_header);
+	qdev->ram_header = NULL;
 	iounmap(qdev->rom);
 	qdev->rom = NULL;
 }
-- 
2.56.0

Re: [PATCH v7] 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: interrupt storm window due to free_irq() before device masking
- [High] drm/qxl: double free in qxl_device_fini() on probe error
- [High] drm/qxl: invalid MMIO access due to deferred free_irq() on PCI unbind
- [Medium] drm/qxl: memory leaks of client_monitors_config and IDRs on teardown
--

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

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

This patch aims to fix a use-after-free bug in the qxl_irq_handler by adding
a qxl_irq_fini() call to the device finalization path. It attempts to
properly tear down the IRQ and cancel related work when the device is
unbound.

>     Masking after free_irq to avoid be overwritten by a concurrent
>     interrupt handler.

Is this explanation correct?

As mentioned below, masking after free_irq() creates a window for an
interrupt storm on a shared PCI line.

If the concern is a concurrent handler re-enabling interrupts, should the
driver instead use a software teardown flag that the handler checks?

[ ... ]

> diff --git a/drivers/gpu/drm/qxl/qxl_irq.c b/drivers/gpu/drm/qxl/qxl_irq.c
> index 4018bcf808e51..7e3e80134562f 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);
> +
> +	free_irq(pdev->irq, ddev);

[Severity: High]
Does this sequence create a race window for an interrupt storm?

The QXL device uses a shared, level-triggered PCI interrupt line (IRQF_SHARED).

If free_irq() unregisters the QXL handler here, but the device is still
allowed to generate interrupts until int_mask is cleared below, couldn't
the hardware assert an interrupt in between?

Since the QXL handler is unregistered, no handler would acknowledge the
interrupt, leading to an interrupt storm that disables the shared line.

Should the hardware interrupt generation be masked before calling free_irq()?

> +
> +	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);
> +}
> diff --git a/drivers/gpu/drm/qxl/qxl_kms.c b/drivers/gpu/drm/qxl/qxl_kms.c
> index 461b7ab9ad5ca..872939414af43 100644
> --- a/drivers/gpu/drm/qxl/qxl_kms.c
> +++ b/drivers/gpu/drm/qxl/qxl_kms.c
> @@ -310,6 +310,8 @@ void qxl_device_fini(struct qxl_device *qdev)
>  	qxl_surf_evict(qdev);
>  	qxl_vram_evict(qdev);
>  
> +	qxl_irq_fini(qdev);

[Severity: High]
Is this the correct place to tear down the IRQ handler?

qxl_device_fini() is called from qxl_drm_release() when the last DRM file
descriptor is closed, not from the PCI unbind path.

If the PCI device is unbound via sysfs while userspace holds a DRM fd
open, the PCI hardware is disabled and unregistered from the bus, but
the IRQ handler remains registered until the fd is closed.

If a shared interrupt fires in the meantime, wouldn't qxl_irq_handler()
blindly access detached MMIO memory (qdev->ram_header->int_pending),
leading to an invalid hardware access?

> +	cancel_work_sync(&qdev->gc_work);
>  	qxl_gem_fini(qdev);
>  	qxl_bo_fini(qdev);
>  	qxl_ring_free(qdev->command_ring);
> @@ -318,6 +320,7 @@ void qxl_device_fini(struct qxl_device *qdev)
>  	io_mapping_free(qdev->surface_mapping);
>  	io_mapping_free(qdev->vram_mapping);
>  	iounmap(qdev->ram_header);
> +	qdev->ram_header = NULL;
>  	iounmap(qdev->rom);
>  	qdev->rom = NULL;
>  }

[Severity: High]
Could this lead to a double free on probe error?

When qxl_pci_probe() fails, the error path explicitly calls qxl_device_fini().

Then the driver core initiates devres cleanup, dropping the reference count
via drm_dev_put(), which triggers qxl_drm_release().

qxl_drm_release() calls qxl_device_fini() a second time.

Since qdev->gc_work.func is never zeroed out here, the early exit check
(!qdev->gc_work.func) at the start of qxl_device_fini() won't prevent a
second execution. This seems like it would cause double free_irq(), and
double frees for the rings and io mappings.

Should qdev->gc_work.func be cleared to NULL at the end of this function?

[Severity: Medium]
This problem wasn't introduced by this patch, but does this function leak
memory during teardown?

It looks like client_monitors_config (allocated via kzalloc_flex() in
qxl_alloc_client_monitors_config()) is never freed.

Additionally, release_idr and surf_id_idr are initialized with
idr_init_base() but do not have matching idr_destroy() calls here.

Could these resources be leaking their memory when the device is unbound?

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