Re: [PATCH v4] drm/gem: Fix and enable CHANGE_HANDLE

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] drm/gem: broken PRIME handling in
  drm_gem_change_handle_ioctl()
- [Medium] drm/gem: stale i915 VMA LUT cache in
  drm_gem_change_handle_ioctl()
- [Low] drm/gem: idr_alloc() -ENOMEM masked in
  drm_gem_change_handle_ioctl()
--

commit 7986c0e82ecacec73e2cd689a660b9387bc87689
Author: David Francis <David.Francis@amd.com>

drm/gem: Fix and enable CHANGE_HANDLE

This commit updates drm_gem_change_handle_ioctl() to reject invalid or
unchanged handles, preload the IDR before allocation, return -EBUSY when
new_handle is already in use, and re-enables DRM_IOCTL_GEM_CHANGE_HANDLE.

> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 3ce38df7b8b85..66f69cf41aaf0 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -1017,44 +1017,33 @@ drm_gem_open_ioctl(struct drm_device *dev, void *data,
>  	return ret;
>  }
>  
> -/*
> - * This ioctl is disabled for security reasons but also it failed
> - * to follow process in terms of adding testing in igt and verifying
> - * all the corner cases which made fixing security bugs in it even
> - * harder than necessary.
> - *
> - * To re-enable this ioctl
> - * 1. land working IGT tests in igt-gpu-tools that cover
> - *    all corner cases and race conditions.
> - * 2. handle idr_preload
> - * 3. handle == 0
> - * 4. handle == new_handle semantics definition.
> - */
>  int drm_gem_change_handle_ioctl(struct drm_device *dev, void *data,
>  				struct drm_file *file_priv)
>  {
>  	struct drm_gem_change_handle *args = data;
>  	struct drm_gem_object *obj;
> -	int new_handle, ret;
> +	int ret;
> +	u32 new_handle;
>  
>  	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;
> +	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);
>  
>  	if (ret < 0) {
>  		spin_unlock(&file_priv->table_lock);
> +		idr_preload_end();
> +		ret = -EBUSY;
>  		goto out_unlock;
>  	}

[Severity: Low]
Does this mask -ENOMEM from idr_alloc()?  If idr_preload() fails to
allocate a radix tree node under memory pressure, idr_alloc() returns
-ENOMEM rather than -ENOSPC, so only -ENOSPC should be translated to
-EBUSY.

>  	obj = idr_replace(&file_priv->object_idr, NULL, args->handle);
>  	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) {

[Severity: Medium]
Can checking obj->dma_buf here race with concurrent PRIME export and
corrupt file_priv->prime?

obj->dma_buf is a device-wide field protected by dev->object_name_lock,
whereas drm_gem_change_handle_ioctl() only holds the per-file
file_priv->prime.lock:

CPU0                                  CPU1
drm_gem_change_handle_ioctl()         drm_gem_prime_handle_to_fd()
  mutex_lock(&file_priv1->prime.lock)   mutex_lock(&file_priv2->prime.lock)
                                        mutex_lock(&dev->object_name_lock)
                                        export_and_register_object()
  if (obj->dma_buf)                       obj->dma_buf = dmabuf;
    <-- data race

Keying the file_priv->prime update on obj->dma_buf rather than checking
whether args->handle is present in file_priv->prime via
drm_prime_lookup_buf_by_handle() also mismanages the per-file PRIME cache:

1. If obj->dma_buf was set by exporting on another drm_file (or via
   another handle H1 on the same file_priv), changing a non-PRIME handle
   H2 (e.g. from DRM_IOCTL_GEM_OPEN or DRM_IOCTL_MODE_GETFB2) to H3 calls
   drm_prime_add_buf_handle() for H3 while drm_prime_remove_buf_handle()
   for H2 is a no-op.  When H1 is already in file_priv->prime, this inserts
   a duplicate dma_buf entry into file_priv->prime.dmabufs.

2. Conversely, when a PRIME-imported object (obj->import_attach != NULL)
   has obj->dma_buf == NULL (for example, if a handle is re-obtained via
   DRM_IOCTL_MODE_GETFB2 after handle_count previously dropped to zero)
   and is exported via drm_gem_prime_handle_to_dmabuf(), the
   (obj->import_attach->dmabuf, args->handle) entry is added to
   file_priv->prime without setting obj->dma_buf:

drivers/gpu/drm/drm_prime.c:drm_gem_prime_handle_to_dmabuf() {
    ...
	if (obj->import_attach) {
		dmabuf = obj->import_attach->dmabuf;
		get_dma_buf(dmabuf);
		goto out_have_obj;
	}
    ...
out_have_obj:
    ...
	ret = drm_prime_add_buf_handle(&file_priv->prime,
				       dmabuf, handle);
}

   Changing args->handle then skips the if (obj->dma_buf) block and leaves
   args->handle orphaned in file_priv->prime, triggering the WARN_ON() in
   drm_prime_destroy_file_private() and leaking the drm_prime_member and
   its dma_buf reference when the file is closed.

> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index 9039a39c43243..bb350895625ce 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -672,8 +672,7 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
>  	DRM_IOCTL_DEF(DRM_IOCTL_GEM_CLOSE, drm_gem_close_ioctl, DRM_RENDER_ALLOW),
>  	DRM_IOCTL_DEF(DRM_IOCTL_GEM_FLINK, drm_gem_flink_ioctl, DRM_AUTH),
>  	DRM_IOCTL_DEF(DRM_IOCTL_GEM_OPEN, drm_gem_open_ioctl, DRM_AUTH),
> -	/* see drm_gem.c:drm_gem_change_handle_ioctl for why this is invalid */
> -	DRM_IOCTL_DEF(DRM_IOCTL_GEM_CHANGE_HANDLE, drm_invalid_op, DRM_RENDER_ALLOW),
> +	DRM_IOCTL_DEF(DRM_IOCTL_GEM_CHANGE_HANDLE, drm_gem_change_handle_ioctl, DRM_RENDER_ALLOW),

[Severity: Medium]
Does re-enabling DRM_IOCTL_GEM_CHANGE_HANDLE leave stale entries in i915's
per-context VMA lookup cache (eb->gem_context->handles_vma)?

In i915, eb_lookup_vma() caches (handle -> vma) in
eb->gem_context->handles_vma and checks that radix tree before consulting
eb->file->object_idr:

drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:eb_lookup_vma() {
    ...
	if (likely(!eb->lut_size)) {
		rcu_read_lock();
		vma = radix_tree_lookup(&eb->gem_context->handles_vma, handle);
		if (likely(vma))
			vma = i915_vma_tryget(vma);
		rcu_read_unlock();
		if (likely(vma))
			return vma;
	}
    ...
}

Entries are only removed from ctx->handles_vma when the context is closed
or when obj->funcs->close (i915_gem_close_object()) runs.  Because
drm_gem_change_handle_ioctl() moves the handle in file_priv->object_idr
without invoking obj->funcs->close or notifying the driver, subsequent
DRM_IOCTL_I915_GEM_EXECBUFFER2 calls using the old handle (even after a
newly created object is assigned that handle ID) will resolve to the old
object's VMA, and repeated handle changes accumulate i915_lut_handle
entries on obj->lut_list.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113150226.1123441-1-David.Francis@amd.com?part=1