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