Re: [PATCH 3/3] accel/amdxdna: flush only the requested range in amdxdna_flush_bo

[sashiko-bot@kernel.org]

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

New issues:
- [High] Holding a vmap mapping for the lifetime of an imported DMA-buf violates the dma-buf API and permanently pins the buffer in the exporter.

Pre-existing issues:
- [High] Userspace can trigger a kernel page fault (DoS) by passing size=0 to the AMDXDNA_DRM_SYNC_BO ioctl.
- [High] CPU cache flushes on imported DMA-BUFs are performed without dma_buf_begin_cpu_access() and dma_buf_end_cpu_access().
--

--- Patch [3]: [PATCH 3/3] accel/amdxdna: flush only the requested range in amdxdna_flush_bo ---
commit b478fba210b28db0ebe941498872d630befd0f64
Author: Taimuraz Kaitmazov <taimuraz@kaitmazov.com>

accel/amdxdna: flush only the requested range in amdxdna_flush_bo

This commit updates amdxdna_flush_bo() to only flush the requested range of a
buffer object instead of the entire buffer. It tries the vmap path first for
imported BOs, falling back to other flush methods. This reduces the time
spent syncing large BOs.

> diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
> index d0c846b02694b..4886f7c082da1 100644
> --- a/drivers/accel/amdxdna/amdxdna_gem.c
> +++ b/drivers/accel/amdxdna/amdxdna_gem.c
[ ... ]
> @@ -1243,12 +1245,16 @@ static int amdxdna_flush_bo(struct amdxdna_gem_obj *abo, u64 offset, u64 size)
>  		return -EINVAL;
>  
>  	size = min(abo->mem.size, end) - offset;
> -	if (is_import_bo(abo))
> +	first = offset >> PAGE_SHIFT;
> +	nr_pages = (PAGE_ALIGN(offset + size) >> PAGE_SHIFT) - first;
> +
> +	kva = __amdxdna_gem_vmap(abo);

[Severity: High]
Does this code pin the imported buffer in the exporter indefinitely?

When __amdxdna_gem_vmap() is called on an imported BO in
drivers/accel/amdxdna/amdxdna_gem.c, it uses dma_buf_vmap() which establishes a
persistent mapping stored in abo->mem.kva.

According to the dma-buf API, mappings must not be held for the lifetime of the
buffer because they prevent the exporter (like AMDGPU) from migrating the
buffer. By replacing the transient drm_clflush_sg() fallback with a persistent
vmap, userspace can intentionally or unintentionally pin VRAM in the exporter,
potentially leading to GPU memory exhaustion or eviction failures.

> +	if (!IS_ERR(kva))
> +		drm_clflush_virt_range(kva + offset, size);

[Severity: High]
This is a pre-existing issue, but can this cause a kernel page fault if
userspace passes size=0 to the AMDXDNA_DRM_SYNC_BO ioctl?

In amdxdna_drm_sync_bo_ioctl(), if args->size is 0, it gets passed to
amdxdna_flush_bo(). This calculates size = 0 and calls drm_clflush_virt_range()
with a length of 0.

In drm_clflush_virt_range() on x86, if length is 0, the for loop is skipped,
but it unconditionally executes:

    clflushopt(end - 1);

Since vmap allocations are often preceded by unmapped guard pages, accessing
kva - 1 can trigger an unhandled kernel page fault, potentially allowing a
local denial of service.

[Severity: High]
This isn't a bug introduced by this patch, but does this perform CPU cache
flushes on imported DMA-BUFs without invoking dma_buf_begin_cpu_access() and
dma_buf_end_cpu_access()?

The dma-buf API mandates that importers must bracket any CPU access to imported
buffers with these hooks. Bypassing the exporter's specific cache maintenance
requirements can potentially lead to cache coherency issues or data corruption.

> +	else if (is_import_bo(abo))
>  		drm_clflush_sg(abo->base.sgt);
> -	else if (amdxdna_gem_vmap(abo))
> -		drm_clflush_virt_range(amdxdna_gem_vmap(abo) + offset, size);
>  	else if (abo->base.pages)
> -		drm_clflush_pages(abo->base.pages, abo->mem.size >> PAGE_SHIFT);
> +		drm_clflush_pages(&abo->base.pages[first], nr_pages);
>  	else
>  		return -EINVAL;

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