Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] drm/gem: signed int truncation in drm_gem_change_handle_ioctl() - [Medium] drm/gem: broken PRIME handling in drm_gem_change_handle_ioctl() --
commit 69ca138819f06e34ad7cfcd2fb35830a526fdf7a Author: David Francis <[email protected]> drm/gem: Fix and enable CHANGE_HANDLE This commit updates drm_gem_change_handle_ioctl() to reject a new_handle of 0 or equal to the existing handle, adds idr_preload() around idr_alloc(), and re-enables DRM_IOCTL_GEM_CHANGE_HANDLE in the core DRM ioctl table. > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c > index 3ce38df7b8b85..6973bf69dd439 100644 > --- a/drivers/gpu/drm/drm_gem.c > +++ b/drivers/gpu/drm/drm_gem.c [ ... ] > @@ -1040,21 +1027,22 @@ int drm_gem_change_handle_ioctl(struct drm_device > *dev, void *data, > if (!drm_core_check_feature(dev, DRIVER_GEM)) > return -EOPNOTSUPP; > > - /* idr_alloc() limitation. */ > - if (args->new_handle > INT_MAX) > - return -EINVAL; > new_handle = args->new_handle; > > - if (args->handle == new_handle) > - return 0; > + /* idr_alloc() limitations. */ > + if (new_handle == 0 || new_handle > INT_MAX || > + args->handle == new_handle) > + return -EINVAL; > > mutex_lock(&file_priv->prime.lock); > + idr_preload(GFP_KERNEL); > spin_lock(&file_priv->table_lock); > - ret = idr_alloc(&file_priv->object_idr, NULL, new_handle, new_handle + > 1, > - GFP_NOWAIT); > + ret = idr_alloc(&file_priv->object_idr, NULL, new_handle, > + new_handle + 1, GFP_NOWAIT); [Severity: High] Does moving new_handle = args->new_handle ahead of the INT_MAX check allow negative values to bypass validation and trigger a warning in idr_alloc()? In struct drm_gem_change_handle, args->new_handle is __u32, whereas the local variable new_handle in drm_gem_change_handle_ioctl() is a signed int. Moving the assignment before the bounds check makes new_handle > INT_MAX tautologically false. When userspace passes args->new_handle >= 0x80000000 (with args->handle != args->new_handle), new_handle wraps to a negative value, passes the check, and is passed as start to idr_alloc(): lib/idr.c:idr_alloc() { if (WARN_ON_ONCE(start < 0)) return -EINVAL; } This reintroduces the warning fixed by commit 12f15d52d38a, and evaluating new_handle + 1 when args->new_handle == INT_MAX also overflows signed int. Should new_handle be declared as u32 (or checked against args->new_handle before assignment)? > > if (ret < 0) { > spin_unlock(&file_priv->table_lock); > + idr_preload_end(); > goto out_unlock; > } > > @@ -1062,10 +1050,12 @@ int drm_gem_change_handle_ioctl(struct drm_device > *dev, void *data, > if (IS_ERR_OR_NULL(obj)) { > idr_remove(&file_priv->object_idr, new_handle); > spin_unlock(&file_priv->table_lock); > + idr_preload_end(); > ret = -ENOENT; > goto out_unlock; > } > spin_unlock(&file_priv->table_lock); > + idr_preload_end(); > > if (obj->dma_buf) { > ret = drm_prime_add_buf_handle(&file_priv->prime, obj->dma_buf, [Severity: Medium] With DRM_IOCTL_GEM_CHANGE_HANDLE now enabled in drm_ioctls[], is it safe for drm_gem_change_handle_ioctl() to read obj->dma_buf and update file_priv->prime here? First, struct drm_gem_object documents that obj->dma_buf is protected by dev->object_name_lock: drivers/gpu/drm/drm_prime.c:export_and_register_object() { ... obj->dma_buf = dmabuf; get_dma_buf(obj->dma_buf); } Reading obj->dma_buf under only file_priv->prime.lock can race with another drm_file exporting the shared object under dev->object_name_lock. Second, checking if (obj->dma_buf) tests whether the underlying GEM object has an associated dma_buf rather than whether args->handle is registered in this file's PRIME cache (via drm_prime_lookup_buf_by_handle(&file_priv->prime, args->handle)). If a client holds a PRIME handle to obj and a second non-PRIME handle to the same obj (for example via GEM_OPEN or MODE_GETFB2), changing the non-PRIME handle will insert a duplicate entry for obj->dma_buf into file_priv->prime.dmabufs while drm_prime_remove_buf_handle(&file_priv->prime, args->handle) will be a no-op. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
