[PATCH v2 5/6] drm/crtc: add sanity checks to create_dumb()

[David Herrmann]

ping

On Thu, Jan 23, 2014 at 3:10 PM, David Herrmann <dh.herrmann at gmail.com> wrote:
> Hi
>
> On Thu, Jan 23, 2014 at 2:55 PM, Ville Syrjälä
> <ville.syrjala at linux.intel.com> wrote:
>> On Thu, Jan 23, 2014 at 01:53:15PM +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?).
>>>
>>> Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>
>>> Signed-off-by: David Herrmann <dh.herrmann at gmail.com>
>>> ---
>>>  drivers/gpu/drm/drm_crtc.c | 17 +++++++++++++++++
>>>  1 file changed, 17 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>>> index 266a01d..2b50702 100644
>>> --- a/drivers/gpu/drm/drm_crtc.c
>>> +++ b/drivers/gpu/drm/drm_crtc.c
>>> @@ -3738,9 +3738,26 @@ 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 cpp, stride, size;
>>>
>>>       if (!dev->driver->dumb_create)
>>>               return -ENOSYS;
>>> +     if (!args->width || !args->height || !args->bpp)
>>> +             return -EINVAL;
>>> +
>>> +     /* overflow checks for 32bit size calculations */
>>> +     cpp = DIV_ROUND_UP(args->bpp, 8);
>>> +     if (cpp > 0xffffffffU / args->width)
>>> +             return -EINVAL;
>>
>> IIRC I used -ERANGE for such things in some drm_framebuffer checks. Not
>> sure if that's the best error value or not, but using the same one in
>> both places would be nice.
>
> I'm actually a fan of EINVAL here, as this is really more about "do
> the arguments make sense?" than "does the range exceed the driver's
> capabilities?" But what do I know.. So more comments on that are
> welcome. And btw., we already mix EINVAL and ERANGE so this would just
> contribute to the already existing mess (see the stride verifications
> which usually return EINVAL, but the overflow checks often return
> ERANGE).
>
> Thanks
> David
>
>>> +     stride = cpp * args->width;
>>> +     if (args->height > 0xffffffffU / stride)
>>> +             return -EINVAL;
>>> +
>>> +     /* test for wrap-around */
>>> +     size = args->height * stride;
>>> +     if (PAGE_ALIGN(size) == 0)
>>> +             return -EINVAL;
>>> +
>>>       return dev->driver->dumb_create(file_priv, dev, args);
>>>  }
>>>
>>> --
>>> 1.8.5.3
>>>
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel at lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
>> --
>> Ville Syrjälä
>> Intel OTC