Re: [PATCH 7/7] drm: Sanitize DRM_IOCTL_MODE_CREATE_DUMB input

From: Daniel Vetter <daniel@ffwll.ch>
To: Thierry Reding <thierry.reding@gmail.com>
Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org>,
	Daniel Vetter <daniel.vetter@ffwll.ch>,
	dri-devel@lists.freedesktop.org,
	Tomi Valkeinen <tomi.valkeinen@ti.com>,
	Archit Taneja <archit@ti.com>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	Russell King <rmk+kernel@arm.linux.org.uk>,
	Dave Airlie <airlied@redhat.com>
Subject: Re: [PATCH 7/7] drm: Sanitize DRM_IOCTL_MODE_CREATE_DUMB input
Date: Wed, 5 Nov 2014 15:42:07 +0100	[thread overview]
Message-ID: <20141105144207.GS26941@phenom.ffwll.local> (raw)
In-Reply-To: <1415193919-1687-8-git-send-email-thierry.reding@gmail.com>

On Wed, Nov 05, 2014 at 02:25:19PM +0100, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> Some drivers treat the pitch and size fields as inputs and will use them
> as minima provided by userspace so that they are only overwritten if the
> minimal requirements of the driver exceed them.
> 
> This can cause strange behaviour when applications don't zero out these
> fields, causing whatever was on the stack to be passed to the IOCTL. In
> a typical case this would become visible as a failed allocation if the
> pitch or size were unusually high. But this could also cause more subtle
> bugs like overallocating dumb framebuffers.
> 
> To prevent drivers from misusing these values, make the DRM core zero
> out the pitch and size fields before passing the structure to the driver
> implementation.
> 
> While at it, also set the output handle field to zero for good measure,
> even though it's less likely to be abused.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/gpu/drm/drm_crtc.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 0f3c24c0981b..6aceb689ccea 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -4755,6 +4755,14 @@ int drm_mode_create_dumb_ioctl(struct drm_device *dev,
>  	if (PAGE_ALIGN(size) == 0)
>  		return -EINVAL;
>  
> +	/*
> +	 * handle, pitch and size are output parameters. Zero them out to
> +	 * prevent drivers from accidentally using uninitialized data.

Maybe add: Unfortunately we can't reject ioctls with garbage in them since
existing userspace is not clearing these fields properly.

With that comment: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

That way it's clear that we can never reuse these fields for flags or
anything at all. Also a good reminder for folks that they really should
have if (args->foo) return -EINVAL for any reserved, unused or output-only
fields.
-Daniel

> +	 */
> +	args->handle = 0;
> +	args->pitch = 0;
> +	args->size = 0;
> +
>  	return dev->driver->dumb_create(file_priv, dev, args);
>  }
>  
> -- 
> 2.1.3
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel