Re: [patch 4/4] drm/qxl: integer overflow in qxl_alloc_surf_ioctl()

[Frediano Ziglio <fziglio@redhat.com>]

> The size calculation can overflow.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/drivers/gpu/drm/qxl/qxl_ioctl.c
> b/drivers/gpu/drm/qxl/qxl_ioctl.c
> index 552dc06..5da9a60 100644
> --- a/drivers/gpu/drm/qxl/qxl_ioctl.c
> +++ b/drivers/gpu/drm/qxl/qxl_ioctl.c
> @@ -401,6 +401,8 @@ static int qxl_alloc_surf_ioctl(struct drm_device *dev,
> void *data,
>  
>  	/* work out size allocate bo with handle */
>  	actual_stride = param->stride < 0 ? -param->stride : param->stride;
> +	if (actual_stride > (INT_MAX - actual_stride) / param->height)
> +		return -EINVAL;
>  	size = actual_stride * param->height + actual_stride;
>  
>  	surf.format = param->format;
> 

There are some issues here. param->stride can be -2^31 so actual_stride will
be probably -2^31. You introduced a possible division by zero (param->height == 0).
Do you know who validate the structure size? I don't see any check on the code.
Looks like the data pointer is coming from userspace untested.
An easiest way to do these check is use 64 bit arithmetic so something like

   u64 actual_stride, size;

   actual_stride = param->stride < 0 ? -param->stride : param->stride;
   size = actual_stride * param->height + actual_stride;
   if (size > INT_MAX)
       return -EINVAL;

take into account that usually division is quite slow so on many system even if 32 bit
the multiplication is faster. And if is not faster you are creating a surface so
is not really a fast path.

You should also check if the stride computed from width and format fit into the stride
provided.

Do you know why actual_stride * param->height is not enough? Why an extra line?

Regards,
  Frediano
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel