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

[Matt Roper]

On Fri, Apr 19, 2019 at 10:12:02PM +0100, Chris Wilson wrote:
> Quoting Matt Roper (2019-04-19 22:06:05)
> > 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?
> 
> The bug is not for a NULL pointer dereference, but a use-after-free --
> so 0x6b6b6b6b6b6b6b not 0x0.
> -Chris

Okay, that makes more sense then.  But in that case I don't think the
WARN_ON tests being added in this patch will help since they're only
checking for NULL.


Matt

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