[Intel-gfx] [PATCH v4 1/8] drm/i915: Make sure fb gtt offsets stay within 32bits

[Ville Syrjälä]

On Tue, Oct 23, 2018 at 07:49:42PM +0100, Chris Wilson wrote:
> Quoting Ville Syrjala (2018-10-23 17:02:01)
> > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > 
> > Let's try to make sure the fb offset computations never hit
> > an integer overflow by making sure the entire fb stays
> > below 32bits. framebuffer_check() in the core already does
> > the same check, but as it doesn't know about tiling some things
> > can slip through. Repeat the check in the driver with tiling
> > taken into account.
> > 
> > v2: Use add_overflows() after massaging it to work for me (Chris)
> > v3: Call it add_overflow_t() to match min_t() & co. (Chris)
> > 
> > Cc: Chris Wilson <chris at chris-wilson.co.uk>
> > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_utils.h    | 11 +++++++----
> >  drivers/gpu/drm/i915/intel_display.c | 18 +++++++++++++++++-
> >  2 files changed, 24 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h
> > index 5858a43e19da..9726df37c4c4 100644
> > --- a/drivers/gpu/drm/i915/i915_utils.h
> > +++ b/drivers/gpu/drm/i915/i915_utils.h
> > @@ -44,16 +44,19 @@
> >                              __stringify(x), (long)(x))
> >  
> >  #if defined(GCC_VERSION) && GCC_VERSION >= 70000
> > -#define add_overflows(A, B) \
> > -       __builtin_add_overflow_p((A), (B), (typeof((A) + (B)))0)
> > +#define add_overflows_t(T, A, B) \
> > +       __builtin_add_overflow_p((A), (B), (T)0)
> >  #else
> > -#define add_overflows(A, B) ({ \
> > +#define add_overflows_t(T, A, B) ({ \
> >         typeof(A) a = (A); \
> >         typeof(B) b = (B); \
> > -       a + b < a; \
> > +       (T)(a + b) < a; \
> >  })
> >  #endif
> >  
> > +#define add_overflows(A, B) \
> > +       add_overflows_t(typeof((A) + (B)), (A), (B))
> > +
> >  #define range_overflows(start, size, max) ({ \
> >         typeof(start) start__ = (start); \
> >         typeof(size) size__ = (size); \
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 969d22ca8dcd..e520facc23e1 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2351,10 +2351,26 @@ static int intel_fb_offset_to_xy(int *x, int *y,
> >                                  int color_plane)
> >  {
> >         struct drm_i915_private *dev_priv = to_i915(fb->dev);
> > +       unsigned int height;
> >  
> >         if (fb->modifier != DRM_FORMAT_MOD_LINEAR &&
> > -           fb->offsets[color_plane] % intel_tile_size(dev_priv))
> > +           fb->offsets[color_plane] % intel_tile_size(dev_priv)) {
> > +               DRM_DEBUG_KMS("Misaligned offset 0x%08x for color plane %d\n",
> > +                             fb->offsets[color_plane], color_plane);
> >                 return -EINVAL;
> > +       }
> > +
> > +       height = drm_framebuffer_plane_height(fb->height, fb, color_plane);
> > +       height = ALIGN(height, intel_tile_height(fb, color_plane));
> 
> fb height is limited to u16 or thereabouts?

Thereabouts. Would be <=16k after the last patch in the series.

> 
> > +
> > +       /* Catch potential overflows early */
> > +       if (add_overflows_t(u32, mul_u32_u32(height, fb->pitches[color_plane]),
> > +                           fb->offsets[color_plane])) {
> > +               DRM_DEBUG_KMS("Bad offset 0x%08x or pitch %d for color plane %d\n",
> > +                             fb->offsets[color_plane], fb->pitches[color_plane],
> > +                             color_plane);
> > +               return -ERANGE;
> > +       }
> 
> Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
> -Chris

-- 
Ville Syrjälä
Intel