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 <[email protected]> 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: [email protected] > 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/[email protected]?part=1
