[PATCH v2] drm/amd/display: fix HDCP workqueue use-after-free on destroy

[PATCH v2] drm/amd/display: fix HDCP workqueue use-after-free on destroy

[Fan Wu]

hdcp_destroy() cancels callback_dwork, watchdog_timer_dwork and
property_validate_dwork, but leaves cpirq_work and property_update_work
queued. These works are queued with schedule_work(), recover struct
hdcp_workqueue through container_of(), and dereference it. If either work
runs after hdcp_destroy() frees hdcp_work, it can trigger a
use-after-free.

The HDCP callbacks also call process_output(), which can requeue the HDCP
works: it always queues property_validate_dwork and may queue
callback_dwork or watchdog_timer_dwork depending on the state-machine
output. A simple cancel sequence can therefore miss work requeued by
another callback. hdcp_handle_cpirq() can also queue cpirq_work from
outside the callback path.

Set a teardown flag under hdcp_work->mutex before draining the works.
The callbacks already hold this mutex while calling process_output(), and
hdcp_handle_cpirq() takes it before queueing cpirq_work, so once teardown
is set no path can requeue work. Then cancel cpirq_work, the delayed
works, and property_update_work before freeing hdcp_work.

This bug was found by static analysis.

Fixes: a193ed2094ba ("drm/amd/display: Create amdgpu_dm_hdcp")
Fixes: da3fd7ac0bcf ("drm/amd/display: Update CP property based on HW query")
Cc: stable@vger.kernel.org
Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>

---
Changes in v2:
- Gate hdcp_handle_cpirq() with the teardown flag so late CPIRQ handling
  cannot requeue cpirq_work after hdcp_destroy() starts.
---
 .../amd/display/amdgpu_dm/amdgpu_dm_hdcp.c    | 20 ++++++++++++++++++-
 .../amd/display/amdgpu_dm/amdgpu_dm_hdcp.h    |  2 ++
 2 files changed, 21 insertions(+), 1 deletion(-)

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 4c164ae4a4f9..2293e9761891 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
@@ -164,6 +164,9 @@ void process_output(struct hdcp_workqueue *hdcp_work)
 {
 	struct mod_hdcp_output output = hdcp_work->output;
 
+	if (hdcp_work->teardown)
+		return;
+
 	if (output.callback_stop)
 		cancel_delayed_work(&hdcp_work->callback_dwork);
 
@@ -310,7 +313,10 @@ void hdcp_handle_cpirq(struct hdcp_workqueue *hdcp_work, unsigned int link_index
 {
 	struct hdcp_workqueue *hdcp_w = &hdcp_work[link_index];
 
-	schedule_work(&hdcp_w->cpirq_work);
+	mutex_lock(&hdcp_w->mutex);
+	if (!hdcp_w->teardown)
+		schedule_work(&hdcp_w->cpirq_work);
+	mutex_unlock(&hdcp_w->mutex);
 }
 
 static void event_callback(struct work_struct *work)
@@ -487,9 +493,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);
 		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);
 	}
 
 	sysfs_remove_bin_file(kobj, &hdcp_work[0].attr);
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_hdcp.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_hdcp.h
index 90b18c450ca6..2313434b57c0 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_hdcp.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_hdcp.h
@@ -54,6 +54,8 @@ struct hdcp_workqueue {
 	struct amdgpu_dm_connector *aconnector[AMDGPU_DM_MAX_DISPLAY_INDEX];
 	struct mutex mutex;
 
+	bool teardown;
+
 	struct mod_hdcp hdcp;
 	struct mod_hdcp_output output;
 	struct mod_hdcp_display display;
-- 
2.34.1

Re: [PATCH v2] drm/amd/display: fix HDCP workqueue use-after-free on destroy

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Out-of-bounds array access due to using DRM connector index instead of hardware link index.
- [High] Lockless access to `hdcp_work->aconnector` leading to Use-After-Free of a DRM connector.
- [High] Out-of-bounds array access on `hdcp_w->aconnector` using the unbounded DRM connector index.
--

commit 378fa8b96a79b8b40551c4d1a03ff5b822a7c73b
Author: Fan Wu <fanwu01@zju.edu.cn>

drm/amd/display: fix HDCP workqueue use-after-free on destroy

hdcp_destroy() cancels callback_dwork, watchdog_timer_dwork and
property_validate_dwork, but leaves cpirq_work and property_update_work
queued. These works are queued with schedule_work(), recover struct
hdcp_workqueue through container_of(), and dereference it.

Cc: stable@vger.kernel.org

> 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..e060d45f8229b 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

[ ... ]

> @@ -361,7 +364,10 @@ void hdcp_handle_cpirq(struct hdcp_workqueue *hdcp_work, unsigned int link_index
>  {
>  	struct hdcp_workqueue *hdcp_w = &hdcp_work[link_index];

[Severity: Critical]
This isn't a bug introduced by this patch, but can this out-of-bounds array
access occur?

When a DisplayPort CP_IRQ occurs, handle_hpd_rx_irq() passes the global
DRM connector index instead of the physical link index:

drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c:handle_hpd_rx_irq() {
    ...
    hdcp_handle_cpirq(adev->dm.hdcp_workqueue, aconnector->base.index);
    ...
}

Because base.index increments for dynamically created MST displays, it can
exceed max_links. Would this cause hdcp_work[link_index] to evaluate out of
bounds here?

> -	schedule_work(&hdcp_w->cpirq_work);
> +	mutex_lock(&hdcp_w->mutex);
> +	if (!hdcp_w->teardown)
> +		schedule_work(&hdcp_w->cpirq_work);
> +	mutex_unlock(&hdcp_w->mutex);
>  }

[ ... ]

[Severity: High]
This is a pre-existing issue, but does this lockless access risk a
use-after-free of the drm_connector?

In event_property_update(), the connector is accessed without holding
hdcp_work->mutex:

drivers/gpu/drm/amd/display/amdgpu_dm/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)
    ...
}

If this races with hdcp_remove_display() which takes the mutex and drops
the connector reference:

drivers/gpu/drm/amd/display/amdgpu_dm/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;
    }
    ...
}

Could event_property_update() dereference a freed connector if it reads
the pointer just before hdcp_remove_display() runs?


[Severity: High]
This is a pre-existing issue, but does this code overflow the
hdcp_w->aconnector[] array?

Functions like hdcp_update_display() and hdcp_remove_display() use the
unbounded DRM connector index to access the array:

drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_hdcp.c:hdcp_update_display() {
    ...
    unsigned int conn_index = aconnector->base.index;
    ...
    hdcp_w->aconnector[conn_index] = aconnector;
    ...
}

Since hdcp_w->aconnector is hardcoded to AMDGPU_DM_MAX_DISPLAY_INDEX (31),
and DRM connector indices are allocated globally, could connecting a large
DisplayPort MST topology cause base.index to exceed 31 and overwrite
adjacent memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706143548.136979-1-fanwu01@zju.edu.cn?part=1