Re: [PATCH 6/7] drm/crtc: add sanity checks to create_dumb()

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

On Tue, Jan 21, 2014 at 12:17:35PM +0100, David Herrmann wrote:
> Hi
> 
> On Tue, Jan 21, 2014 at 10:49 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Mon, Jan 20, 2014 at 08:26:28PM +0100, David Herrmann wrote:
> >> Lets make sure some basic expressions are always true:
> >>   bpp != NULL
> >>   width != NULL
> >>   height != NULL
> >>   stride = bpp * width < 2^32
> >>   size = stride * height < 2^32
> >>   PAGE_ALIGN(size) < 2^32
> >>
> >> At least the udl driver doesn't check for multiplication-overflows, so
> >> lets just make sure it will never happen. These checks allow drivers to do
> >> any 32bit math without having to test for mult-overflows themselves.
> >>
> >> The two divisions might hurt performance a bit, but dumb_create() is only
> >> used for scanout-buffers, so that should be fine. We could use 64bit math
> >> to avoid the divisions, but that may be slow on 32bit machines.. Or maybe
> >> there should just be a "safe_mult32()" helper, which currently doesn't
> >> exist (I think?).
> >>
> >> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> >> ---
> >>  drivers/gpu/drm/drm_crtc.c | 15 +++++++++++++++
> >>  1 file changed, 15 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> >> index 266a01d..ff647fa 100644
> >> --- a/drivers/gpu/drm/drm_crtc.c
> >> +++ b/drivers/gpu/drm/drm_crtc.c
> >> @@ -3738,9 +3738,24 @@ int drm_mode_create_dumb_ioctl(struct drm_device *dev,
> >>                              void *data, struct drm_file *file_priv)
> >>  {
> >>       struct drm_mode_create_dumb *args = data;
> >> +     u32 Bpp, stride, size;
> >
> > s/Bpp/bpp/
> 
> It's actually "Bytes per pixel", not "Bits per pixel". We generally
> use "bpp" as "bits per pixel" so I'd like to avoid the confusion. But
> yeah, upper-case names are unusual so I can also use bytes_pp or sth
> similar?

cpp is fairly commonly used for bytes per pixel, if you want to stick to
something short but still recognizable.

> 
> >>
> >>       if (!dev->driver->dumb_create)
> >>               return -ENOSYS;
> >> +     if (!args->width || !args->height || !args->bpp)
> >> +             return -EINVAL;
> >> +
> >> +     /* overflow checks for 32bit size calculations */
> >> +     Bpp = (args->bpp + 7) / 8;
> >
> > Again DIV_ROUND_UP
> 
> yepp, fixed.
> 
> >
> >> +     if (Bpp > 0xffffffffU / args->width)
> >> +             return -EINVAL;
> >> +     stride = Bpp * args->width;
> >> +     if (args->height > 0xffffffffU / stride)
> >> +             return -EINVAL;
> >> +     size = args->height * stride;
> >> +     if (PAGE_ALIGN(size) < size)
> >
> > Maybe check for 0 here and add a small comment that this checks
> > wrap-around.
> 
> Hm, the comment above those if()s already says "overflow checks", and
> it should be obvious that all three are overflow checks, so I don't
> think we need another comment (especially because I didn't add any
> empty lines between them).
> 
> I will change it to "if (!PAGE_ALIGN(size))". The "x + off < x" is an
> addition-overflow-check so I think it doesn't apply here.
> 
> Thanks
> David
> 
> >
> >> +             return -EINVAL;
> >> +
> >>       return dev->driver->dumb_create(file_priv, dev, args);
> >>  }
> >>
> >> --
> >> 1.8.5.3
> >>
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC