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

[sashiko-bot@kernel.org]

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