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

[sashiko-bot@kernel.org]

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 <David.Francis@amd.com>

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/20261103155411.3305376-1-David.Francis@amd.com?part=1