Re: [PATCH v10 10/69] drm/display: hdmi: Add HDMI 2.0 scrambling management helpers

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The SCDC scrambling monitor work item fails to reschedule itself, terminating the periodic polling after a single successful check or transient error.
- [High] Missing NULL check for connector->ddc in drm_connector_hdmi_disable_scrambling() causes a crash when scrambling is disabled for a connector without a DDC bus.
- [High] TOCTOU race condition in drm_connector_hdmi_scdc_work leading to a potential Use-After-Free of the I2C adapter.
- [High] Dead code in drm_connector_hdmi_sync_scdc fails to synchronize with hardware programming due to an impossible condition.
--

--- Patch [10]: [PATCH v10 10/69] drm/display: hdmi: Add HDMI 2.0 scrambling management helpers ---
commit 3991d28f3f40213e274370d230b6145165dadbee
Author: Cristian Ciocaltea <[email protected]>

drm/display: hdmi: Add HDMI 2.0 scrambling management helpers

This commit adds helpers to manage the full lifecycle of HDMI 2.0 SCDC
scrambling on both source and sink sides. It introduces checking if a mode
needs scrambling, enabling/disabling with monitoring, and restoring the
SCDC state on reconnection.

> diff --git a/drivers/gpu/drm/display/drm_hdmi_helper.c b/drivers/gpu/drm/display/drm_hdmi_helper.c
> index 5cb0b033b1713..cfecfef639ad0 100644
> --- a/drivers/gpu/drm/display/drm_hdmi_helper.c
> +++ b/drivers/gpu/drm/display/drm_hdmi_helper.c

[ ... ]

> @@ -426,3 +434,291 @@ drm_hdmi_acr_get_n_cts(unsigned long long tmds_char_rate,
>  	*out_cts = cts;
>  }
>  EXPORT_SYMBOL(drm_hdmi_acr_get_n_cts);

[ ... ]

> +static int drm_connector_hdmi_try_scrambling_setup(struct drm_connector *connector)
> +{
> +	bool done;
> +
> +	done = drm_scdc_set_high_tmds_clock_ratio(connector, true);
> +	if (!done)
> +		return -EIO;

[Severity: High]
Does this prevent the SCDC worker from retrying on transient errors?

If drm_scdc_set_high_tmds_clock_ratio() fails, the function returns -EIO
early and skips the schedule_delayed_work() call, preventing any further
monitoring of the scrambling state.

[ ... ]

> +static void drm_connector_hdmi_scdc_work(struct work_struct *work)
> +{
> +	struct drm_connector *connector;
> +	struct drm_connector_hdmi *hdmi;
> +
> +	hdmi = container_of(to_delayed_work(work), struct drm_connector_hdmi,
> +			    scdc_work);
> +	connector = container_of(hdmi, struct drm_connector, hdmi);
> +
> +	/*
> +	 * The connector, and hence this work item, may outlive the source
> +	 * driver, e.g. when userspace keeps a DRM file descriptor open past
> +	 * unbind. Never poke the DDC bus after the connector got unregistered,
> +	 * as the underlying I2C adapter may already be gone.
> +	 */
> +	if (drm_connector_is_unregistered(connector))
> +		return;

[Severity: High]
Could this lockless check lead to a Use-After-Free of the I2C adapter?

If the worker thread is preempted immediately after this check returns false,
the driver unbind process could complete and free the I2C adapter. When the
worker resumes, it calls drm_scdc_get_scrambling_status() which accesses the
now-freed connector->ddc.

> +
> +	if (READ_ONCE(connector->hdmi.scrambler_enabled) &&
> +	    !drm_scdc_get_scrambling_status(connector))
> +		drm_connector_hdmi_try_scrambling_setup(connector);

[Severity: High]
Will this work item fail to reschedule itself if the SCDC status is correct?

If drm_scdc_get_scrambling_status() returns true (meaning scrambling is
working), this function simply returns without rescheduling the worker. The
rescheduling only happens inside drm_connector_hdmi_try_scrambling_setup()
on a successful setup, so the periodic monitoring stops after the first
successful check.

> +}

[ ... ]

> +int drm_connector_hdmi_disable_scrambling(struct drm_connector *connector)
> +{
> +	struct drm_connector_hdmi *hdmi = &connector->hdmi;
> +	struct drm_device *dev = connector->dev;
> +
> +	if (!READ_ONCE(hdmi->scrambler_enabled))
> +		return 0;
> +
> +	drm_dbg_kms(dev, "Disabling scrambling\n");
> +
> +	WRITE_ONCE(hdmi->scrambler_enabled, false);
> +
> +	/*
> +	 * A driver may force scrambler_enabled at init to trigger a disable
> +	 * at boot (e.g. vc4), bypassing the enable path that sets up the work
> +	 * item. Only cancel it once it has actually been initialized.
> +	 */
> +	if (hdmi->scdc_work_initialized)
> +		cancel_delayed_work_sync(&hdmi->scdc_work);
> +
> +	if (connector->status == connector_status_connected &&
> +	    drm_scdc_sink_supports_scrambling(connector)) {
> +		drm_scdc_set_scrambling(connector, false);
> +		drm_scdc_set_high_tmds_clock_ratio(connector, false);

[Severity: High]
Can this cause a NULL pointer dereference if connector->ddc is missing?

If a driver forces hdmi->scrambler_enabled to true at init to trigger a
disable at boot, and the connector has no DDC bus (connector->ddc is NULL),
drm_scdc_set_scrambling() will unconditionally dereference the NULL DDC
adapter. The enable path correctly checks for connector->ddc, but the disable
path does not.

> +	}
> +
> +	return hdmi->funcs->scrambler_disable(connector);
> +}

[ ... ]

> +int drm_connector_hdmi_sync_scdc(struct drm_connector *connector, bool plugged,
> +				 struct drm_modeset_acquire_ctx *ctx)
> +{
> +	struct drm_connector_hdmi *hdmi = &connector->hdmi;
> +	struct drm_device *dev = connector->dev;
> +	struct drm_connector_state *conn_state;
> +	struct drm_crtc *crtc;
> +	u8 config;
> +	int ret;
> +
> +	if (!connector || !plugged || !ctx)
> +		return 0;

[ ... ]

> +	crtc = conn_state->crtc;
> +	if (!crtc)
> +		return 0;
> +
> +	ret = drm_modeset_lock(&crtc->mutex, ctx);
> +	if (ret)
> +		return ret;
> +
> +	if (!crtc->state || !crtc->state->active)
> +		return 0;
> +
> +	if (!connector->ddc || !drm_scdc_sink_supports_scrambling(connector))
> +		return 0;
> +
> +	if (conn_state->commit &&
> +	    !try_wait_for_completion(&conn_state->commit->hw_done))

[Severity: High]
Is this wait condition dead code?

Earlier in the function, it verifies that crtc is not NULL (if (!crtc)
return 0;). According to the struct drm_connector_state documentation, the
commit field is only set when crtc is NULL. Since crtc is guaranteed to be
non-NULL here, conn_state->commit will always be NULL, making this check
impossible to hit. Consequently, the function proceeds to perform SCDC I2C
reads concurrently with active hardware programming.

> +		return 0;
> +
> +	ret = drm_scdc_readb(connector->ddc, SCDC_TMDS_CONFIG, &config);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=10