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

[David Herrmann]

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