From: "Christian König" <ckoenig.leichtzumerken@gmail.com>
To: "Tvrtko Ursulin" <tvrtko.ursulin@linux.intel.com>,
"Christian König" <christian.koenig@amd.com>,
dri-devel@lists.freedesktop.org
Cc: "Rob Clark" <robdclark@chromium.org>,
lima@lists.freedesktop.org,
"Tvrtko Ursulin" <tvrtko.ursulin@intel.com>,
nouveau@lists.freedesktop.org, amd-gfx@lists.freedesktop.org,
"Steven Price" <steven.price@arm.com>,
"Noralf Trønnes" <noralf@tronnes.org>,
"Ben Skeggs" <bskeggs@redhat.com>,
"Daniel Vetter" <daniel@ffwll.ch>,
"David Herrmann" <dh.herrmann@gmail.com>,
spice-devel@lists.freedesktop.org,
virtualization@lists.linux-foundation.org,
"Zack Rusin" <zackr@vmware.com>
Subject: Re: [Nouveau] [PATCH] drm/gem: Expose the buffer object handle to userspace last
Date: Mon, 20 Feb 2023 11:01:55 +0100 [thread overview]
Message-ID: <b7bb9888-b1d3-0d89-1e4f-bc25475b0071@gmail.com> (raw)
In-Reply-To: <767620c4-385f-c0e8-dcb1-513ef4402ad0@linux.intel.com>
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@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@lists.freedesktop.org
>>> Cc: Rob Clark <robdclark@chromium.org>
>>> Cc: Ben Skeggs <bskeggs@redhat.com>
>>> Cc: David Herrmann <dh.herrmann@gmail.com>
>>> Cc: Noralf Trønnes <noralf@tronnes.org>
>>> Cc: David Airlie <airlied@gmail.com>
>>> Cc: Daniel Vetter <daniel@ffwll.ch>
>>> Cc: amd-gfx@lists.freedesktop.org
>>> Cc: lima@lists.freedesktop.org
>>> Cc: nouveau@lists.freedesktop.org
>>> Cc: Steven Price <steven.price@arm.com>
>>> Cc: virtualization@lists.linux-foundation.org
>>> Cc: spice-devel@lists.freedesktop.org
>>> Cc: Zack Rusin <zackr@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;
>>> }
>>
next prev parent reply other threads:[~2023-02-20 10:02 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-02-14 12:50 [Nouveau] [PATCH] drm/gem: Expose the buffer object handle to userspace last Tvrtko Ursulin
2023-02-14 13:59 ` Christian König
2023-02-20 9:55 ` Tvrtko Ursulin
2023-02-20 10:01 ` Christian König [this message]
2023-02-20 10:23 ` Tvrtko Ursulin
2023-02-23 9:17 ` Christian König
2023-02-15 9:57 ` Steven Price
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=b7bb9888-b1d3-0d89-1e4f-bc25475b0071@gmail.com \
--to=ckoenig.leichtzumerken@gmail.com \
--cc=amd-gfx@lists.freedesktop.org \
--cc=bskeggs@redhat.com \
--cc=christian.koenig@amd.com \
--cc=daniel@ffwll.ch \
--cc=dh.herrmann@gmail.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=lima@lists.freedesktop.org \
--cc=noralf@tronnes.org \
--cc=nouveau@lists.freedesktop.org \
--cc=robdclark@chromium.org \
--cc=spice-devel@lists.freedesktop.org \
--cc=steven.price@arm.com \
--cc=tvrtko.ursulin@intel.com \
--cc=tvrtko.ursulin@linux.intel.com \
--cc=virtualization@lists.linux-foundation.org \
--cc=zackr@vmware.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).