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 <[email protected]> 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/[email protected]?part=1
