drm: Set old handle to NULL before prime swap in change_handle

There was a potential race condition in change_handle. The ioctl
briefly had a single object with two idr entries; a concurrent
gem_close could delete the object and remove one of the handles
while leaving the other one dangling, which could subsequently
be dereferenced for a use-after-free.

To fix this, do the same dance that gem_close itself does.
(f6cd7daecff5 drm: Release driver references to handle before making it available again)
First idr_replace the old handle to NULL. Later, if the prime
operations are successful, actually close it.

create_tail required a similar dance to avoid a similar problem.
(bd46cece51a3 drm/gem: Fix race in drm_gem_handle_create_tail())
It idr_allocs the new handle with NULL, then swaps in the correct
object later to avoid races. We don't need to do that here, since
the only operations that could race are drm_prime, and
change_handle holds the prime lock for the entire duration.

v2: cleanups of error paths

Signed-off-by: David Francis <David.Francis@amd.com>
Co-authored-by: Dave Airlie <airlied@gmail.com>
Reported-by: Puttimet Thammasaeng <pwn8official@gmail.com>
Tested-by: Vitaly Prosyak <Vitaly.Prosyak@amd.com>
Cc: Simona Vetter <simona@ffwll.ch>
Cc: stable@vger.kernel.org
Cc: Christian Koenig <Christian.Koenig@amd.com>
Fixes: 53096728b8910 ("drm: Add DRM prime interface to reassign GEM handle")
Signed-off-by: Dave Airlie <airlied@redhat.com>

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index d6424267260bdb517a17318c373dab2a0aa7af62..51a887cc7fd744c0f6269d7f3d4dbe1fa713271d 100644 (file)
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -1019,7 +1019,7 @@ 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;
+       struct drm_gem_object *obj, *idrobj;
        int handle, ret;
 
        if (!drm_core_check_feature(dev, DRIVER_GEM))
@@ -1042,8 +1042,29 @@ int drm_gem_change_handle_ioctl(struct drm_device *dev, void *data,
        mutex_lock(&file_priv->prime.lock);
 
        spin_lock(&file_priv->table_lock);
+
+       /* When create_tail allocs an obj idr, it needs to first alloc as NULL,
+       * then later replace with the correct object. This is not necessary
+       * here, because the only operations that could race are drm_prime
+       * bookkeeping, and we hold the prime lock.
+       */
        ret = idr_alloc(&file_priv->object_idr, obj, handle, handle + 1,
                        GFP_NOWAIT);
+
+       if (ret < 0) {
+              spin_unlock(&file_priv->table_lock);
+              goto out_unlock;
+       }
+
+       idrobj = idr_replace(&file_priv->object_idr, NULL, handle);
+       if (idrobj != obj) {
+              idr_replace(&file_priv->object_idr, idrobj, handle);
+              idr_remove(&file_priv->object_idr, args->new_handle);
+              spin_unlock(&file_priv->table_lock);
+              ret = -ENOENT;
+              goto out_unlock;
+       }
+
        spin_unlock(&file_priv->table_lock);
 
        if (ret < 0)
@@ -1055,6 +1076,8 @@ int drm_gem_change_handle_ioctl(struct drm_device *dev, void *data,
                if (ret < 0) {
                        spin_lock(&file_priv->table_lock);
                        idr_remove(&file_priv->object_idr, handle);
+                       idrobj = idr_replace(&file_priv->object_idr, obj, handle);
+                       WARN_ON(idrobj != NULL);
                        spin_unlock(&file_priv->table_lock);
                        goto out_unlock;
                }