On Sun, Jun 01, 2025 at 03:06:15PM +0100, Adrián Larumbe wrote:
> Hi Simona,
>
> On 28.05.2025 11:13, Simona Vetter wrote:
> > The object is potentially already gone after the drm_gem_object_put().
> > In general the object should be fully constructed before calling
> > drm_gem_handle_create(), except the debugfs tracking uses a separate
> > lock and list and separate flag to denotate whether the object is
> > actually initilized.
> >
> > Since I'm touching this all anyway simplify this by only adding the
> > object to the debugfs when it's ready for that, which allows us to
> > delete that separate flag. panthor_gem_debugfs_bo_rm() already checks
> > whether we've actually been added to the list or this is some error
> > path cleanup.
> >
> > Fixes: a3707f53eb3f ("drm/panthor: show device-wide list of DRM GEM objects
> > over DebugFS")
> > Cc: Adrián Larumbe <adrian.laru...@collabora.com>
> > Cc: Boris Brezillon <boris.brezil...@collabora.com>
> > Cc: Steven Price <steven.pr...@arm.com>
> > Cc: Liviu Dudau <liviu.du...@arm.com>
> > Signed-off-by: Simona Vetter <simona.vet...@intel.com>
> > Signed-off-by: Simona Vetter <simona.vet...@ffwll.ch>
> > ---
> > drivers/gpu/drm/panthor/panthor_gem.c | 31 +++++++++++++--------------
> > drivers/gpu/drm/panthor/panthor_gem.h | 3 ---
> > 2 files changed, 15 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/panthor/panthor_gem.c
> > b/drivers/gpu/drm/panthor/panthor_gem.c
> > index 7c00fd77758b..f334444cb5df 100644
> > --- a/drivers/gpu/drm/panthor/panthor_gem.c
> > +++ b/drivers/gpu/drm/panthor/panthor_gem.c
> > @@ -16,9 +16,11 @@
> > #include "panthor_mmu.h"
> >
> > #ifdef CONFIG_DEBUG_FS
> > -static void panthor_gem_debugfs_bo_add(struct panthor_device *ptdev,
> > - struct panthor_gem_object *bo)
> > +static void panthor_gem_debugfs_bo_add(struct panthor_gem_object *bo)
> > {
> > + struct panthor_device *ptdev = container_of(bo->base.base.dev,
> > + struct panthor_device,
> > base);
> > +
> > INIT_LIST_HEAD(&bo->debugfs.node);
> >
> > bo->debugfs.creator.tgid = current->group_leader->pid;
> > @@ -44,12 +46,10 @@ static void panthor_gem_debugfs_bo_rm(struct
> > panthor_gem_object *bo)
> >
> > static void panthor_gem_debugfs_set_usage_flags(struct panthor_gem_object
> > *bo, u32 usage_flags)
> > {
> > - bo->debugfs.flags = usage_flags |
> > PANTHOR_DEBUGFS_GEM_USAGE_FLAG_INITIALIZED;
> > + bo->debugfs.flags = usage_flags;
> > + panthor_gem_debugfs_bo_add(bo);
> > }
> > #else
> > -static void panthor_gem_debugfs_bo_add(struct panthor_device *ptdev,
> > - struct panthor_gem_object *bo)
> > -{}
> > static void panthor_gem_debugfs_bo_rm(struct panthor_gem_object *bo) {}
> > static void panthor_gem_debugfs_set_usage_flags(struct panthor_gem_object
> > *bo, u32 usage_flags) {}
> > #endif
> > @@ -246,7 +246,7 @@ struct drm_gem_object *panthor_gem_create_object(struct
> > drm_device *ddev, size_t
> > drm_gem_gpuva_set_lock(&obj->base.base, &obj->gpuva_list_lock);
> > mutex_init(&obj->label.lock);
> >
> > - panthor_gem_debugfs_bo_add(ptdev, obj);
> > + INIT_LIST_HEAD(&obj->debugfs.node);
>
> This is going to break builds with no DebugFS support.
Oops, forgot to build-test this. Note that runtime testing would be good,
I don't have the hw for that. Or can some CI pick this up somewhere?
> > return &obj->base.base;
> > }
> > @@ -285,6 +285,12 @@ panthor_gem_create_with_handle(struct drm_file *file,
> > bo->base.base.resv = bo->exclusive_vm_root_gem->resv;
> > }
> >
> > + /*
> > + * No explicit flags are needed in the call below, since the
> > + * function internally sets the INITIALIZED bit for us.
> > + */
>
> If we got rid of the INITIALIZED usage flag, then this comment should also be
> reworded.
Will also fix this for v2.
>
> > + panthor_gem_debugfs_set_usage_flags(bo, 0);
> > +
> > /*
> > * Allocate an id of idr table where the obj is registered
> > * and handle has the id what user can see.
> > @@ -296,12 +302,6 @@ panthor_gem_create_with_handle(struct drm_file *file,
> > /* drop reference from allocate - handle holds it now. */
> > drm_gem_object_put(&shmem->base);
> >
> > - /*
> > - * No explicit flags are needed in the call below, since the
> > - * function internally sets the INITIALIZED bit for us.
> > - */
> > - panthor_gem_debugfs_set_usage_flags(bo, 0);
> > -
> > return ret;
> > }
> >
> > @@ -387,7 +387,7 @@ static void panthor_gem_debugfs_bo_print(struct
> > panthor_gem_object *bo,
> > unsigned int refcount = kref_read(&bo->base.base.refcount);
> > char creator_info[32] = {};
> > size_t resident_size;
> > - u32 gem_usage_flags = bo->debugfs.flags &
> > (u32)~PANTHOR_DEBUGFS_GEM_USAGE_FLAG_INITIALIZED;
> > + u32 gem_usage_flags = bo->debugfs.flags;
> > u32 gem_state_flags = 0;
> >
> > /* Skip BOs being destroyed. */
> > @@ -436,8 +436,7 @@ void panthor_gem_debugfs_print_bos(struct
> > panthor_device *ptdev,
> >
> > scoped_guard(mutex, &ptdev->gems.lock) {
> > list_for_each_entry(bo, &ptdev->gems.node, debugfs.node) {
> > - if (bo->debugfs.flags &
> > PANTHOR_DEBUGFS_GEM_USAGE_FLAG_INITIALIZED)
> > - panthor_gem_debugfs_bo_print(bo, m, &totals);
> > + panthor_gem_debugfs_bo_print(bo, m, &totals);
> > }
> > }
> >
> > diff --git a/drivers/gpu/drm/panthor/panthor_gem.h
> > b/drivers/gpu/drm/panthor/panthor_gem.h
> > index 4dd732dcd59f..8fc7215e9b90 100644
> > --- a/drivers/gpu/drm/panthor/panthor_gem.h
> > +++ b/drivers/gpu/drm/panthor/panthor_gem.h
> > @@ -35,9 +35,6 @@ enum panthor_debugfs_gem_usage_flags {
> >
> > /** @PANTHOR_DEBUGFS_GEM_USAGE_FLAG_FW_MAPPED: BO is mapped on the FW
> > VM. */
> > PANTHOR_DEBUGFS_GEM_USAGE_FLAG_FW_MAPPED =
> > BIT(PANTHOR_DEBUGFS_GEM_USAGE_FW_MAPPED_BIT),
> > -
> > - /** @PANTHOR_DEBUGFS_GEM_USAGE_FLAG_INITIALIZED: BO is ready for
> > DebugFS display. */
> > - PANTHOR_DEBUGFS_GEM_USAGE_FLAG_INITIALIZED = BIT(31),
> > };
> >
> > /**
> > --
> > 2.49.0
>
> There's a Panfrost port of the functionality this patch fixes pending merge
> into drm-misc,
> so I should probably ask either Boris or Steven to hold off on merging them
> till I've made
> sure there's no potential UAF in it.
The important part is to do all init before the call to gem_object_put(),
that prevents the UAF. Doing all init before handle_create() is just nice
on top, since that aligns with the core design and avoids the need for a
separate init flag (for which you're at least missing the right memory
barriers here).
Thanks for your comments.
-Sima
--
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
