ping
On Thu, Jan 23, 2014 at 3:10 PM, David Herrmann <dh.herrmann@xxxxxxxxx> wrote:
> Hi
>
> On Thu, Jan 23, 2014 at 2:55 PM, Ville Syrjälä
> <ville.syrjala@xxxxxxxxxxxxxxx> 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@xxxxxxxx>
>>> Signed-off-by: David Herrmann <dh.herrmann@xxxxxxxxx>
>>> ---
>>> 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@xxxxxxxxxxxxxxxxxxxxx
>>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
>> --
>> Ville Syrjälä
>> Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
