Re: [PATCH v3 3/3] accel/amdxdna: check the sync range for overflow on a device BO

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

Pre-existing issues:
- [High] Unprivileged userspace can trigger a kernel warning (`drm_WARN`) by passing an out-of-bounds offset to the sync BO ioctl.
- [High] The `amdxdna_drm_sync_bo_ioctl` function flushes device BO heaps without pinning them, exposing a Use-After-Free race on the pages array.
- [High] Calling `amdxdna_gem_vmap()` during cache flushing permanently pins user BOs and consumes vmalloc space, leading to resource exhaustion.
- [Medium] The DEV BO sync path fails to validate the requested offset and size against the Buffer Object's actual logical bounds.
- [Medium] Double-checked locking on `abo->mem.kva` in `amdxdna_gem_vmap()` lacks proper memory barriers.
--

--- Patch [3]: [PATCH v3 3/3] accel/amdxdna: check the sync range for overflow on a device BO ---
commit d082579a99611b0a3869e9ef109a07c5c42aef65
Author: Taimuraz Kaitmazov <taimuraz@kaitmazov.com>
Subject: accel/amdxdna: check the sync range for overflow on a device BO

This commit fixes an integer overflow issue in amdxdna_drm_sync_bo_ioctl()
by using check_add_overflow() to calculate the flush range. It rejects
requests that wrap around the address space instead of silently reporting
success.

> diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
> index 1798003cdd45c..2507877fff29a 100644
> --- a/drivers/accel/amdxdna/amdxdna_gem.c
> +++ b/drivers/accel/amdxdna/amdxdna_gem.c
> @@ -1302,8 +1302,13 @@ int amdxdna_drm_sync_bo_ioctl(struct drm_device *dev,
>  		struct amdxdna_gem_obj *heap;
>  		unsigned long heap_id;
>  		u64 bo_start = amdxdna_gem_dev_addr(abo);
> -		u64 flush_start = bo_start + args->offset;
> -		u64 flush_end = flush_start + args->size;
> +		u64 flush_start, flush_end;
> +
> +		if (check_add_overflow(bo_start, args->offset, &flush_start) ||
> +		    check_add_overflow(flush_start, args->size, &flush_end)) {
> +			ret = -EINVAL;
> +			goto put_obj;
> +		}

[Severity: Medium]
This is a pre-existing issue, but the DEV BO sync path fails to validate the
requested offset and size against the Buffer Object's actual logical bounds.

While this check prevents integer wrap-around, does the code ever check if
args->offset >= abo->mem.size? It seems an unprivileged user can request
cache flushing for memory regions belonging to other DEV BOs allocated from
the same heap chunk, as the ioctl intersects this out-of-bounds range with
the physical address bounds of the heap chunks and successfully flushes them.

>  
>  		xa_for_each_range(&client->dev_heap_xa, heap_id, heap,
>  				  abo->heap_start_id, abo->heap_end_id) {

[Severity: High]
This isn't a bug introduced by this patch, but does flushing device BO heaps
without pinning them expose a use-after-free race on the pages array?

When flushing an AMDXDNA_BO_DEV object, the ioctl calls
amdxdna_flush_bo(heap) on the heap within this loop without pinning it.
If amdxdna_gem_vmap(heap) fails inside amdxdna_flush_bo(), it falls back
to accessing abo->base.pages:

drivers/accel/amdxdna/amdxdna_gem.c:amdxdna_flush_bo() {
    ...
    } else if (abo->base.pages) {
        drm_clflush_pages(abo->base.pages, abo->mem.size >> PAGE_SHIFT);
    }
    ...
}

Can a concurrent thread call the sync ioctl directly on the heap BO, pinning
and then unpinning it, freeing heap->base.pages while the first thread is
iterating over it in drm_clflush_pages()?


[Severity: High]
This is a pre-existing issue, but can unprivileged userspace trigger a
kernel warning if they pass an out-of-bounds offset to this ioctl for a
non-DEV BO?

If a user calls amdxdna_drm_sync_bo_ioctl() on a non-DEV BO with
args->offset >= abo->mem.size, amdxdna_flush_bo() correctly rejects it by
returning -EINVAL. The caller then unconditionally executes drm_WARN:

drivers/accel/amdxdna/amdxdna_gem.c:amdxdna_drm_sync_bo_ioctl() {
    ...
		ret = amdxdna_flush_bo(abo, args->offset, args->size);
		amdxdna_gem_unpin(abo);

		if (ret) {
			drm_WARN(&xdna->ddev, 1, "Can not get flush memory");
			goto put_obj;
		}
    ...
}

Can an unprivileged user trigger this kernel stack trace and potentially
crash the system if panic_on_warn is enabled?


[Severity: High]
This is a pre-existing issue, but does calling amdxdna_gem_vmap() during
cache flushing permanently pin BOs and consume vmalloc space?

In amdxdna_flush_bo(), cache flushing relies on amdxdna_gem_vmap():

drivers/accel/amdxdna/amdxdna_gem.c:amdxdna_flush_bo() {
    ...
    } else if (amdxdna_gem_vmap(abo)) {
        drm_clflush_virt_range(amdxdna_gem_vmap(abo) + offset, size);
    }
    ...
}

Because amdxdna_gem_vmap() establishes a persistent kernel virtual mapping
that is only released when the BO is finally destroyed, could a user
exhaust kernel vmalloc space and permanently pin physical memory by
repeatedly creating BOs and calling the SYNC_BO ioctl?


[Severity: Medium]
This is a pre-existing issue, but does the double-checked locking on
abo->mem.kva in amdxdna_gem_vmap() lack proper memory barriers?

drivers/accel/amdxdna/amdxdna_gem.c:amdxdna_gem_vmap() {
    if (abo->mem.kva)
        return abo->mem.kva;

    /* The first call to get the kva, taking slow path. */
    guard(mutex)(&abo->lock);
    ...
}

Could a concurrent thread observe a non-NULL kernel virtual address before
the page table writes or structural initialization inside drm_gem_vmap() are
fully committed to memory, since the lockless read lacks smp_load_acquire()
and the assignment inside the locked section lacks smp_store_release()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260813164700.43960-1-taimuraz@kaitmazov.com?part=3