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

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