Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
Pre-existing issues: - [High] The `SYNC_BO` ioctl unconditionally maps the target buffer into kernel virtual memory, allowing unprivileged userspace to exhaust `vmalloc` space and permanently pin physical memory (DoS). - [High] Dangling `abo->client` pointer causes Use-After-Free in `amdxdna_gem_del_bo_usage` when a shared BO outlives its creator client. - [High] Lockless access to `abo->base.pages` in `amdxdna_flush_bo` causes Use-After-Free during `SYNC_BO` on DEV BOs if vmap fails. - [Medium] Integer overflow in `flush_end` computation causes `SYNC_BO` to silently skip flushing DEV BOs when `args->size` is `U64_MAX`. -- --- Patch [2]: [PATCH v3 2/3] accel/amdxdna: return early from a zero-length flush --- commit af59e76eb538c56b42cb7c548539fdedd358a3a4 Author: Taimuraz Kaitmazov <[email protected]> accel/amdxdna: return early from a zero-length flush SYNC_BO does not constrain its size, so a request for zero bytes reaches amdxdna_flush_bo(). On the vmap path that calls drm_clflush_virt_range() with a length of zero, which flushes the byte in front of the region rather than anything the caller asked to maintain; on the import path it flushes the whole scatterlist, since that arm ignores the range. Nothing needs flushing for an empty range, so answer before choosing a path. > diff --git a/drivers/accel/amdxdna/amdxdna_gem.c > b/drivers/accel/amdxdna/amdxdna_gem.c > index d7d36c9523253..1798003cdd45c 100644 > --- a/drivers/accel/amdxdna/amdxdna_gem.c > +++ b/drivers/accel/amdxdna/amdxdna_gem.c [ ... ] > static void > amdxdna_gem_del_bo_usage(struct amdxdna_gem_obj *abo) > { > struct amdxdna_client *client = abo->client; > > if (amdxdna_gem_skip_bo_usage(abo)) > return; > > guard(mutex)(&client->mm_lock); [Severity: High] This isn't a bug introduced by this patch, but can abo->client be a dangling pointer here? When a BO is exported via PRIME and imported by another process, open_ref becomes 2, but abo->client is not updated because amdxdna_gem_obj_open() returns early: amdxdna_gem.c:amdxdna_gem_obj_open() { ... abo->open_ref++; if (abo->open_ref > 1) return 0; ... } If the creator process closes the BO and exits, its amdxdna_client struct is freed, leaving abo->client dangling. When the importing process later closes the BO, amdxdna_gem_obj_close() calls amdxdna_gem_del_bo_usage(), which attempts to acquire the lock inside the freed client struct. Could this lead to a Use-After-Free? [ ... ] > @@ -1259,6 +1259,9 @@ static int amdxdna_flush_bo(struct amdxdna_gem_obj > *abo, u64 offset, u64 size) > return -EINVAL; > > size = min(abo->mem.size, end) - offset; > + if (!size) > + return 0; > + > if (is_import_bo(abo)) > drm_clflush_sg(abo->base.sgt); > else if (amdxdna_gem_vmap(abo)) [Severity: High] This is a pre-existing issue, but does this unconditionally map the target buffer into kernel virtual memory without ever unmapping it? Any unprivileged user can allocate large BOs and call the SYNC_BO ioctl on them. Since amdxdna_gem_vunmap() is only called on free, does this allow an attacker to exhaust vmalloc space and permanently pin physical memory? > drm_clflush_virt_range(amdxdna_gem_vmap(abo) + offset, size); > else if (abo->base.pages) [Severity: High] This isn't a bug introduced by this patch, but is abo->base.pages accessed locklessly while the DEV BO is completely unpinned? During SYNC_BO on an AMDXDNA_BO_DEV, the DEV BO and its heaps are not pinned. If amdxdna_gem_vmap() fails, the code falls back here. Since the BO is unpinned and no locks are held, can a concurrent thread unpin the BO and cause the shrinker to free the pages array, leading to a Use-After-Free in drm_clflush_pages()? > drm_clflush_pages(abo->base.pages, abo->mem.size >> PAGE_SHIFT); [ ... ] > if (abo->type == AMDXDNA_BO_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; [Severity: Medium] This is a pre-existing issue, but can flush_end overflow here? If a user passes U64_MAX as args->size, flush_end wraps around to a small value. This makes the intersection check (start >= end) evaluate to true, silently skipping the flush. Could this lead to data corruption for the application using the NPU? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
