From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by smtp.subspace.kernel.org (Postfix) with ESMTP id CEC4535BDA6 for ; Wed, 22 Oct 2025 14:28:56 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=217.140.110.172 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1761143338; cv=none; b=IaghMBB7OGQVKbZR8LV4siQtRfE4tawaQekyLWRpnfSdESML1zlX09iL8poISOTUgPkwLJSosJAqWikG2RfDh497ZE1BTC71mq7Y0Ik1Swmkp5XP2l1YydOR/qgriXgMgJwosbFEweY0iGCngQkEn/335H5AyDA0inb1Eb7TQfw= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1761143338; c=relaxed/simple; bh=mJhr3NncY9clkh0gjeE3PCslsrYZ4XHyY8UYjCnbUkA=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=PGqMlQYZKd95qWBTKnuGb7HvgtDY0r6LEUaHNyEwBdz/zf6kxaOenTjwYae8RYYl0bsXarqsb10pTsCnUhB6Y33oJoVWjoM5TtYtQjviCyCM5M43Wk/qNbAt0ecDGkt9g4kLHC/QOCw7trU28idjShFZMS68h3hnJRESN7FKqvc= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com; spf=pass smtp.mailfrom=arm.com; arc=none smtp.client-ip=217.140.110.172 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=arm.com 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 EC4661063; Wed, 22 Oct 2025 07:28:47 -0700 (PDT) Received: from [10.57.33.187] (unknown [10.57.33.187]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 555443F59E; Wed, 22 Oct 2025 07:28:53 -0700 (PDT) Message-ID: <1cffaf6a-7e99-416f-af50-5659b1738af2@arm.com> Date: Wed, 22 Oct 2025 15:28:51 +0100 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2] drm/panthor: Fix UAF race between device unplug and FW event processing To: Boris Brezillon Cc: Ketil Johnsen , Liviu Dudau , Maarten Lankhorst , Maxime Ripard , Thomas Zimmermann , David Airlie , Simona Vetter , Heiko Stuebner , dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org References: <20251022103014.1082629-1-ketil.johnsen@arm.com> <20251022143751.769c1f23@fedora> <20251022160033.2f645528@fedora> From: Steven Price Content-Language: en-GB In-Reply-To: <20251022160033.2f645528@fedora> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit On 22/10/2025 15:00, Boris Brezillon wrote: > On Wed, 22 Oct 2025 14:36:23 +0100 > Steven Price wrote: > >> On 22/10/2025 13:37, Boris Brezillon wrote: >>> On Wed, 22 Oct 2025 12:30:13 +0200 >>> Ketil Johnsen wrote: >>> >>>> The function panthor_fw_unplug() will free the FW memory sections. >>>> The problem is that there could still be pending FW events which are yet >>>> not handled at this point. process_fw_events_work() can in this case try >>>> to access said freed memory. >>>> >>>> This fix introduces a destroyed state for the panthor_scheduler object, >>>> and we check for this before processing FW events. >>>> >>>> Signed-off-by: Ketil Johnsen >>>> Fixes: de85488138247 ("drm/panthor: Add the scheduler logical block") >>>> --- >>>> v2: >>>> - Followed Boris's advice and handle the race purely within the >>>> scheduler block (by adding a destroyed state) >>>> --- >>>> drivers/gpu/drm/panthor/panthor_sched.c | 15 ++++++++++++--- >>>> 1 file changed, 12 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c >>>> index 0cc9055f4ee52..4996f987b8183 100644 >>>> --- a/drivers/gpu/drm/panthor/panthor_sched.c >>>> +++ b/drivers/gpu/drm/panthor/panthor_sched.c >>>> @@ -315,6 +315,13 @@ struct panthor_scheduler { >>>> */ >>>> struct list_head stopped_groups; >>>> } reset; >>>> + >>>> + /** >>>> + * @destroyed: Scheduler object is (being) destroyed >>>> + * >>>> + * Normal scheduler operations should no longer take place. >>>> + */ >>>> + bool destroyed; >>> >>> Do we really need a new field for that? Can't we just reset >>> panthor_device::scheduler to NULL early enough in the unplug path? >>> I guess it's not that simple if we have works going back to ptdev >>> and then dereferencing ptdev->scheduler, but I think it's also >>> fundamentally broken to have scheduler works active after the >>> scheduler teardown has started, so we might want to add some more >>> checks in the work callbacks too. >>> >>>> }; >>>> >>>> /** >>>> @@ -1765,7 +1772,10 @@ static void process_fw_events_work(struct work_struct *work) >>>> u32 events = atomic_xchg(&sched->fw_events, 0); >>>> struct panthor_device *ptdev = sched->ptdev; >>>> >>>> - mutex_lock(&sched->lock); >>>> + guard(mutex)(&sched->lock); >>>> + >>>> + if (sched->destroyed) >>>> + return; >>>> >>>> if (events & JOB_INT_GLOBAL_IF) { >>>> sched_process_global_irq_locked(ptdev); >>>> @@ -1778,8 +1788,6 @@ static void process_fw_events_work(struct work_struct *work) >>>> sched_process_csg_irq_locked(ptdev, csg_id); >>>> events &= ~BIT(csg_id); >>>> } >>>> - >>>> - mutex_unlock(&sched->lock); >>>> } >>>> >>>> /** >>>> @@ -3882,6 +3890,7 @@ void panthor_sched_unplug(struct panthor_device *ptdev) >>>> cancel_delayed_work_sync(&sched->tick_work); >>>> >>>> mutex_lock(&sched->lock); >>>> + sched->destroyed = true; >>>> if (sched->pm.has_ref) { >>>> pm_runtime_put(ptdev->base.dev); >>>> sched->pm.has_ref = false; >>> >>> Hm, I'd really like to see a cancel_work_sync(&sched->fw_events_work) >>> rather than letting the work execute after we've started tearing down >>> the scheduler object. >>> >>> If you follow my suggestion to reset the ptdev->scheduler field, I >>> guess something like that would do: >>> >>> void panthor_sched_unplug(struct panthor_device *ptdev) >>> { >>> struct panthor_scheduler *sched = ptdev->scheduler; >>> >>> /* We want the schedu */ >>> WRITE_ONCE(*ptdev->scheduler, NULL); >>> >>> cancel_work_sync(&sched->fw_events_work); >>> cancel_delayed_work_sync(&sched->tick_work); >>> >>> mutex_lock(&sched->lock); >>> if (sched->pm.has_ref) { >>> pm_runtime_put(ptdev->base.dev); >>> sched->pm.has_ref = false; >>> } >>> mutex_unlock(&sched->lock); >>> } >>> >>> and >>> >>> void panthor_sched_report_fw_events(struct panthor_device *ptdev, u32 events) { >>> struct panthor_scheduler *sched = READ_ONCE(*ptdev->scheduler); >>> >>> /* Scheduler is not initialized, or it's gone. */ >>> if (!sched) >>> return; >>> >>> atomic_or(events, &sched->fw_events); >>> sched_queue_work(sched, fw_events); >>> } >> >> Note there's also the path of panthor_mmu_irq_handler() calling >> panthor_sched_report_mmu_fault() which will need to READ_ONCE() as well >> to be safe. > > This could be hidden behind a panthor_device_get_sched() helper, I > guess. Anyway, it's not so much that I'm against the addition of an > extra bool, but AFAICT, the problem is not entirely solved, as there > could be a pending work that gets executed after sched_unplug() > returns, and I adding this bool check just papers over the real bug > (which is that we never cancel the fw_event work). > >> >> I agree having an extra bool is ugly, but it easier to reason about than >> the lock-free WRITE_ONCE/READ_ONCE dance. It worries me that this will >> be regressed in the future. I can't immediately see how to wrap this in >> a helper to ensure this is kept correct. > > Sure, but you're not really catching cases where the work runs after > the scheduler component has been unplugged in case someone forgot to > cancel some works. I think I'd rather identify those cases with a > kernel panic, than a random UAF when the work is being executed. > Ultimately, we should probably audit all works used in the driver, to > make sure they are properly cancelled at unplug() time by the relevant > _unplug() functions. Yes I agree, we should have a cancel_work_sync(&sched->fw_events_work) call somewhere on the unplug path. That needs to be after the job irq has been disabled which is currently done in panthor_fw_unplug(). Thanks, Steve