Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [High] Use-After-Free / Memory Leak in nouveau_connector_create() on LVDS initialization error path. - [High] NULL pointer dereference in nouveau_dp_irq() when initializing the drm variable. - [High] Use-After-Free on the drm object due to late irq_work cancellation. -- --- Patch [2]: [PATCH 2/3] drm/nouveau: cancel the DP IRQ work before freeing the connector --- commit f68e09b38970f3b87ae7ed5977b718c0b00485df Author: Marek Czernohous <[email protected]> drm/nouveau: cancel the DP IRQ work before freeing the connector This patch adds a cancel_work_sync() call to nouveau_connector_destroy() to ensure the DP IRQ work is cancelled before the connector is freed. This prevents a use-after-free where the irq_work remains on the system queue past the kfree(). Link: https://sashiko.dev/#/patchset/[email protected]?part=1 > diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c > b/drivers/gpu/drm/nouveau/nouveau_connector.c > index b0b0ad9a0c248..e49dcaa6d210b 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_connector.c > +++ b/drivers/gpu/drm/nouveau/nouveau_connector.c > @@ -397,6 +397,7 @@ nouveau_connector_destroy(struct drm_connector *connector) > struct nouveau_connector *nv_connector = nouveau_connector(connector); > nvif_event_dtor(&nv_connector->irq); > nvif_event_dtor(&nv_connector->hpd); > + cancel_work_sync(&nv_connector->irq_work); [Severity: High] This isn't a bug introduced by this patch, but does cancelling the work here leave a race condition that causes a use-after-free on the drm object? During driver teardown, nouveau_display_fini() cancels drm->hpd_work. Later, drm_mode_config_cleanup() calls nouveau_connector_destroy(), which now waits on nv_connector->irq_work here. If irq_work is actively running on a worker thread when this executes, cancel_work_sync() will block. While finishing, nouveau_dp_irq() calls nouveau_connector_hpd(), which unconditionally schedules drm->hpd_work: nouveau_dp_irq() { ... nouveau_connector_hpd(nv_connector, NVIF_CONN_EVENT_V0_IRQ | hpd); } Since drm->hpd_work was already cancelled and is never cancelled again, it is placed back on the system workqueue. After teardown completes and the drm object is freed in nouveau_drm_device_remove(), the workqueue could execute nouveau_display_hpd_work(), dereferencing the freed pointer. Should this cancellation happen earlier, such as in nouveau_display_fini()? [Severity: High] This is a pre-existing issue, but looking at nouveau_connector_create() in nouveau_connector.c, does the LVDS initialization error path leak the connector and cause a use-after-free? nouveau_connector_create() { ... drm_connector_init(dev, &nv_connector->base, funcs, type); ... if (type == DRM_MODE_CONNECTOR_LVDS) { ... ret = nouveau_bios_parse_lvds_table(dev, 0, &dummy, &dummy); if (ret) { NV_ERROR(drm, "Error parsing LVDS table, disabling\n"); kfree(nv_connector); return ERR_PTR(ret); } ... } drm_connector_init() adds the connector to dev->mode_config.connector_list and allocates an IDA. If the LVDS table parsing fails, the error path frees nv_connector with kfree() but doesn't call drm_connector_cleanup(). Does this leave a dangling pointer in the global connector list that will crash when traversed, and leak the allocated IDA? [Severity: High] This is a pre-existing issue, but looking at nouveau_dp_irq() in nouveau_dp.c, can we encounter a NULL pointer dereference here? nouveau_dp_irq() { ... struct nouveau_encoder *outp = find_encoder(connector, DCB_OUTPUT_DP); struct nouveau_drm *drm = nouveau_drm(outp->base.base.dev); ... if (!outp) return; ... } find_encoder() can return NULL if a valid DisplayPort encoder isn't found, which the code anticipates. However, outp is immediately dereferenced during variable initialization to fetch the dev pointer for nouveau_drm() before the null check executes. Can this cause a kernel oops if outp is NULL? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
