HPD enable/disable are silent no-ops: register modification never written back

• File: sys/dev/drm/amd/display/dc/dce/dce_link_encoder.c
• Lines: 1366, 1378
• Severity: Info
• CVSS: CVSS:3.1/AV:L/AC:H/PR:H/UI:N/S:U:C:N/I:N/A:N
• CWE: CWE-665 Improper Initialization
• Confidence: certain

Summary

dce110_link_encoder_enable_hpd and dce110_link_encoder_disable_hpd both read DC_HPD_CONTROL, modify a local uint32_t copy via set_reg_field_value, but never call dm_write_reg to persist the change.

enable_hpd additionally contains dead code: get_reg_field_value(hpd_enable, DC_HPD_CONTROL, DC_HPD_EN) extracts a field from the wrong source variable (the literal 0 local hpd_enable) and discards the return value.

Net effect: both HPD-control entry points are silently inert; the DC_HPD_EN bit is never changed by this code path.

Compare aux_initialize (dce_link_encoder.c:489-509) which correctly ends with dm_write_reg(ctx, addr, value).

Root cause

In dce110_link_encoder_enable_hpd (dce_link_encoder.c:1366-1378):

uint32_t hpd_enable = 0;
uint32_t value = dm_read_reg(ctx, addr);
get_reg_field_value(hpd_enable, DC_HPD_CONTROL, DC_HPD_EN);   /* (1) bug */
if (hpd_enable == 0)
    set_reg_field_value(value, 1, DC_HPD_CONTROL, DC_HPD_EN); /* (2) bug */

Bug (1): get_reg_field_value is a function-like macro whose return value is the extracted field (dm_services.h:120-124 → get_reg_field_value_ex at dm_services.h:114-118 returns (mask & reg_value) >> shift).

As a statement its return is discarded, so hpd_enable stays 0; the call also reads from the wrong variable (hpd_enable==0 instead of value).

Intent was clearly: hpd_enable = get_reg_field_value(value, DC_HPD_CONTROL, DC_HPD_EN);

Bug (2): set_reg_field_value(var, val, reg, field) expands to (var) = set_reg_field_value_ex(...) (dm_services.h:136-141), modifying only the local value.

No dm_write_reg(ctx, addr, value) follows; the modified value is dropped on function return.

dm_write_reg appears nowhere in either HPD function (grep over the file confirms only aux_initialize at lines 499, 507 calls dm_write_reg).

The same write-back defect exists independently in dce110_link_encoder_disable_hpd (dce_link_encoder.c:1380-1388): set_reg_field_value(value, 0, ...) mutates locals and returns.

Threat

No direct memory-safety impact: no OOB, no UAF, no info leak, no privilege escalation.

This is a functional defect: HPD enable/disable requests from the dc_link HPD gating path (e.g. dp_set_hw_test_pattern in dc_link_hwss.c:198,291 and the DP PHY test-pattern flow that sets disable_hpd=true in amdgpu_dm_debugfs.c:592) silently fail to take effect, so hot-plug detection remains in whatever state the hardware was last left in.

Reachable by any local user with write access to the amdgpu DP test-pattern debugfs node (mode 0664 typical, root:video).

Practical consequence is incorrect display hotplug behavior during PHY compliance testing and resume transitions, not a kernel compromise.

Filed at Info per the severity rubric (no demonstrated security impact), as a hardening/defense-in-depth note that the HPD-gate assumed by surrounding code does not actually close.

Exploit / PoC

Functional PoC showing the no-op (no privilege required beyond debugfs write access).

On a DragonFlyBSD/amdgpu system with a DP connector, as a user with write access to /sys/kernel/debug/dri/0/amdgpu_dp_test_pattern:

1. Read the DC_HPD_CONTROL register (e.g. mmread 0x1a0e for HPD0_DC_HPD_CONTROL): DC_HPD_EN bit = X.
2. Trigger dce110_link_encoder_disable_hpd via the dc_link_disable_hpd path (e.g. echo a PHY test-pattern selection into the debugfs node that sets disable_hpd=true; amdgpu_dm_debugfs.c:586-594).
3. Re-read HPD0_DC_HPD_CONTROL: DC_HPD_EN still equals X (no change). Expected behavior (per function name and per the Linux DRM contract) is DC_HPD_EN == 0 after disable_hpd.
4. Symmetrically for enable_hpd: starting from DC_HPD_EN=0, calling enable_hpd leaves DC_HPD_EN at 0.

Success criterion: DC_HPD_EN bit value is unchanged across the call (proving the function is inert).

Recommended fix

Persist the modified value to hardware with dm_write_reg, mirroring aux_initialize (dce_link_encoder.c:489-509).

For enable_hpd, also fix the dead/incorrect get_reg_field_value call.

--- a/sys/dev/drm/amd/display/dc/dce/dce_link_encoder.c
+++ b/sys/dev/drm/amd/display/dc/dce/dce_link_encoder.c
@@ -1366,16 +1366,18 @@ void dce110_link_encoder_enable_hpd(struct link_encoder *enc)
 void dce110_link_encoder_enable_hpd(struct link_encoder *enc)
 {
    struct dce110_link_encoder *enc110 = TO_DCE110_LINK_ENC(enc);
    struct dc_context *ctx = enc110->base.ctx;
    uint32_t addr = HPD_REG(DC_HPD_CONTROL);
-   uint32_t hpd_enable = 0;
    uint32_t value = dm_read_reg(ctx, addr);

-   get_reg_field_value(hpd_enable, DC_HPD_CONTROL, DC_HPD_EN);
-
-   if (hpd_enable == 0)
-       set_reg_field_value(value, 1, DC_HPD_CONTROL, DC_HPD_EN);
+   /* Read the current DC_HPD_EN bit; if it is not already set, set it and
+    * write the modified value back to hardware. */
+   if (((HPD0_DC_HPD_CONTROL__DC_HPD_EN_MASK & value) >>
+                   HPD0_DC_HPD_CONTROL__DC_HPD_EN__SHIFT) == 0) {
+       set_reg_field_value(value, 1, DC_HPD_CONTROL, DC_HPD_EN);
+       dm_write_reg(ctx, addr, value);
+   }
 }

 void dce110_link_encoder_disable_hpd(struct link_encoder *enc)
 {
    struct dce110_link_encoder *enc110 = TO_DCE110_LINK_ENC(enc);
    struct dc_context *ctx = enc110->base.ctx;
    uint32_t addr = HPD_REG(DC_HPD_CONTROL);
    uint32_t value = dm_read_reg(ctx, addr);

    set_reg_field_value(value, 0, DC_HPD_CONTROL, DC_HPD_EN);
+   dm_write_reg(ctx, addr, value);
 }