Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] The patch adds a teardown flag to prevent internal requeuing of works in `hdcp_destroy()`, but fails to prevent `cpirq_work` from being externally requeued by the HPD RX IRQ handler. During driver teardown, `hdcp_destroy()` frees `adev->dm.hdcp_workqueue`. However, `delayed_hpd_wq` and `hpd_rx_offload_wq` are not destroyed/flushed until later in `amdgpu_dm_fini()`. If the DP RX IRQ handler `handle_hpd_rx_irq()` runs concurrently on one of these workqueues, it checks `adev->dm.hdcp_workqueue` locklessly, which may still be non-NULL. It then calls `hdcp_handle_cpirq()`, which executes `schedule_work()` on the already freed memory, causing workqueue corruption and a Use-After-Free. - [High] Preexisting lockless access to the shared `hdcp_work->aconnector` array leads to a potential Use-After-Free. `event_property_update` fetches the `aconnector` pointer locklessly and dereferences it to read its status (`connector->status` and `aconnector->base.state`). Concurrently, a display unplug event can trigger `hdcp_remove_display()`, which acquires the mutex, clears the array slot, and drops the DRM reference via `drm_connector_put()`. For dynamically managed MST connectors, dropping this reference can result in the memory being freed immediately. The locklessly executing `event_property_update` will then access the freed connector memory. -- commit efb12556c957ec71a3ffe14ba50df873396200c3 Author: Fan Wu <[email protected]> Date: Mon Jul 6 09:24:17 2026 +0000 drm/amd/display: fix HDCP workqueue use-after-free on destroy This commit fixes a use-after-free issue during HDCP workqueue destruction. It adds a teardown flag to prevent internal callbacks from requeuing work after the destruction process has started, and cancels all queued work synchronously before freeing the memory. > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_hdcp.c > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_hdcp.c > index 039f6516085c8..b7219928d9052 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_hdcp.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_hdcp.c [ ... ] > @@ -528,9 +531,21 @@ void hdcp_destroy(struct kobject *kobj, struct > hdcp_workqueue *hdcp_work) > int i = 0; > > for (i = 0; i < hdcp_work->max_link; i++) { > + /* > + * process_output() can requeue the HDCP works. Set teardown > + * under the callback mutex first so no callback can requeue > + * work after destroy starts, then drain any work already > + * queued. > + */ > + mutex_lock(&hdcp_work[i].mutex); > + hdcp_work[i].teardown = true; > + mutex_unlock(&hdcp_work[i].mutex); > + > + cancel_work_sync(&hdcp_work[i].cpirq_work); [Severity: High] This is a pre-existing issue, but can the DP RX IRQ handler still queue cpirq_work after this teardown flag is set? During driver teardown, amdgpu_dm_fini() calls hdcp_destroy() and nullifies adev->dm.hdcp_workqueue before delayed_hpd_wq and hpd_rx_offload_wq are flushed. If the IRQ handler runs concurrently, it could read the workqueue pointer locklessly and blindly queue work on the freed memory: amdgpu_dm_irq.c:handle_hpd_rx_irq() { if (adev->dm.hdcp_workqueue) hdcp_handle_cpirq(adev->dm.hdcp_workqueue, ...); } amdgpu_dm_hdcp.c:hdcp_handle_cpirq() { schedule_work(&hdcp_w->cpirq_work); } Does hdcp_handle_cpirq() need to check the new teardown flag or take the mutex before scheduling the work? > cancel_delayed_work_sync(&hdcp_work[i].callback_dwork); > cancel_delayed_work_sync(&hdcp_work[i].watchdog_timer_dwork); > cancel_delayed_work_sync(&hdcp_work[i].property_validate_dwork); > + cancel_work_sync(&hdcp_work[i].property_update_work); [Severity: High] This is a pre-existing issue, but is there a race condition between property_update_work and dynamic display unplug? In event_property_update(), the connector is fetched locklessly from the hdcp_work->aconnector array and dereferenced to read its status: amdgpu_dm_hdcp.c:event_property_update() { aconnector = hdcp_work->aconnector[conn_index]; if (!aconnector) continue; connector = &aconnector->base; if (connector->status != connector_status_connected) continue; } Concurrently, a display unplug event might trigger hdcp_remove_display(), which drops the DRM reference: amdgpu_dm_hdcp.c:hdcp_remove_display() { if (hdcp_w->aconnector[conn_index]) { drm_connector_put(&hdcp_w->aconnector[conn_index]->base); hdcp_w->aconnector[conn_index] = NULL; } } If the connector memory is freed when the reference is dropped, could event_property_update() access freed memory when it reads connector->status? > } > > sysfs_remove_bin_file(kobj, &hdcp_work[0].attr); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
