[Intel-gfx] [PATCH] drm/i915: Fix a potential integer overflow with framebuffers extending past 4 GiB

[Ville Syrjälä]

On Wed, Sep 12, 2018 at 09:13:07AM +0100, Chris Wilson wrote:
> Quoting Ville Syrjala (2018-09-11 17:54:57)
> > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > 
> > If we have framebuffers that are >= 4GiB in size we will overflow
> > the fb size check in intel_fill_fb_info().
> > 
> > Currently that is only possible with NV12 and CCS as offsets[1]
> > may be anything between 0 and 0xffffffff. ofsets[0] is currently
> > required to be 0 so we can't hit the overflow with any single
> > plane format (thanks to max fb size of 8kx8k and max stride of
> > 32 KiB).
> > 
> > In the future we may allow almost any framebuffer to exceed 4GiB
> > in size so we really should fix the overflow. Not that the overflow
> > is particularly dangerous. It's mostly just a sanity check against
> > insane userspace. The display engine can't write to memory anyway
> > so I suppose in the worst case we might anger the hw by attempting
> > scanout past the end of the ggtt, or we might scan out some data
> > that we're not supposed to see from other parts of the ggtt.
> > 
> > Note that triggering this overflow depends on the driver
> > aligning the fb height to the next tile boundary to push the
> > calculated size above 4GiB. With linear buffers the effective
> > tile height is one so that never happens, and the core already
> > has a check for 32bit overflow of offsets[]+pitches[]*height.
> > 
> > Testcase: igt/kms_big_fb/x-tiled-addfb-size-offset-overflow
> > Testcase: igt/kms_big_fb/y-tiled-addfb-size-offset-overflow
> > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 2b77d9350a3a..2b474d049074 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2636,9 +2636,10 @@ intel_fill_fb_info(struct drm_i915_private *dev_priv,
> >                 max_size = max(max_size, offset + size);
> >         }
> >  
> > -       if (max_size * tile_size > obj->base.size) {
> > -               DRM_DEBUG_KMS("fb too big for bo (need %u bytes, have %zu bytes)\n",
> > -                             max_size * tile_size, obj->base.size);
> > +       if (mul_u32_u32(max_size, tile_size) > obj->base.size) {
> > +               DRM_DEBUG_KMS("fb too big for bo (need %llu bytes, have %zu bytes)\n",
> > +                             (unsigned long long) mul_u32_u32(max_size, tile_size),
> 
> mul_u32_u32 returns u64 i.e. unsigned long long; %llu is the one true
> format specifier for u64 (Linus decree #103789)

Well whaddyaknow, so it is. Never realized that for some reason.

> 
> Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
> -Chris

-- 
Ville Syrjälä
Intel