Re: [PATCH v3 12/17] drm/panthor: Make the VM cleanup path more robust against UAF

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Self-deadlock and lockdep violation in panthor_mmu_pre_suspend when AS disable fails.
- [High] Unconditional removal of PANTHOR_AS_FORBID_UNMAP leads to Use-After-Free if soft reset fails.
--

--- Patch [12]: [PATCH v3 12/17] drm/panthor: Make the VM cleanup path more robust against UAF ---
commit 5812e6e60c1cdbc7bb20a9803d7bf5a58663d7d2
Author: Boris Brezillon <[email protected]>
Subject: drm/panthor: Make the VM cleanup path more robust against UAF

The commit defers the cleanup of VMs when a hardware hang prevents eviction
of a page table from its AS slot, waiting for the next post_reset. It
introduces a cleanup list for VMs and schedules a workqueue to process it
once the hardware is confirmed idle.

> diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
> index acdc0dd04f8be..5f4aab5813d52 100644
> --- a/drivers/gpu/drm/panthor/panthor_mmu.c
> +++ b/drivers/gpu/drm/panthor/panthor_mmu.c
[ ... ]
> @@ -1985,23 +2002,67 @@ static void panthor_mmu_irq_handler(struct panthor_device *ptdev, u32 status)
>   *
>   * We also suspend the MMU IRQ.
>   */
> -void panthor_mmu_suspend(struct panthor_device *ptdev)
> +void panthor_mmu_pre_suspend(struct panthor_device *ptdev)
>  {
>  	mutex_lock(&ptdev->mmu->as.slots_lock);
>  	for (u32 i = 0; i < ARRAY_SIZE(ptdev->mmu->as.slots); i++) {
>  		struct panthor_as *as = ptdev->mmu->as.slots[i].as;
> +		int ret;
>  
> -		if (as) {
> -			drm_WARN_ON(&ptdev->base,
> -				    panthor_mmu_as_disable(ptdev, i, false));
> +		if (!as)
> +			continue;
> +
> +		/* If the disable fails, leave the AS on its slot so we can
> +		 * properly evict it when we're sure the GPU is off.
> +		 */
> +		ret = panthor_mmu_as_disable(ptdev, i, false);
> +		if (!ret)
>  			panthor_as_release_hw_slot_locked(as);
> -		}
> +		else
> +			panthor_as_restrict_usage_locked(as,
> +							 PANTHOR_AS_FORBID_USE |
> +							 PANTHOR_AS_FORBID_MAP |
> +							 PANTHOR_AS_FORBID_UNMAP);
>  	}
>  	mutex_unlock(&ptdev->mmu->as.slots_lock);

[Severity: High]
Does this code cause a lockdep violation and self-deadlock if the hardware
hangs and panthor_mmu_as_disable() fails?

The panthor_mmu_pre_suspend() function acquires ptdev->mmu->as.slots_lock
before the loop. When calling panthor_as_restrict_usage_locked(), that
function asserts lockdep_assert_held(&as->op_lock) which doesn't appear
to be held here.

Furthermore, panthor_as_restrict_usage_locked() immediately tries to
acquire the slots_lock again:

static void panthor_as_restrict_usage_locked(...)
{
	...
	if (new_restrictions & PANTHOR_AS_FORBID_USE) {
		guard(mutex)(&ptdev->mmu->as.slots_lock);

Since standard mutexes are non-recursive, won't this self-deadlock the
thread performing the suspend?

>  
>  	panthor_mmu_irq_suspend(&ptdev->mmu->irq);
>  }
>  
> +static void mmu_post_reset_cleanup(struct panthor_device *ptdev)
> +{
> +	guard(mutex)(&ptdev->mmu->as.slots_lock);
> +
> +	/* Now that the reset is effective, we can assume that none of the
> +	 * AS slots are setup, and clear the faulty flags too.
> +	 */
> +	ptdev->mmu->as.alloc_mask = 0;
> +	ptdev->mmu->as.faulty_mask = 0;
> +
> +	for (u32 i = 0; i < ARRAY_SIZE(ptdev->mmu->as.slots); i++) {
> +		struct panthor_as *as = ptdev->mmu->as.slots[i].as;
> +
> +		if (!as)
> +			continue;
> +
> +		panthor_as_release_hw_slot_locked(as);
> +
> +		/* FIXME: We shouldn't drop the no-unmap restriction if
> +		 * we're in the unplug path and the device wasn't properly
> +		 * stopped with a SOFT_RESET.
> +		 */
> +		atomic_and(~PANTHOR_AS_FORBID_UNMAP, &as->restrictions);

[Severity: High]
Can this unconditional clearing lead to a use-after-free if a soft reset
fails to halt the GPU?

As noted in the FIXME comment, dropping this restriction might be dangerous.
If the device wasn't properly stopped, the hardware could still be actively
performing DMA. Dropping PANTHOR_AS_FORBID_UNMAP here will allow
vm_cleanup() to execute panthor_vm_unmap_range() and
panthor_heap_pool_destroy(), potentially freeing memory that the active
GPU can still access.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=12