[Intel-gfx] [PATCH] drm/i915/icl: Prevent possibe de-reference in skl_check_pipe_max_pixel_clock.

[Matt Roper]

On Mon, Apr 15, 2019 at 08:12:50PM -0700, clinton.a.taylor at intel.com wrote:
> From: Clint Taylor <clinton.a.taylor at intel.com>
> 
> Add protections to prevent NULL de-reference for a couple variables used
> in skl_check_pipe_max_pixel_clock to prevent GP exception from occurring
> during some IGT tests.
> 
> References: https://bugs.freedesktop.org/show_bug.cgi?id=109084
> 
> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> Cc: Martin Peres <martin.peres at linux.intel.com>
> Signed-off-by: Clint Taylor <clinton.a.taylor at intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 4 ++++
>  drivers/gpu/drm/i915/intel_pm.c      | 3 +++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 3bd40a4a6739..945861cef520 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11377,6 +11377,10 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
>  
>  		if (!ret)
>  			ret = icl_check_nv12_planes(pipe_config);
> +
> +		if (WARN_ON(!intel_crtc))

If intel_crtc is NULL, then crtc should also be NULL as well, and we
already dereferenced that earlier:

        struct drm_i915_private *dev_priv = to_i915(crtc->dev);

So if crtc/intel_crtc were the problem, I believe this check would still
be too late to catch and prevent a crash.


> +			return -EINVAL;
> +
>  		if (!ret)
>  			ret = skl_check_pipe_max_pixel_rate(intel_crtc,
>  							    pipe_config);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 7357bddf9ad9..df5d01d4345b 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4160,6 +4160,9 @@ int skl_check_pipe_max_pixel_rate(struct intel_crtc *intel_crtc,
>  		uint_fixed_16_16_t fp_9_div_8 = div_fixed16(9, 8);
>  		int bpp;
>  
> +		if (WARN_ON(!pstate))
> +			return -EINVAL;

The pstate here comes from drm_atomic_crtc_state_for_each_plane_state,
which does a 'for_each_if' to only execute the loop body if pstate is
non-NULL, so I don't see how this one could be possible either.


Do you see the driver tripping over either of these guards once this
patch is applied?  If we've got a backtrace for a gp fault, can we
convert the RIP back into a specific line of code that triggered the
fault?


Matt

> +
>  		if (!intel_wm_plane_visible(cstate,
>  					    to_intel_plane_state(pstate)))
>  			continue;
> -- 
> 2.19.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795