Re: [Intel-gfx] [PATCH] drm/i915/display: Fix a use-after-free when intel_edp_init_connector fails

[Jani Nikula <jani.nikula@linux.intel.com>]

On Tue, 20 Dec 2022, Maarten Lankhorst <maarten.lankhorst@linux.intel.com> wrote:
> We enable the DP aux channel during probe, but may free the connector
> soon afterwards. Ensure the DP aux display power put is completed before
> everything is freed, to prevent a use-after-free in icl_aux_pw_to_phy(),
> called from icl_combo_phy_aux_power_well_disable.

Feels like the placement of the intel_display_power_flush_work_sync()
call in intel_dp_aux_fini() is a bit arbitrary.

If we add it in intel_dp_aux_fini(), the async and sync waits will both
be called on the regular encoder destroy path.

Maybe both intel_ddi_encoder_destroy() and intel_dp_encoder_destroy()
should call intel_display_power_flush_work_sync(), instead of async, and
maybe the error paths should call those functions instead of just
drm_encoder_cleanup()?

Imre?


BR,
Jani.


>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display_power.c | 2 +-
>  drivers/gpu/drm/i915/display/intel_display_power.h | 1 +
>  drivers/gpu/drm/i915/display/intel_dp_aux.c        | 2 ++
>  3 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
> index 04915f85a0df..0edb5532461f 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> @@ -776,7 +776,7 @@ void intel_display_power_flush_work(struct drm_i915_private *i915)
>   * Like intel_display_power_flush_work(), but also ensure that the work
>   * handler function is not running any more when this function returns.
>   */
> -static void
> +void
>  intel_display_power_flush_work_sync(struct drm_i915_private *i915)
>  {
>  	struct i915_power_domains *power_domains = &i915->display.power.domains;
> diff --git a/drivers/gpu/drm/i915/display/intel_display_power.h b/drivers/gpu/drm/i915/display/intel_display_power.h
> index 7136ea3f233e..dc10ee0519e6 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_power.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_power.h
> @@ -188,6 +188,7 @@ void __intel_display_power_put_async(struct drm_i915_private *i915,
>  				     enum intel_display_power_domain domain,
>  				     intel_wakeref_t wakeref);
>  void intel_display_power_flush_work(struct drm_i915_private *i915);
> +void intel_display_power_flush_work_sync(struct drm_i915_private *i915);
>  #if IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM)
>  void intel_display_power_put(struct drm_i915_private *dev_priv,
>  			     enum intel_display_power_domain domain,
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux.c b/drivers/gpu/drm/i915/display/intel_dp_aux.c
> index f1835c74bff0..1006dddad2d5 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_aux.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_aux.c
> @@ -680,6 +680,8 @@ void intel_dp_aux_fini(struct intel_dp *intel_dp)
>  	if (cpu_latency_qos_request_active(&intel_dp->pm_qos))
>  		cpu_latency_qos_remove_request(&intel_dp->pm_qos);
>  
> +	/* Ensure async work from intel_dp_aux_xfer() is flushed before we clean up */
> +	intel_display_power_flush_work_sync(dp_to_i915(intel_dp));
>  	kfree(intel_dp->aux.name);
>  }

-- 
Jani Nikula, Intel Open Source Graphics Center