Re: [PATCH 7/7] drm/panfrost: Name the BOs around an unhandled page fault

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] drm/panfrost: UAF on zero-refcount mappings in
  panfrost_mmu_report_fault_bos()
- [Low] drm/panfrost: unmapped BOs are removed from mmu->mm before fault
  lookup
--

commit ead4ad306bba42948b358f03bb31d38d71c27615
Author: Arun Iyengar <aiyengar1990@gmail.com>

drm/panfrost: Name the BOs around an unhandled page fault

This commit logs the buffer object containing an unhandled GPU page fault
address, or the nearest buffer objects below and above it in the faulting
address space.

> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> index 13728b0c46efc..b0319d680b12c 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> @@ -595,6 +595,86 @@ addr_to_mapping(struct panfrost_device *pfdev, int as, u64 addr)
>  	return mapping;
>  }
>  
> +static void panfrost_mmu_print_fault_bo(struct panfrost_device *pfdev,
> +					struct panfrost_gem_mapping *m,
> +					u64 addr, const char *where)
> +{
> +	struct panfrost_gem_object *bo = m->obj;
> +	u64 start = m->mmnode.start << PAGE_SHIFT;
> +	u64 end = (m->mmnode.start + m->mmnode.size) << PAGE_SHIFT;
> +	u64 dist = addr < start ? start - addr : addr >= end ? addr - end : addr - start;
> +
> +	mutex_lock(&bo->label.lock);
> +	dev_err(pfdev->base.dev,
> +		"fault bo: %s: VA [0x%llx-0x%llx) size 0x%zx, fault at %s0x%llx, %s%s%s, label \"%s\"\n",
> +		where, start, end, bo->base.base.size,
> +		addr < start ? "-" : addr >= end ? "end+" : "start+", dist,
> +		m->active ? "mapped" : "NOT mapped",
> +		bo->is_heap ? ", heap" : "", bo->noexec ? ", noexec" : "",
> +		bo->label.str ? bo->label.str : "");
> +	mutex_unlock(&bo->label.lock);
> +}
> +
> +/*
> + * Report the BOs around an unhandled fault address in the faulting address
> + * space: the one containing it (if any), else the nearest one below and above.
> + * An access just past the end of a BO that a hole follows is a GPU overrun of
> + * that BO (userspace or hardware), a fault inside a BO whose mapping is gone
> + * points at the kernel, an address far from any BO at a stale or garbage
> + * pointer.
> + */

[Severity: Low]
Can a BO whose mapping has been torn down ever be reported as "inside" and
"NOT mapped" here?

panfrost_mmu_unmap() is only called from panfrost_gem_teardown_mapping(),
which immediately acquires mapping->mmu->mm_lock and removes
mapping->mmnode from mmu->mm.  Once a mapping is torn down (whether on
handle close or shrinker purge), its node is no longer in mmu->mm, so a
fault at its old VA will be reported between the neighboring "below" and
"above" BOs rather than inside an unmapped BO.  Conversely, for nodes still
in mmu->mm (outside transient setup or teardown windows), m->active is only
false for a heap BO that has not faulted in its first chunk yet, while a
partially mapped heap BO has m->active set to true even when faulting in an
unmapped chunk.

> +static void panfrost_mmu_report_fault_bos(struct panfrost_device *pfdev,
> +					  int as, u64 addr)
> +{
> +	struct panfrost_gem_mapping *below = NULL, *above = NULL;
> +	u64 offset = addr >> PAGE_SHIFT;
> +	struct drm_mm_node *node;
> +	struct panfrost_mmu *mmu;
> +	bool inside = false;
> +
> +	spin_lock(&pfdev->as_lock);
> +	list_for_each_entry(mmu, &pfdev->as_lru_list, list) {
> +		if (as == mmu->as)
> +			goto found_mmu;
> +	}
> +	spin_unlock(&pfdev->as_lock);
> +	dev_err(pfdev->base.dev, "fault bo: no context holds AS%d\n", as);
> +	return;
> +
> +found_mmu:
> +	spin_lock(&mmu->mm_lock);
> +	/* The node list is sorted by address. */
> +	drm_mm_for_each_node(node, &mmu->mm) {
> +		if (node->start > offset) {
> +			above = drm_mm_node_to_panfrost_mapping(node);
> +			break;
> +		}
> +		below = drm_mm_node_to_panfrost_mapping(node);
> +	}
> +	if (below) {
> +		inside = offset < below->mmnode.start + below->mmnode.size;
> +		kref_get(&below->refcount);
> +		if (inside)
> +			above = NULL;
> +	}
> +	if (above)
> +		kref_get(&above->refcount);
> +	spin_unlock(&mmu->mm_lock);
> +	spin_unlock(&pfdev->as_lock);
> +
> +	if (!below && !above)
> +		dev_err(pfdev->base.dev, "fault bo: AS%d has no BO\n", as);
> +	if (below) {
> +		panfrost_mmu_print_fault_bo(pfdev, below, addr,
> +					    inside ? "inside" : "below");
> +		panfrost_gem_mapping_put(below);
> +	}
> +	if (above) {
> +		panfrost_mmu_print_fault_bo(pfdev, above, addr, "above");
> +		panfrost_gem_mapping_put(above);
> +	}
> +}

[Severity: High]
Can kref_get() in panfrost_mmu_report_fault_bos() race with
panfrost_gem_mapping_release() and cause a refcount addition-on-zero and
use-after-free on below or above?

When the last reference to a mapping is dropped in
panfrost_gem_mapping_put(), mapping->refcount reaches zero before
panfrost_gem_teardown_mapping() unmaps the page tables and acquires
mapping->mmu->mm_lock to remove mapping->mmnode from mmu->mm:

drivers/gpu/drm/panfrost/panfrost_gem.c:panfrost_gem_teardown_mapping() {
	if (mapping->active)
		panfrost_mmu_unmap(mapping);

	spin_lock(&mapping->mmu->mm_lock);
	if (drm_mm_node_allocated(&mapping->mmnode))
		drm_mm_remove_node(&mapping->mmnode);
	spin_unlock(&mapping->mmu->mm_lock);
}

If a GPU job accesses that BO while its GEM handle is being closed,
panfrost_mmu_unmap() triggers the unhandled GPU page fault while
mapping->mmnode is still in mmu->mm with refcount == 0:

CPU0                                  CPU1
panfrost_gem_close()
  panfrost_gem_mapping_put()
    kref_put(&mapping->refcount, ...)
      panfrost_gem_mapping_release()
        panfrost_gem_teardown_mapping()
          panfrost_mmu_unmap(mapping)
                                      panfrost_mmu_irq_handler_thread()
                                        panfrost_mmu_report_fault_bos()
                                          spin_lock(&mmu->mm_lock)
                                          kref_get(&below->refcount)
                                            <-- addition on zero
                                          spin_unlock(&mmu->mm_lock)
          spin_lock(&mmu->mm_lock)
          drm_mm_remove_node(...)
          spin_unlock(&mmu->mm_lock)
        drm_gem_object_put(...)
        kfree(mapping)
                                        panfrost_mmu_print_fault_bo()
                                          mutex_lock(&bo->label.lock)
                                            <-- use-after-free

Using kref_get_unless_zero() (or skipping nodes whose refcount is already
zero while walking mmu->mm) would avoid grabbing a dying mapping.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110-panfrost-long-jobs-v1-0-24508939ae8d@gmail.com?part=7