Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] drm/i915: missing drm_connector_put() in FRL modeset retry work causes refcount leak or UAF - [High] drm/i915: refcount underflow and UAF in HDMI FRL modeset retry work --
commit a06323f8771fc01d3d18f7c00ae7725f251aafec Author: Ankit Nautiyal <[email protected]> drm/i915/hdmi: Add support for sending uevent to user for FRL training failure This commit adds an HDMI-specific work function for handling modeset retries on link training failure. It also overrides the default connector work item to use this new function when FRL is supported. > diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c > b/drivers/gpu/drm/i915/display/intel_hdmi.c > index 0391597c9847f..69612ebe80f04 100644 > --- a/drivers/gpu/drm/i915/display/intel_hdmi.c > +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c > @@ -3397,6 +3399,31 @@ void intel_infoframe_init(struct intel_digital_port > *dig_port) > } > } > > +/* Common code with DP, need to put in a common place */ > +static void intel_hdmi_modeset_retry_work_fn(struct work_struct *work) > +{ > + struct intel_connector *intel_connector; > + struct drm_connector *connector; > + > + intel_connector = container_of(work, typeof(*intel_connector), > + modeset_retry_work); > + connector = &intel_connector->base; > + DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", connector->base.id, > + connector->name); > + > + /* Grab the locks before changing connector property*/ > + mutex_lock(&connector->dev->mode_config.mutex); > + /* > + * Set connector link status to BAD and send a Uevent to notify > + * userspace to do a modeset. > + */ > + drm_connector_set_link_status_property(connector, > + DRM_MODE_LINK_STATUS_BAD); > + mutex_unlock(&connector->dev->mode_config.mutex); > + /* Send Hotplug uevent so userspace can reprobe */ > + drm_kms_helper_hotplug_event(connector->dev); > +} [Severity: High] Does this function leak a connector reference, or leave the connector open to a use-after-free? The generic intel_connector_modeset_retry_work_fn expects a connector reference to be taken before queueing, and balances it by calling drm_connector_put() at the end of the work function. This HDMI-specific function appears functionally identical to the generic counterpart but omits the drm_connector_put() call. If the work is queued via intel_connector_queue_modeset_retry_work(), which acquires a reference, the missing put causes a permanent memory leak. If the work is instead queued directly via schedule_work() without taking a reference, no reference is held while the work executes. This allows a concurrent teardown to free the connector out from under the running work item. [ ... ] > @@ -3470,6 +3497,11 @@ bool intel_hdmi_init_connector(struct > intel_digital_port *dig_port, > if (!intel_hdmi->cec_notifier) > drm_dbg_kms(display->drm, "CEC notifier get failed\n"); > > + /* Initialize the work for modeset in case of link train failure */ > + if (HAS_HDMI_FRL(display)) > + INIT_WORK(&intel_connector->modeset_retry_work, > + intel_hdmi_modeset_retry_work_fn); > + [Severity: High] Does overriding the modeset_retry_work function here break the lifecycle contract for the workqueue? intel_connector_alloc() initializes modeset_retry_work with the generic intel_connector_modeset_retry_work_fn, which manages connector refcounts. If this work is queued directly using schedule_work() without acquiring a connector reference first, and then cancelled during teardown via intel_connector_cancel_modeset_retry_work() (called from intel_hpd_poll_fini()), it will unconditionally call drm_connector_put() upon successful cancellation. Since no reference was acquired before scheduling the work, this cancellation drops a reference that was never acquired, causing a refcount underflow and premature freeing of the connector. > return true; > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=39
