Re: [PATCH 10/12] 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] Use-After-Free of `mmu` structure during device teardown due to incorrect drmm action registration order.
- [Medium] Memory leak of stranded VMs in `cleanup_list` if the device suspends before a scheduled reset can execute.
--

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

The VM cleanup tries to gracefully evict the page table from its AS slot to
make sure the HW doesn't have access to the memory anymore. But it might
happen that the eviction fails because the HW hung, and in that case, we
have no guarantee that the HW won't access the memory until we've properly
reset the GPU. Defer the cleanup of VMs after the reset is effective when
this situation happens.

diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
--- a/drivers/gpu/drm/panthor/panthor_mmu.c
+++ b/drivers/gpu/drm/panthor/panthor_mmu.c

[ ... ]

@@ -2098,64 +2129,114 @@ void panthor_mmu_post_reset(struct panthor_device *ptdev)
> +	if (!drm_dev_enter(&ptdev->base, &cookie)) {
> +		guard(mutex)(&ptdev->mmu->as.slots_lock);
>  
> -		if (drm_dev_enter(&ptdev->base, &cookie)) {
> -			panthor_mmu_as_disable(ptdev, as->hw_slot.id, false);
> -			drm_dev_exit(cookie);
> +		/* We're in the unplug path and can't recover from
> +		 * that, so we just forcibly evict the pgtable. The
> +		 * no-unmap restriction will leak resources if
> +		 * we can't guarantee the HW stopped.
> +		 */
> +		if (as->hw_slot.id >= 0)
> +			panthor_as_release_hw_slot_locked(as);
>  
> -		panthor_as_release_hw_slot_locked(as);
> +		return true;
> +	}
>  
> -	}
> -	mutex_unlock(&ptdev->mmu->as.slots_lock);
> -	mutex_unlock(&vm->as->op_lock);
>  
> -	if (vm->dummy)
> -		drm_gem_object_put(&vm->dummy->base);
> +	scoped_guard(mutex, &ptdev->mmu->as.slots_lock) {
> +		if (as->hw_slot.id >= 0) {
> +			ret = panthor_mmu_as_disable(ptdev, as->hw_slot.id, false);
> +			if (!ret) {
> +				panthor_as_release_hw_slot_locked(as);
> +			} else {
> +				list_add_tail(&vm->node, &ptdev->mmu->as.cleanup_list);
> +				panthor_device_schedule_reset(ptdev);

[Severity: Medium]
Could this leak stranded VMs if the device suspends before the scheduled
reset executes?

If a hardware hang prevents AS slot eviction here, the VM is added to the
cleanup_list and a reset is scheduled. If the device suspends before the
reset work executes, panthor_device_suspend() synchronously cancels the
reset work. During resume, panthor_mmu_resume() is called but does not
invoke mmu_post_reset_cleanup() to process this cleanup_list, leaving the
VMs stranded.

> +			}
>  		}
>  
> -		panthor_as_release_hw_slot_locked(as);
> +		/* Page table is no longer resident, we can relax the no-unmap
> +		 * restriction.
> +		 */
> +		ready_for_cleanup = as->hw_slot.id < 0;
> +		if (ready_for_cleanup)
> +			atomic_and(~PANTHOR_AS_FORBID_UNMAP, &as->restrictions);
>  	}

[ ... ]

@@ -3488,6 +3573,27 @@ static void panthor_mmu_info_init(struct panthor_device *ptdev)
>  	ptdev->mmu_info.page_size_bitmap = SZ_4K | SZ_2M;
>  }
>  
> +static void mmu_cleanup_vms_work(struct work_struct *work)
> +{
> +	struct panthor_mmu *mmu =
> +		container_of(work, struct panthor_mmu, vm.cleanup_work);
> +	struct panthor_vm *vm, *tmp;
> +	LIST_HEAD(cleanup_list);
> +
> +	/* Collect the VMs to cleanup first. */
> +	scoped_guard(mutex, &mmu->as.slots_lock) {
> +		list_for_each_entry_safe(vm, tmp, &mmu->as.cleanup_list, node) {

[Severity: High]
Can this result in a use-after-free during device teardown?

In panthor_device_init(), the panthor_device_flush_cleanup_wq action is
registered via drmm_add_action() before panthor_mmu_init() is called. Then,
in panthor_mmu_init(), ptdev->mmu is allocated via drmm_kzalloc().

Because drmm actions execute in LIFO order, ptdev->mmu will be freed before
the workqueue is flushed. If this delayed work is pending or running during
teardown, it will access the already-freed mmu structure here when trying to
acquire the as.slots_lock.

> +			if (vm->as->hw_slot.id < 0)
> +				list_move_tail(&vm->node, &cleanup_list);
> +		}
> +	}
> +
> +	list_for_each_entry_safe(vm, tmp, &cleanup_list, node) {
> +		list_del(&vm->node);
> +		vm_cleanup(vm);
> +	}
> +}
> +

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