[Spice-devel] [PATCH] drm/gem: Expose the buffer object handle to userspace last

[Christian König]

Am 20.02.23 um 10:55 schrieb Tvrtko Ursulin:
>
> Hi,
>
> On 14/02/2023 13:59, Christian König wrote:
>> Am 14.02.23 um 13:50 schrieb Tvrtko Ursulin:
>>> From: Tvrtko Ursulin <tvrtko.ursulin at 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.
>>>
>>> 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.
>>>
>>> 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.
>>
>> Yes I've looked into this once as well, but couldn't completely solve 
>> it for some reason.
>>
>> Give me a day or two to get this tested and all the logic swapped 
>> back into my head again.
>
> Managed to recollect what the problem with earlier attempts was?

Nope, that's way to long ago. I can only assume that I ran into problems 
with the object_name_lock.

Probably best to double check if that doesn't result in a lock inversion 
when somebody grabs the reservation lock in their ->load() callback.

Regards,
Christian.

>
> Regards,
>
> Tvrtko
>
>> Christian.
>>
>>>
>>> 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-devel at lists.freedesktop.org
>>> Cc: Rob Clark <robdclark at chromium.org>
>>> Cc: Ben Skeggs <bskeggs at redhat.com>
>>> Cc: David Herrmann <dh.herrmann at gmail.com>
>>> Cc: Noralf Trønnes <noralf at tronnes.org>
>>> Cc: David Airlie <airlied at gmail.com>
>>> Cc: Daniel Vetter <daniel at ffwll.ch>
>>> Cc: amd-gfx at lists.freedesktop.org
>>> Cc: lima at lists.freedesktop.org
>>> Cc: nouveau at lists.freedesktop.org
>>> Cc: Steven Price <steven.price at arm.com>
>>> Cc: virtualization at lists.linux-foundation.org
>>> Cc: spice-devel at lists.freedesktop.org
>>> Cc: Zack Rusin <zackr at vmware.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;
>>>   }
>>