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 <[email protected]> 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/[email protected]?part=4
