Tvrtko Ursulin
2023-Feb-20 09:55 UTC
[Nouveau] [PATCH] drm/gem: Expose the buffer object handle to userspace last
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; >> ? } >
Christian König
2023-Feb-20 10:01 UTC
[Nouveau] [PATCH] drm/gem: Expose the buffer object handle to userspace last
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; >>> ? } >>
Tvrtko Ursulin
2023-Feb-20 10:23 UTC
[Nouveau] [PATCH] drm/gem: Expose the buffer object handle to userspace last
On 20/02/2023 10:01, Christian K?nig wrote:> 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.Hmm I don't immediately follow the connection. But I have only found radeon_driver_load_kms as using the load callback. Is there any lockdep enabled CI for that driver which could tell us if there is a problem there? Regards, Tvrtko> > 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; >>>> ? } >>> >
Apparently Analagous Threads
- [PATCH] drm/gem: Expose the buffer object handle to userspace last
- [PATCH] drm/gem: Expose the buffer object handle to userspace last
- [PATCH] drm/gem: Expose the buffer object handle to userspace last
- [PATCH] drm/gem: Expose the buffer object handle to userspace last
- [PATCH] drm/gem: Expose the buffer object handle to userspace last