On 14/02/2023 12:50, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursu...@intel.com>
> 
> Currently drm_gem_handle_create_tail exposes the handle to userspace
> before the buffer object constructions is complete. This allowing
> of working against a partially constructed object, which may also be in
> the process of having its creation fail, can have a range of negative
> outcomes.
> 
> A lot of those will depend on what the individual drivers are doing in
> their obj->funcs->open() callbacks, and also with a common failure mode
> being -ENOMEM from drm_vma_node_allow.
> 
> We can make sure none of this can happen by allocating a handle last,
> although with a downside that more of the function now runs under the
> dev->object_name_lock.
> 
> Looking into the individual drivers open() hooks, we have
> amdgpu_gem_object_open which seems like it could have a potential security
> issue without this change.
> 
> A couple drivers like qxl_gem_object_open and vmw_gem_object_open
> implement no-op hooks so no impact for them.
> 
> A bunch of other require a deeper look by individual owners to asses for
> impact. Those are lima_gem_object_open, nouveau_gem_object_open,
> panfrost_gem_open, radeon_gem_object_open and virtio_gpu_gem_object_open.

I've looked over the panfrost code, and I can't see how this could
create a security hole there. It looks like there's a path which can
confuse the shrinker (so objects might not be purged when they could
be[1]) but they would be freed properly in the normal path - so no worse
than user space could already do.

[1] gpu_usecount is incremented in panfrost_lookup_bos() per bo, but not
decremented on failure.

> Putting aside the risk assesment of the above, some common scenarios to
> think about are along these lines:
> 
> 1)
> Userspace closes a handle by speculatively "guessing" it from a second
> thread.
> 
> This results in an unreachable buffer object so, a memory leak.
> 
> 2)
> Same as 1), but object is in the process of getting closed (failed
> creation).
> 
> The second thread is then able to re-cycle the handle and idr_remove would
> in the first thread would then remove the handle it does not own from the
> idr.

This, however, looks plausible - and I can see how this could
potentially trigger a security hole in user space.

> 3)
> Going back to the earlier per driver problem space - individual impact
> assesment of allowing a second thread to access and operate on a partially
> constructed handle / object. (Can something crash? Leak information?)
> 
> In terms of identifying when the problem started I will tag some patches
> as references, but not all, if even any, of them actually point to a
> broken state. I am just identifying points at which more opportunity for
> issues to arise was added.
> 
> References: 304eda32920b ("drm/gem: add hooks to notify driver when object 
> handle is created/destroyed")
> References: ca481c9b2a3a ("drm/gem: implement vma access management")
> References: b39b5394fabc ("drm/gem: Add drm_gem_object_funcs")
> Cc: dri-de...@lists.freedesktop.org
> Cc: Rob Clark <robdcl...@chromium.org>
> Cc: Ben Skeggs <bske...@redhat.com>
> Cc: David Herrmann <dh.herrm...@gmail.com>
> Cc: Noralf Trønnes <nor...@tronnes.org>
> Cc: David Airlie <airl...@gmail.com>
> Cc: Daniel Vetter <dan...@ffwll.ch>
> Cc: amd-...@lists.freedesktop.org
> Cc: l...@lists.freedesktop.org
> Cc: nouveau@lists.freedesktop.org
> Cc: Steven Price <steven.pr...@arm.com>
> Cc: virtualizat...@lists.linux-foundation.org
> Cc: spice-de...@lists.freedesktop.org
> Cc: Zack Rusin <za...@vmware.com>

FWIW I think this makes the code easier to reason about, so

Reviewed-by: Steven Price <steven.pr...@arm.com>

> ---
>  drivers/gpu/drm/drm_gem.c | 48 +++++++++++++++++++--------------------
>  1 file changed, 24 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index aa15c52ae182..e3d897bca0f2 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -356,52 +356,52 @@ drm_gem_handle_create_tail(struct drm_file *file_priv,
>                          u32 *handlep)
>  {
>       struct drm_device *dev = obj->dev;
> -     u32 handle;
>       int ret;
>  
>       WARN_ON(!mutex_is_locked(&dev->object_name_lock));
>       if (obj->handle_count++ == 0)
>               drm_gem_object_get(obj);
>  
> +     ret = drm_vma_node_allow(&obj->vma_node, file_priv);
> +     if (ret)
> +             goto err_put;
> +
> +     if (obj->funcs->open) {
> +             ret = obj->funcs->open(obj, file_priv);
> +             if (ret)
> +                     goto err_revoke;
> +     }
> +
>       /*
> -      * Get the user-visible handle using idr.  Preload and perform
> -      * allocation under our spinlock.
> +      * Get the user-visible handle using idr as the _last_ step.
> +      * Preload and perform allocation under our spinlock.
>        */
>       idr_preload(GFP_KERNEL);
>       spin_lock(&file_priv->table_lock);
> -
>       ret = idr_alloc(&file_priv->object_idr, obj, 1, 0, GFP_NOWAIT);
> -
>       spin_unlock(&file_priv->table_lock);
>       idr_preload_end();
>  
> -     mutex_unlock(&dev->object_name_lock);
>       if (ret < 0)
> -             goto err_unref;
> -
> -     handle = ret;
> +             goto err_close;
>  
> -     ret = drm_vma_node_allow(&obj->vma_node, file_priv);
> -     if (ret)
> -             goto err_remove;
> +     mutex_unlock(&dev->object_name_lock);
>  
> -     if (obj->funcs->open) {
> -             ret = obj->funcs->open(obj, file_priv);
> -             if (ret)
> -                     goto err_revoke;
> -     }
> +     *handlep = ret;
>  
> -     *handlep = handle;
>       return 0;
>  
> +err_close:
> +     if (obj->funcs->close)
> +             obj->funcs->close(obj, file_priv);
>  err_revoke:
>       drm_vma_node_revoke(&obj->vma_node, file_priv);
> -err_remove:
> -     spin_lock(&file_priv->table_lock);
> -     idr_remove(&file_priv->object_idr, handle);
> -     spin_unlock(&file_priv->table_lock);
> -err_unref:
> -     drm_gem_object_handle_put_unlocked(obj);
> +err_put:
> +     if (--obj->handle_count == 0)
> +             drm_gem_object_put(obj);
> +
> +     mutex_unlock(&dev->object_name_lock);
> +
>       return ret;
>  }
>  

Reply via email to