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@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.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
