[PATCH 2/2] drm/panthor: Fix UAF in panthor_gem_create_with_handle() debugfs code
Liviu Dudau
liviu.dudau at arm.com
Tue Jul 8 09:21:07 UTC 2025
Hi Simona,
On Mon, Jul 07, 2025 at 05:18:14PM +0200, 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.
Thanks for the cleanup, just minor nits.
>
> v2: Fix build issues for !CONFIG_DEBUGFS (Adrián)
>
> Fixes: a3707f53eb3f ("drm/panthor: show device-wide list of DRM GEM objects over DebugFS")
> Cc: Adrián Larumbe <adrian.larumbe at collabora.com>
> Cc: Boris Brezillon <boris.brezillon at collabora.com>
> Cc: Steven Price <steven.price at arm.com>
> Cc: Liviu Dudau <liviu.dudau at arm.com>
> Signed-off-by: Simona Vetter <simona.vetter at intel.com>
> Signed-off-by: Simona Vetter <simona.vetter at ffwll.ch>
> ---
> drivers/gpu/drm/panthor/panthor_gem.c | 34 ++++++++++++++-------------
> drivers/gpu/drm/panthor/panthor_gem.h | 3 ---
> 2 files changed, 18 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/panthor/panthor_gem.c b/drivers/gpu/drm/panthor/panthor_gem.c
> index 7c00fd77758b..8232f91f3bf7 100644
> --- a/drivers/gpu/drm/panthor/panthor_gem.c
> +++ b/drivers/gpu/drm/panthor/panthor_gem.c
> @@ -16,10 +16,14 @@
> #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_init(struct panthor_gem_object *bo)
> {
> INIT_LIST_HEAD(&bo->debugfs.node);
> +}
Would be nice to have an empty line here separating the functions.
> +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);
>
> bo->debugfs.creator.tgid = current->group_leader->pid;
> get_task_comm(bo->debugfs.creator.process_name, current->group_leader);
> @@ -44,14 +48,13 @@ 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) {}
> +static void panthor_gem_debugfs_bo_init(struct panthor_gem_object *bo) {}
> #endif
>
> static void panthor_gem_free_object(struct drm_gem_object *obj)
> @@ -246,7 +249,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);
> + panthor_gem_debugfs_bo_init(obj);
>
> return &obj->base.base;
> }
> @@ -285,6 +288,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.
> + */
This comment is no longer needed (or not in this form).
> + 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 +305,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 +390,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 +439,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
>
Otherwise, patch looks good to me!
Reviewed-by: Liviu Dudau <liviu.dudau at arm.com>
Best regards,
Liviu
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
¯\_(ツ)_/¯
More information about the Intel-xe
mailing list