Re: [PATCH] drm/gem: Try to fix change_handle ioctl, attempt 4

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing IS_ERR check on idr_replace() return value causes an invalid pointer dereference.
--

commit f7209a31976a5023734386f397179e3835aa2048
Author: Simona Vetter <simona.vetter@ffwll.ch>

drm/gem: Try to fix change_handle ioctl, attempt 4

This commit modifies the change_handle ioctl to resolve a race condition
by merging the GEM object lookup and two-stage idr_replace. It also
temporarily disables the ioctl in drm_ioctls to prevent further abuse until
the implementation is fully verified.

> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index ac751a42db281..66cfd52c7cff9 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
[ ... ]
> @@ -1028,52 +1028,36 @@ int drm_gem_change_handle_ioctl(struct drm_device *dev, void *data,
[ ... ]
> +	obj = idr_replace(&file_priv->object_idr, NULL, args->handle);
> +	if (!obj) {

[Severity: High]
Will this result in dereferencing an error pointer if args->handle is invalid?

If idr_replace() fails to find the handle, it returns ERR_PTR(-ENOENT) rather
than NULL. The check here only tests for NULL, allowing an error pointer to
fall through.

> +		idr_remove(&file_priv->object_idr, new_handle);
> +		spin_unlock(&file_priv->table_lock);
> +		ret = -ENOENT;
> +		goto out_unlock;
> +	}
>  	spin_unlock(&file_priv->table_lock);
>  
>  	if (obj->dma_buf) {

If an error pointer falls through the check above, wouldn't evaluating
obj->dma_buf here cause an invalid pointer dereference? Should the error
handling for idr_replace() be updated to use IS_ERR(obj)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260604191916.1713387-1-simona.vetter@ffwll.ch?part=1