Re: [PATCH v5 04/15] drm/i915/alpm: Enable debugfs for DP2.1

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Unconditional dereference of `connector->encoder` in ALPM debugfs operations leads to a kernel crash on inactive DP MST connectors.

Pre-existing issues:
- [High] Use-After-Free of CRTC state and unsafe hardware register access in `i915_edp_lobf_info_show`.
--

--- Patch [4]: [PATCH v5 04/15] drm/i915/alpm: Enable debugfs for DP2.1 ---
commit 92b26450255b1614cb8fb75394a055699100a23e
Author: Animesh Manna <animesh.manna@intel.com>

drm/i915/alpm: Enable debugfs for DP2.1

The commit enables ALPM debug information through LOBF debug info.
It replaces the DISPLAY_VER and DRM_MODE_CONNECTOR_eDP checks with a
common api check instead.

> diff --git a/drivers/gpu/drm/i915/display/intel_alpm.c b/drivers/gpu/drm/i915/display/intel_alpm.c
> index 08ad77cd32f8c..8cfcda97cf588 100644
> --- a/drivers/gpu/drm/i915/display/intel_alpm.c
> +++ b/drivers/gpu/drm/i915/display/intel_alpm.c
> @@ -617,11 +617,9 @@ DEFINE_SIMPLE_ATTRIBUTE(i915_edp_lobf_debug_fops,
>  
>  void intel_alpm_lobf_debugfs_add(struct intel_connector *connector)
>  {
> -	struct intel_display *display = to_intel_display(connector);
>  	struct dentry *root = connector->base.debugfs_entry;
>  
> -	if (DISPLAY_VER(display) < 20 ||
> -	    connector->base.connector_type != DRM_MODE_CONNECTOR_eDP)
> +	if (!intel_alpm_source_supported(connector))
>  		return;
>  

[Severity: Critical]
Does relaxing this condition allow debugfs files to be created for dynamic MST
connectors?

If so, could this lead to a NULL pointer dereference in
i915_edp_lobf_debug_get() and i915_edp_lobf_debug_set()?

For MST connectors, connector->encoder is NULL until a mode is set. If the
debugfs files are accessed on an inactive MST connector, the unconditional
dereference:

drivers/gpu/drm/i915/display/intel_alpm.c:i915_edp_lobf_debug_get() {
    ...
    struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder);
    ...
}

would crash the kernel.

[Severity: High]
This is a pre-existing issue, but I noticed a potential use-after-free and
unsafe hardware register access in i915_edp_lobf_info_show().

When reading the debugfs file, i915_edp_lobf_info_show() accesses
crtc->base.state and ALPM_CTL:

drivers/gpu/drm/i915/display/intel_alpm.c:i915_edp_lobf_info_show() {
    ...
    crtc_state = to_intel_crtc_state(crtc->base.state);
    cpu_transcoder = crtc_state->cpu_transcoder;
    alpm_ctl = intel_de_read(display, ALPM_CTL(display, cpu_transcoder));
    ...
}

If this is read concurrently with a non-routing atomic commit (which can swap
and free crtc->state without connection_mutex), could this result in a
use-after-free?

Additionally, is it safe to read from the ALPM_CTL register without first
checking if the CRTC is active and acquiring a display power domain wakeref?

>  	debugfs_create_file("i915_edp_lobf_debug", 0644, root,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825054536.3455704-1-animesh.manna@intel.com?part=4