drm/qxl: integer overflow in qxl_alloc_surf_ioctl()
Subsystems (4)
PATCH 4/4 drm/qxl: integer overflow in qxl_alloc_surf_ioctl()
The size calculation can overflow.
drivers/gpu/drm/qxl/qxl_ioctl.c +2 -0
@@ -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
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Quoted 18 lines
The size calculation can overflow.
Signed-off-by Dan Carpenter <[email protected]>drivers/gpu/drm/qxl/qxl_ioctl.c +2
-0@@ -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;
Quoted 2 lines
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
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
PATCH v2 4/4 drm/qxl: integer overflow in qxl_alloc_surf_ioctl()
The size calculation can overflow. I don't know if this leads to memory corruption, but it causes a static checker warning.
---
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.
drivers/gpu/drm/qxl/qxl_ioctl.c +3 -1
@@ -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;
Quoted 9 lines
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 <[email protected]>---
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
Quoted 23 lines
drivers/gpu/drm/qxl/qxl_ioctl.c +3
-1@@ -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;