Re: [PATCH v3 2/3] accel/amdxdna: return early from a zero-length flush

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