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 <[email protected]> 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/[email protected]?part=3
