Re: [PATCH] drm/i915: Make sure fb gtt offsets stay within 32bits

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Sep 20, 2018 at 09:07:35PM +0100, Chris Wilson wrote:
> Quoting Ville Syrjala (2018-09-20 20:10:18)
> > From: Ville Syrjälä <ville.syrjala@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.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 18 +++++++++++++++++-
> >  1 file changed, 17 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index e642b7717106..67259c719ffe 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 (mul_u32_u32(height, fb->pitches[color_plane]) +
> 
> if (add_overflows(mul_u32_u32(height, fb->pitches[color_plane]),
> 		  fb->offsets[color_plane],
> 		  U32_MAX) {
> ?

Didn't know we had that. Looks like what we have right now doesn't quite
work as it only takes two arguments.

Oh interesting. __builtin_add_overflow_p() & co. work supposedly work on
bitfields as well.

Bah. Looks like this stuff generatess worse code than the normal thing:

    401e:       f7 e3                   mul    %ebx
    4020:       89 45 ac                mov    %eax,-0x54(%ebp)
    4023:       89 c8                   mov    %ecx,%eax
    4025:       89 55 b0                mov    %edx,-0x50(%ebp)
    4028:       31 d2                   xor    %edx,%edx
    402a:       03 45 ac                add    -0x54(%ebp),%eax
    402d:       13 55 b0                adc    -0x50(%ebp),%edx
    4030:       83 fa 00                cmp    $0x0,%edx
    4033:       0f 87 87 02 00 00       ja     42c0 <intel_framebuffer_init+0xb50>

vs.

    401a:       f7 65 b8                mull   -0x48(%ebp)
    401d:       89 45 ac                mov    %eax,-0x54(%ebp)
    4020:       89 d8                   mov    %ebx,%eax
    4022:       89 55 b0                mov    %edx,-0x50(%ebp)
    4025:       31 d2                   xor    %edx,%edx
    4027:       03 45 ac                add    -0x54(%ebp),%eax
    402a:       13 55 b0                adc    -0x50(%ebp),%edx
    402d:       3b 55 b0                cmp    -0x50(%ebp),%edx
    4030:       77 0c                   ja     403e <intel_framebuffer_init+0x8ce>
    4032:       72 05                   jb     4039 <intel_framebuffer_init+0x8c9>
    4034:       3b 45 ac                cmp    -0x54(%ebp),%eax
    4037:       73 05                   jae    403e <intel_framebuffer_init+0x8ce>
    4039:       b9 01 00 00 00          mov    $0x1,%ecx
    403e:       85 d2                   test   %edx,%edx
    4040:       0f 85 9a 02 00 00       jne    42e0 <intel_framebuffer_init+0xb70>
    4046:       85 c9                   test   %ecx,%ecx
    4048:       0f 85 92 02 00 00       jne    42e0 <intel_framebuffer_init+0xb70>

So five branches instead of just the one. That seems a bit excessive.

> 
> > +           fb->offsets[color_plane] > UINT_MAX) {
> > +               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;
> > +       }
> >  
> >         *x = 0;
> >         *y = 0;
> > -- 
> > 2.16.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx