[PATCH] drm/qxl: fix 32-bit overflow in the ALLOC_SURF size computation

[PATCH] drm/qxl: fix 32-bit overflow in the ALLOC_SURF size computation

[Aldo Ariel Panzardo]

qxl_alloc_surf_ioctl() works out the backing size of a surface with

	actual_stride = param->stride < 0 ? -param->stride : param->stride;
	size = actual_stride * param->height + actual_stride;

where size and actual_stride are int and param->height is __u32.  Every
operand comes straight from userspace through DRM_IOCTL_QXL_ALLOC_SURF,
which is DRM_AUTH, and the expression is evaluated modulo 2^32 with no
overflow check.

The wrapped value is what reaches qxl_bo_create(), which only rounds it
up to a page.  The original width, height and stride are kept verbatim
in bo->surf and are later handed to the device by qxl_hw_surface_alloc()
together with the address of that undersized allocation, so the driver
tells the host about a surface far larger than the memory backing it.

For example stride=4096, height=1048576 gives
4096 * (1048576 + 1) = 0x1_0000_1000, which truncates to 4096: a
one-page buffer object described to the device as a 4 GiB surface.
Measured on 6.12.101 by probing mmap() lengths against the resulting
GEM object, the backing is 4096 bytes while the surface declared to the
device is 4294967296 bytes.

Two smaller problems are fixed at the same time.  Negating param->stride
is undefined for INT_MIN, and unlike QXL_ALLOC there is no rejection of
a zero-sized request.

Compute the size in a type that cannot wrap, reject INT_MIN and zero
dimensions, and bound the result so it still fits the int parameter of
qxl_gem_object_create().

Fixes: f64122c1f6ad ("drm: add new QXL driver. (v1.4)")
Cc: stable@vger.kernel.org
Signed-off-by: Aldo Ariel Panzardo <qwe.aldo@gmail.com>
---
 drivers/gpu/drm/qxl/qxl_ioctl.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_ioctl.c b/drivers/gpu/drm/qxl/qxl_ioctl.c
--- a/drivers/gpu/drm/qxl/qxl_ioctl.c
+++ b/drivers/gpu/drm/qxl/qxl_ioctl.c
@@ -23,6 +23,7 @@
  *          Alon Levy
  */
 
+#include <linux/overflow.h>
 #include <linux/pci.h>
 #include <linux/uaccess.h>
 
@@ -386,12 +387,26 @@
 	struct drm_qxl_alloc_surf *param = data;
 	int handle;
 	int ret;
-	int size, actual_stride;
+	int actual_stride;
+	size_t size;
 	struct qxl_surface surf;
 
 	/* work out size allocate bo with handle */
+	if (param->stride == INT_MIN)
+		return -EINVAL;
 	actual_stride = param->stride < 0 ? -param->stride : param->stride;
-	size = actual_stride * param->height + actual_stride;
+	if (!actual_stride || !param->width || !param->height)
+		return -EINVAL;
+	/*
+	 * size = actual_stride * (height + 1), evaluated in a type that cannot
+	 * wrap, then bounded so it survives the int parameter of
+	 * qxl_gem_object_create().
+	 */
+	if (check_mul_overflow((size_t)actual_stride,
+			       (size_t)param->height + 1, &size))
+		return -EINVAL;
+	if (size > INT_MAX)
+		return -EINVAL;
 
 	surf.format = param->format;
 	surf.width = param->width;

Re: [PATCH] drm/qxl: fix 32-bit overflow in the ALLOC_SURF size computation

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] The integer overflow fix for surface size computation remains vulnerable on 32-bit architectures due to wrap-around in the addition operation before the overflow check.

Pre-existing issues:
- [High] A memory leak of a GEM object occurs if handle creation fails in `qxl_gem_object_create_with_handle()`.
--

--- Patch [1]: [PATCH] drm/qxl: fix 32-bit overflow in the ALLOC_SURF size computation ---
commit 49a28e971687973dbde5077170aba70504fc31af
Author: Aldo Ariel Panzardo <qwe.aldo@gmail.com>

drm/qxl: fix 32-bit overflow in the ALLOC_SURF size computation
    
The patch attempts to fix an integer overflow in the QXL_ALLOC_SURF ioctl
by checking for multiplication overflow and bounding the result, but some
edge cases remain.

Cc: stable@vger.kernel.org

> diff --git a/drivers/gpu/drm/qxl/qxl_ioctl.c b/drivers/gpu/drm/qxl/qxl_ioctl.c
> index 591b026ceff9c..f245ead003802 100644
> --- a/drivers/gpu/drm/qxl/qxl_ioctl.c
> +++ b/drivers/gpu/drm/qxl/qxl_ioctl.c

[ ... ]

> @@ -386,12 +387,26 @@ int qxl_alloc_surf_ioctl(struct drm_device *dev, void *data, struct drm_file *fi
>  	struct drm_qxl_alloc_surf *param = data;
>  	int handle;
>  	int ret;
> -	int size, actual_stride;
> +	int actual_stride;
> +	size_t size;
>  	struct qxl_surface surf;
>  
>  	/* work out size allocate bo with handle */
> +	if (param->stride == INT_MIN)
> +		return -EINVAL;
>  	actual_stride = param->stride < 0 ? -param->stride : param->stride;
> -	size = actual_stride * param->height + actual_stride;
> +	if (!actual_stride || !param->width || !param->height)
> +		return -EINVAL;
> +	/*
> +	 * size = actual_stride * (height + 1), evaluated in a type that cannot
> +	 * wrap, then bounded so it survives the int parameter of
> +	 * qxl_gem_object_create().
> +	 */
> +	if (check_mul_overflow((size_t)actual_stride,
> +			       (size_t)param->height + 1, &size))
> +		return -EINVAL;

[Severity: High]
On 32-bit platforms where size_t is 32 bits, can the addition
(size_t)param->height + 1 wrap around before the overflow check occurs?

For example, if userspace passes 0xFFFFFFFF for height, does this evaluate
to 0, resulting in size = 0 and bypassing the overflow protection entirely?

> +	if (size > INT_MAX)
> +		return -EINVAL;
>  
>  	surf.format = param->format;
>  	surf.width = param->width;

[Severity: High]
This is a pre-existing issue, but does qxl_gem_object_create_with_handle()
leak the GEM object if handle creation fails?

Further down in qxl_alloc_surf_ioctl(), we call 
qxl_gem_object_create_with_handle(). Looking at that function in 
drivers/gpu/drm/qxl/qxl_gem.c:

int qxl_gem_object_create_with_handle(...)
{
        ...
        r = drm_gem_handle_create(file_priv, local_gobj, handle);
        if (r)
                return r;
        ...
}

If drm_gem_handle_create() fails, it returns without calling
drm_gem_object_put() to drop the initial reference on local_gobj. Could
a malicious userspace program repeatedly allocate surfaces until handle
creation fails to exhaust memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260808230945.67525-1-qwe.aldo@gmail.com?part=1