From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 762EFC83F1D for ; Thu, 10 Jul 2025 09:23:31 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 3428210E36D; Thu, 10 Jul 2025 09:23:31 +0000 (UTC) Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by gabe.freedesktop.org (Postfix) with ESMTP id E497D10E36D; Thu, 10 Jul 2025 09:23:29 +0000 (UTC) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 4493A1EA6; Thu, 10 Jul 2025 02:23:18 -0700 (PDT) Received: from [10.1.34.27] (e122027.cambridge.arm.com [10.1.34.27]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 2DCA03F66E; Thu, 10 Jul 2025 02:23:28 -0700 (PDT) Message-ID: <08b8dd1b-5178-428d-a830-3af9b7fcea5a@arm.com> Date: Thu, 10 Jul 2025 10:23:26 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] drm/panthor: Fix UAF in panthor_gem_create_with_handle() debugfs code To: Simona Vetter Cc: DRI Development , Intel Xe Development , =?UTF-8?Q?Adri=C3=A1n_Larumbe?= , Boris Brezillon , Liviu Dudau , Simona Vetter References: <20250707151814.603897-2-simona.vetter@ffwll.ch> <20250709135220.1428931-1-simona.vetter@ffwll.ch> <6fe2409f-b561-4546-92e1-dd7f8d45ef12@arm.com> From: Steven Price Content-Language: en-GB In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-BeenThere: intel-xe@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel Xe graphics driver List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-xe-bounces@lists.freedesktop.org Sender: "Intel-xe" On 10/07/2025 09:53, Simona Vetter wrote: > On Wed, Jul 09, 2025 at 04:48:21PM +0100, Steven Price wrote: >> On 09/07/2025 14:52, 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. ^^^^^^^^^^ Another nit (although checkpatch takes the credit for noticing): it should have an 'a': initialized :p I've fixed this and pushed to drm-misc-next. Thanks, Steve >>> >>> 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. >>> >>> v2: Fix build issues for !CONFIG_DEBUGFS (Adrián) >>> >>> v3: Add linebreak and remove outdated comment (Liviu) >>> >>> Fixes: a3707f53eb3f ("drm/panthor: show device-wide list of DRM GEM objects over DebugFS") >>> Cc: Adrián Larumbe >>> Cc: Boris Brezillon >>> Cc: Steven Price >>> Cc: Liviu Dudau >>> Reviewed-by: Liviu Dudau >>> Signed-off-by: Simona Vetter >>> Signed-off-by: Simona Vetter >> >> Reviewed-by: Steven Price > > Thanks for the review! > >> Although a nit on the email subject - you're missing the "v3" tag ;) > > Yeah I often forget to set that, oops :-P > > Will you or someone from the panthor team land this, or should I push it > to drm-misc-next myself? > -Sima > >> >> Steve >> >>> --- >>> 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..a123bc740ba1 100644 >>> --- a/drivers/gpu/drm/panthor/panthor_gem.c >>> +++ b/drivers/gpu/drm/panthor/panthor_gem.c >>> @@ -16,10 +16,15 @@ >>> #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); >>> +} >>> + >>> +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 +49,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 +250,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 +289,8 @@ panthor_gem_create_with_handle(struct drm_file *file, >>> bo->base.base.resv = bo->exclusive_vm_root_gem->resv; >>> } >>> >>> + 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), >>> }; >>> >>> /** >> >