| Message ID | 20190416031250.21690-1-clinton.a.taylor@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/icl: Prevent possibe de-reference in skl_check_pipe_max_pixel_clock. | expand |
On Mon, Apr 15, 2019 at 08:12:50PM -0700, clinton.a.taylor@intel.com wrote: > From: Clint Taylor <clinton.a.taylor@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@intel.com> > Cc: Martin Peres <martin.peres@linux.intel.com> > Signed-off-by: Clint Taylor <clinton.a.taylor@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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoting Matt Roper (2019-04-19 22:06:05) > On Mon, Apr 15, 2019 at 08:12:50PM -0700, clinton.a.taylor@intel.com wrote: > > From: Clint Taylor <clinton.a.taylor@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@intel.com> > > Cc: Martin Peres <martin.peres@linux.intel.com> > > Signed-off-by: Clint Taylor <clinton.a.taylor@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
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@intel.com wrote: > > > From: Clint Taylor <clinton.a.taylor@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@intel.com> > > > Cc: Martin Peres <martin.peres@linux.intel.com> > > > Signed-off-by: Clint Taylor <clinton.a.taylor@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
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)) + 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; + if (!intel_wm_plane_visible(cstate, to_intel_plane_state(pstate))) continue;