[PATCH 16/17] drm/tegra: gem: dumb: pitch and size are outputs

[Thierry Reding]

On Mon, Nov 03, 2014 at 10:51:42AM +0100, Daniel Vetter wrote:
> On Mon, Nov 03, 2014 at 10:27:47AM +0100, Thierry Reding wrote:
> > From: Thierry Reding 
> > 
> > When creating a dumb buffer object using the DRM_IOCTL_MODE_CREATE_DUMB
> > IOCTL, only the width, height, bpp and flags parameters are inputs. The
> > caller is not guaranteed to zero out or set handle, pitch and size, so
> > the driver must not treat these values as possible inputs.
> > 
> > Fixes a bug where running the Weston compositor on Tegra DRM would cause
> > an attempt to allocate a 3 GiB framebuffer to be allocated.
> > 
> > Fixes: de2ba664c30f ("gpu: host1x: drm: Add memory manager and fb")
> > Cc: stable at vger.kernel.org
> > Signed-off-by: Thierry Reding 
> 
> Shouldn't we also clear these fields in the drm core ioctl code? This
> is indeed surprising (yay for lacking input validation!), doing this
> mistake in each driver won't scale ...

They are clearly documented as being outputs in the drm_mode_create_dumb
struct (include/uapi/drm/drm_mode.h), so this was really just me being
stupid a couple of year ago.

But yes, validating the input in the core sounds like a good idea to
avoid this in other drivers in the future.

Thierry
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: 

[PATCH 16/17] drm/tegra: gem: dumb: pitch and size are outputs

[Daniel Vetter]

On Mon, Nov 03, 2014 at 10:27:47AM +0100, Thierry Reding wrote:
> From: Thierry Reding 
> 
> When creating a dumb buffer object using the DRM_IOCTL_MODE_CREATE_DUMB
> IOCTL, only the width, height, bpp and flags parameters are inputs. The
> caller is not guaranteed to zero out or set handle, pitch and size, so
> the driver must not treat these values as possible inputs.
> 
> Fixes a bug where running the Weston compositor on Tegra DRM would cause
> an attempt to allocate a 3 GiB framebuffer to be allocated.
> 
> Fixes: de2ba664c30f ("gpu: host1x: drm: Add memory manager and fb")
> Cc: stable at vger.kernel.org
> Signed-off-by: Thierry Reding 

Shouldn't we also clear these fields in the drm core ioctl code? This
is indeed surprising (yay for lacking input validation!), doing this
mistake in each driver won't scale ...
-Daniel

> ---
>  drivers/gpu/drm/tegra/gem.c | 10 +++---
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tegra/gem.c b/drivers/gpu/drm/tegra/gem.c
> index 8b1095d05c58..8348783f7d64 100644
> --- a/drivers/gpu/drm/tegra/gem.c
> +++ b/drivers/gpu/drm/tegra/gem.c
> @@ -399,16 +399,12 @@ void tegra_bo_free_object(struct drm_gem_object *gem)
>  int tegra_bo_dumb_create(struct drm_file *file, struct drm_device *drm,
>struct drm_mode_create_dumb *args)
>  {
> - int min_pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
> + unsigned int min_pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
>   struct tegra_drm *tegra = drm->dev_private;
>   struct tegra_bo *bo;
>  
> - min_pitch = round_up(min_pitch, tegra->pitch_align);
> - if (args->pitch < min_pitch)
> - args->pitch = min_pitch;
> -
> - if (args->size < args->pitch * args->height)
> - args->size = args->pitch * args->height;
> + args->pitch = round_up(min_pitch, tegra->pitch_align);
> + args->size = args->pitch * args->height;
>  
>   bo = tegra_bo_create_with_handle(file, drm, args->size, 0,
>>handle);
> -- 
> 2.1.2
> 
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 16/17] drm/tegra: gem: dumb: pitch and size are outputs

[Thierry Reding]

From: Thierry Reding 

When creating a dumb buffer object using the DRM_IOCTL_MODE_CREATE_DUMB
IOCTL, only the width, height, bpp and flags parameters are inputs. The
caller is not guaranteed to zero out or set handle, pitch and size, so
the driver must not treat these values as possible inputs.

Fixes a bug where running the Weston compositor on Tegra DRM would cause
an attempt to allocate a 3 GiB framebuffer to be allocated.

Fixes: de2ba664c30f ("gpu: host1x: drm: Add memory manager and fb")
Cc: stable at vger.kernel.org
Signed-off-by: Thierry Reding 
---
 drivers/gpu/drm/tegra/gem.c | 10 +++---
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/tegra/gem.c b/drivers/gpu/drm/tegra/gem.c
index 8b1095d05c58..8348783f7d64 100644
--- a/drivers/gpu/drm/tegra/gem.c
+++ b/drivers/gpu/drm/tegra/gem.c
@@ -399,16 +399,12 @@ void tegra_bo_free_object(struct drm_gem_object *gem)
 int tegra_bo_dumb_create(struct drm_file *file, struct drm_device *drm,
 struct drm_mode_create_dumb *args)
 {
-   int min_pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
+   unsigned int min_pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
struct tegra_drm *tegra = drm->dev_private;
struct tegra_bo *bo;

-   min_pitch = round_up(min_pitch, tegra->pitch_align);
-   if (args->pitch < min_pitch)
-   args->pitch = min_pitch;
-
-   if (args->size < args->pitch * args->height)
-   args->size = args->pitch * args->height;
+   args->pitch = round_up(min_pitch, tegra->pitch_align);
+   args->size = args->pitch * args->height;

bo = tegra_bo_create_with_handle(file, drm, args->size, 0,
 >handle);
-- 
2.1.2