On Tue, Sep 25, 2018 at 09:29:44PM +0100, Chris Wilson wrote:
> Quoting Ville Syrjala (2018-09-25 20:37:07)
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >
> > 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)
>
> Oh, it had fallen out of use.
>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/i915_utils.h | 8 ++++----
> > drivers/gpu/drm/i915/intel_display.c | 18 +++++++++++++++++-
> > 2 files changed, 21 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h
> > index 395dd2511568..c43ec993fa90 100644
> > --- a/drivers/gpu/drm/i915/i915_utils.h
> > +++ b/drivers/gpu/drm/i915/i915_utils.h
> > @@ -44,13 +44,13 @@
> > __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(A, B, C) \
> > + __builtin_add_overflow_p((A), (B), (C))
> > #else
> > -#define add_overflows(A, B) ({ \
> > +#define add_overflows(A, B, C) ({ \
> > typeof(A) a = (A); \
> > typeof(B) b = (B); \
> > - a + b < a; \
> > + (typeof(C))(a + b) < a; \
> > })
> > #endif
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 4c5c2b39e65c..a3ae24e03316 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2400,10 +2400,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));
> > +
> > + /* Catch potential overflows early */
> > + if (add_overflows(mul_u32_u32(height, fb->pitches[color_plane]),
> > + fb->offsets[color_plane], (u32)0)) {
>
> Should we just pass type? Atm we aren't using the value for anything.
> Then it would be add_overflows_t(a, b, T) with the obvious wrapping for
> add_overflows(a, b). Although to be consistent with min_t, perhaps
> add_overflows_t(T, a, b).
Indeed, that does seem a bit more consistent with existing stuff.
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
