[7/7] drm: Sanitize DRM_IOCTL_MODE_CREATE_DUMB input

Commit Message

Thierry Reding Nov. 5, 2014, 1:25 p.m. UTC

From: Thierry Reding <[e-mail]>

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 <[e-mail]>
---
 drivers/gpu/drm/drm_crtc.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Daniel Vetter Nov. 5, 2014, 2:42 p.m. UTC | #1

On Wed, Nov 05, 2014 at 02:25:19PM +0100, Thierry Reding wrote:
> From: Thierry Reding <[e-mail]>
> 
> 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 <[e-mail]>
> ---
>  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 <[e-mail]>

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
>

Patch

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.
+	 */
+	args->handle = 0;
+	args->pitch = 0;
+	args->size = 0;
+
 	return dev->driver->dumb_create(file_priv, dev, args);
 }