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