All of lore.kernel.org
 help / color / mirror / Atom feed
* [patch 4/4] drm/qxl: integer overflow in qxl_alloc_surf_ioctl()
@ 2015-09-17 11:24 Dan Carpenter
  2015-09-18  9:46 ` Frediano Ziglio
  0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2015-09-17 11:24 UTC (permalink / raw)
  To: David Airlie
  Cc: Dave Airlie, security, Ilja Van Sprundel, dri-devel,
	Frediano Ziglio

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

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [patch 4/4] drm/qxl: integer overflow in qxl_alloc_surf_ioctl()
  2015-09-17 11:24 [patch 4/4] drm/qxl: integer overflow in qxl_alloc_surf_ioctl() Dan Carpenter
@ 2015-09-18  9:46 ` Frediano Ziglio
  2015-09-22 14:32     ` Dan Carpenter
  0 siblings, 1 reply; 6+ messages in thread
From: Frediano Ziglio @ 2015-09-18  9:46 UTC (permalink / raw)
  To: Dan Carpenter; +Cc: Dave Airlie, security, Ilja Van Sprundel, dri-devel

> 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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [patch 4/4 v2] drm/qxl: integer overflow in qxl_alloc_surf_ioctl()
  2015-09-18  9:46 ` Frediano Ziglio
@ 2015-09-22 14:32     ` Dan Carpenter
  0 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2015-09-22 14:32 UTC (permalink / raw)
  To: David Airlie; +Cc: Dave Airlie, kernel-janitors, dri-devel, Frediano Ziglio

The size calculation can overflow.  I don't know if this leads to
memory corruption, but it causes a static checker warning.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2: I don't know think the size is capped anywhere.  In my first version
of this patch, I introduced a divide by zero bug.

diff --git a/drivers/gpu/drm/qxl/qxl_ioctl.c b/drivers/gpu/drm/qxl/qxl_ioctl.c
index b2db482..49b3158 100644
--- a/drivers/gpu/drm/qxl/qxl_ioctl.c
+++ b/drivers/gpu/drm/qxl/qxl_ioctl.c
@@ -396,12 +396,14 @@ static int qxl_alloc_surf_ioctl(struct drm_device *dev, void *data,
 	struct qxl_bo *qobj;
 	int handle;
 	int ret;
-	int size, actual_stride;
+	u64 size, actual_stride;
 	struct qxl_surface surf;
 
 	/* work out size allocate bo with handle */
 	actual_stride = param->stride < 0 ? -param->stride : param->stride;
 	size = actual_stride * param->height + actual_stride;
+	if (size > INT_MAX)
+		return -EINVAL;
 
 	surf.format = param->format;
 	surf.width = param->width;

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [patch 4/4 v2] drm/qxl: integer overflow in qxl_alloc_surf_ioctl()
@ 2015-09-22 14:32     ` Dan Carpenter
  0 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2015-09-22 14:32 UTC (permalink / raw)
  To: David Airlie; +Cc: Dave Airlie, kernel-janitors, dri-devel, Frediano Ziglio

The size calculation can overflow.  I don't know if this leads to
memory corruption, but it causes a static checker warning.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2: I don't know think the size is capped anywhere.  In my first version
of this patch, I introduced a divide by zero bug.

