Re: [PATCH 2/3] drm/nouveau: cancel the DP IRQ work before freeing the connector

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