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

Commit Message

David Herrmann Jan. 23, 2014, 12:53 p.m. UTC

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 <[e-mail]>
Signed-off-by: David Herrmann <[e-mail]>
---
 drivers/gpu/drm/drm_crtc.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

Comments

Ville Syrjälä Jan. 23, 2014, 1:55 p.m. UTC | #1

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 <[e-mail]>
> Signed-off-by: David Herrmann <[e-mail]>
> ---
>  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.

> +	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
> [e-mail]
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

David Herrmann Jan. 23, 2014, 2:10 p.m. UTC | #2

Hi

On Thu, Jan 23, 2014 at 2:55 PM, Ville Syrjälä
<[e-mail]> 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 <[e-mail]>
>> Signed-off-by: David Herrmann <[e-mail]>
>> ---
>>  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
>> [e-mail]
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Ville Syrjälä
> Intel OTC

David Herrmann Feb. 5, 2014, 9:29 p.m. UTC | #3

ping

On Thu, Jan 23, 2014 at 3:10 PM, David Herrmann <[e-mail]> wrote:
> Hi
>
> On Thu, Jan 23, 2014 at 2:55 PM, Ville Syrjälä
> <[e-mail]> 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 <[e-mail]>
>>> Signed-off-by: David Herrmann <[e-mail]>
>>> ---
>>>  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
>>> [e-mail]
>>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
>> --
>> Ville Syrjälä
>> Intel OTC

Patch

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;
+	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);
 }