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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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@ffwll.ch>
> Signed-off-by: David Herrmann <dh.herrmann@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.

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

-- 
Ville Syrjälä
Intel OTC