diff --git a/drivers/gpu/drm/qxl/qxl_ioctl.c b/drivers/gpu/drm/qxl/qxl_ioctl.c
index b2db482..49b3158 100644
--- a/drivers/gpu/drm/qxl/qxl_ioctl.c
+++ b/drivers/gpu/drm/qxl/qxl_ioctl.c
@@ -396,12 +396,14 @@ static int qxl_alloc_surf_ioctl(struct drm_device *dev, void *data,
 	struct qxl_bo *qobj;
 	int handle;
 	int ret;
-	int size, actual_stride;
+	u64 size, actual_stride;
 	struct qxl_surface surf;
 
 	/* work out size allocate bo with handle */
 	actual_stride = param->stride < 0 ? -param->stride : param->stride;
 	size = actual_stride * param->height + actual_stride;
+	if (size > INT_MAX)
+		return -EINVAL;
 
 	surf.format = param->format;
 	surf.width = param->width;
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [patch 4/4 v2] drm/qxl: integer overflow in qxl_alloc_surf_ioctl()
  2015-09-22 14:32     ` Dan Carpenter
@ 2015-09-23 10:45       ` Frediano Ziglio
  -1 siblings, 0 replies; 6+ messages in thread
From: Frediano Ziglio @ 2015-09-23 10:45 UTC (permalink / raw)
  To: Dan Carpenter; +Cc: Dave Airlie, kernel-janitors, dri-devel

> 
> The size calculation can overflow.  I don't know if this leads to
> memory corruption, but it causes a static checker warning.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> v2: I don't know think the size is capped anywhere.  In my first version
> of this patch, I introduced a divide by zero bug.
> 

Beside the second sentence I would ack

Frediano

> diff --git a/drivers/gpu/drm/qxl/qxl_ioctl.c
> b/drivers/gpu/drm/qxl/qxl_ioctl.c
> index b2db482..49b3158 100644
> --- a/drivers/gpu/drm/qxl/qxl_ioctl.c
> +++ b/drivers/gpu/drm/qxl/qxl_ioctl.c
> @@ -396,12 +396,14 @@ static int qxl_alloc_surf_ioctl(struct drm_device *dev,
> void *data,
>  	struct qxl_bo *qobj;
>  	int handle;
>  	int ret;
> -	int size, actual_stride;
> +	u64 size, actual_stride;
>  	struct qxl_surface surf;
>  
>  	/* work out size allocate bo with handle */
>  	actual_stride = param->stride < 0 ? -param->stride : param->stride;
>  	size = actual_stride * param->height + actual_stride;
> +	if (size > INT_MAX)
> +		return -EINVAL;
>  
>  	surf.format = param->format;
>  	surf.width = param->width;
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [patch 4/4 v2] drm/qxl: integer overflow in qxl_alloc_surf_ioctl()
@ 2015-09-23 10:45       ` Frediano Ziglio
  0 siblings, 0 replies; 6+ messages in thread
From: Frediano Ziglio @ 2015-09-23 10:45 UTC (permalink / raw)
  To: Dan Carpenter; +Cc: Dave Airlie, kernel-janitors, dri-devel

> 
> The size calculation can overflow.  I don't know if this leads to
> memory corruption, but it causes a static checker warning.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> v2: I don't know think the size is capped anywhere.  In my first version
> of this patch, I introduced a divide by zero bug.
> 

Beside the second sentence I would ack

Frediano

> diff --git a/drivers/gpu/drm/qxl/qxl_ioctl.c
> b/drivers/gpu/drm/qxl/qxl_ioctl.c
> index b2db482..49b3158 100644
> --- a/drivers/gpu/drm/qxl/qxl_ioctl.c
> +++ b/drivers/gpu/drm/qxl/qxl_ioctl.c
> @@ -396,12 +396,14 @@ static int qxl_alloc_surf_ioctl(struct drm_device *dev,
> void *data,
>  	struct qxl_bo *qobj;
>  	int handle;
>  	int ret;
> -	int size, actual_stride;
> +	u64 size, actual_stride;
>  	struct qxl_surface surf;
>  
>  	/* work out size allocate bo with handle */
>  	actual_stride = param->stride < 0 ? -param->stride : param->stride;
>  	size = actual_stride * param->height + actual_stride;
> +	if (size > INT_MAX)
> +		return -EINVAL;
>  
>  	surf.format = param->format;
>  	surf.width = param->width;
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-09-23 10:45 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-09-17 11:24 [patch 4/4] drm/qxl: integer overflow in qxl_alloc_surf_ioctl() Dan Carpenter
2015-09-18  9:46 ` Frediano Ziglio
2015-09-22 14:32   ` [patch 4/4 v2] " Dan Carpenter
2015-09-22 14:32     ` Dan Carpenter
2015-09-23 10:45     ` Frediano Ziglio
2015-09-23 10:45       ` Frediano Ziglio

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.