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

[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?

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;
>>   }
>