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

[sashiko-bot@kernel.org]

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