[PATCH v3 0/3] accel/amdxdna: SYNC_BO correctness fixes

[PATCH v3 0/3] accel/amdxdna: SYNC_BO correctness fixes

[Taimuraz Kaitmazov]

Three independent fixes in and around amdxdna_drm_sync_bo_ioctl(). None of
them depend on each other.

Patch 1 refuses an I/O memory mapping of an imported BO, which the driver
currently stores as if it were an ordinary kernel address. Patches 2 and 3
fix two ways the ioctl mishandles its own range: a zero length reaching
drm_clflush_virt_range(), and an offset and size added to the BO address
without an overflow check, one level above a function that checks the same
arithmetic.

Changes in v3:
  - dropped the range patch ("flush only the requested range") and the
    quiet-vmap patch that existed only to serve it. Christian NAKed
    flushing an imported dma-buf, and the numbers in that patch came from
    exactly that case; I also have not tested its premise on a matching
    tree.
  - patch 1: corrected a wrong sentence in the commit message about which
    errno drm_gem_vmap_locked() returns.
  - patch 2: also says what changes for an imported BO, where a zero-length
    request currently flushes the whole scatterlist.

Refusing a sync on a genuinely foreign import is still being discussed on
the v2 thread and is not part of this series.

v2: https://lore.kernel.org/all/20260811231351.1011244-1-taimuraz@kaitmazov.com/

Compile-tested on drm-misc-next only: x86_64 defconfig with
DRM_ACCEL_AMDXDNA=m, gcc 16.2.1, no W=1 warnings. Not booted, and not run
on hardware.

Taimuraz Kaitmazov (3):
  accel/amdxdna: refuse an I/O memory mapping of an imported BO
  accel/amdxdna: return early from a zero-length flush
  accel/amdxdna: check the sync range for overflow on a device BO

 drivers/accel/amdxdna/amdxdna_gem.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

-- 
2.55.0

[PATCH v3 0/3] accel/amdxdna: SYNC_BO correctness fixes

[Taimuraz Kaitmazov]

Three independent fixes in and around amdxdna_drm_sync_bo_ioctl(). None of
them depend on each other.

Patch 1 refuses an I/O memory mapping of an imported BO, which the driver
currently stores as if it were an ordinary kernel address. Patches 2 and 3
fix two ways the ioctl mishandles its own range: a zero length reaching
drm_clflush_virt_range(), and an offset and size added to the BO address
without an overflow check, one level above a function that checks the same
arithmetic.

Changes in v3:
  - dropped the range patch ("flush only the requested range") and the
    quiet-vmap patch that existed only to serve it. Christian NAKed
    flushing an imported dma-buf, and the numbers in that patch came from
    exactly that case; I also have not tested its premise on a matching
    tree.
  - patch 1: corrected a wrong sentence in the commit message about which
    errno drm_gem_vmap_locked() returns.
  - patch 2: also says what changes for an imported BO, where a zero-length
    request currently flushes the whole scatterlist.

Refusing a sync on a genuinely foreign import is still being discussed on
the v2 thread and is not part of this series.

v2: https://lore.kernel.org/all/20260811231351.1011244-1-taimuraz@kaitmazov.com/

Compile-tested on drm-misc-next only: x86_64 defconfig with
DRM_ACCEL_AMDXDNA=m, gcc 16.2.1, no W=1 warnings. Not booted, and not run
on hardware.

Taimuraz Kaitmazov (3):
  accel/amdxdna: refuse an I/O memory mapping of an imported BO
  accel/amdxdna: return early from a zero-length flush
  accel/amdxdna: check the sync range for overflow on a device BO

 drivers/accel/amdxdna/amdxdna_gem.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

-- 
2.55.0

[PATCH v3 0/3] accel/amdxdna: SYNC_BO correctness fixes

[Taimuraz Kaitmazov]

Three independent fixes in and around amdxdna_drm_sync_bo_ioctl(). None of
them depend on each other.

Patch 1 refuses an I/O memory mapping of an imported BO, which the driver
currently stores as if it were an ordinary kernel address. Patches 2 and 3
fix two ways the ioctl mishandles its own range: a zero length reaching
drm_clflush_virt_range(), and an offset and size added to the BO address
without an overflow check, one level above a function that checks the same
arithmetic.

Changes in v3:
  - dropped the range patch ("flush only the requested range") and the
    quiet-vmap patch that existed only to serve it. Christian NAKed
    flushing an imported dma-buf, and the numbers in that patch came from
    exactly that case; I also have not tested its premise on a matching
    tree.
  - patch 1: corrected a wrong sentence in the commit message about which
    errno drm_gem_vmap_locked() returns.
  - patch 2: also says what changes for an imported BO, where a zero-length
    request currently flushes the whole scatterlist.

Refusing a sync on a genuinely foreign import is still being discussed on
the v2 thread and is not part of this series.

v2: https://lore.kernel.org/all/20260811231351.1011244-1-taimuraz@kaitmazov.com/

Compile-tested on drm-misc-next only: x86_64 defconfig with
DRM_ACCEL_AMDXDNA=m, gcc 16.2.1, no W=1 warnings. Not booted, and not run
on hardware.

Taimuraz Kaitmazov (3):
  accel/amdxdna: refuse an I/O memory mapping of an imported BO
  accel/amdxdna: return early from a zero-length flush
  accel/amdxdna: check the sync range for overflow on a device BO

 drivers/accel/amdxdna/amdxdna_gem.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

-- 
2.55.0

[PATCH v3 1/3] accel/amdxdna: refuse an I/O memory mapping of an imported BO

[Taimuraz Kaitmazov]

amdxdna_gem_obj_vmap() takes whatever dma_buf_vmap() returns and only
rejects a NULL vaddr. iosys_map is discriminated by is_iomem, so an
exporter answering with an I/O mapping leaves a void __iomem pointer in
abo->mem.kva, which amdxdna_cmd_set_error() memsets and memcpys through.

amdxdna_drm_va_tbl takes a dmabuf_fd, so such a BO can be any exporter's
buffer. amdgpu cannot reach this: its .pin forces GTT for a non peer to
peer attachment like ours. An exporter on drm_gem_prime_dmabuf_ops has
no .pin, and drm_gem_ttm_vmap() answers iomem for a VRAM resident
object, so an NPU paired with nouveau or radeon does.

Refuse the mapping. vmw_gem_vmap() does the same; unlike that one this
path is reachable from an unprivileged ioctl, so it does not warn.

Signed-off-by: Taimuraz Kaitmazov <taimuraz@kaitmazov.com>
---
 drivers/accel/amdxdna/amdxdna_gem.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
index 1f190b319bb..b66ec9e4828 100644
--- a/drivers/accel/amdxdna/amdxdna_gem.c
+++ b/drivers/accel/amdxdna/amdxdna_gem.c
@@ -683,10 +683,16 @@ static int amdxdna_gem_obj_vmap(struct drm_gem_object *obj, struct iosys_map *ma
 
 	dma_resv_assert_held(obj->resv);
 
-	if (is_import_bo(abo))
+	if (is_import_bo(abo)) {
 		ret = dma_buf_vmap(abo->dma_buf, map);
-	else
+		/* Callers use mem.kva as an ordinary kernel address. */
+		if (!ret && map->is_iomem) {
+			dma_buf_vunmap(abo->dma_buf, map);
+			return -EOPNOTSUPP;
+		}
+	} else {
 		ret = drm_gem_shmem_object_vmap(obj, map);
+	}
 	if (ret)
 		return ret;
 	if (!map->vaddr)
-- 
2.55.0

[PATCH v3 1/3] accel/amdxdna: refuse an I/O memory mapping of an imported BO

[Taimuraz Kaitmazov]

amdxdna_gem_obj_vmap() takes whatever dma_buf_vmap() returns and only
rejects a NULL vaddr. iosys_map is discriminated by is_iomem, so an
exporter answering with an I/O mapping leaves a void __iomem pointer in
abo->mem.kva, which amdxdna_cmd_set_error() memsets and memcpys through.

amdxdna_drm_va_tbl takes a dmabuf_fd, so such a BO can be any exporter's
buffer. amdgpu cannot reach this: its .pin forces GTT for a non peer to
peer attachment like ours. An exporter on drm_gem_prime_dmabuf_ops has
no .pin, and drm_gem_ttm_vmap() answers iomem for a VRAM resident
object, so an NPU paired with nouveau or radeon does.

Refuse the mapping. vmw_gem_vmap() does the same; unlike that one this
path is reachable from an unprivileged ioctl, so it does not warn.

Signed-off-by: Taimuraz Kaitmazov <taimuraz@kaitmazov.com>
---
 drivers/accel/amdxdna/amdxdna_gem.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
index 1f190b319bb..b66ec9e4828 100644
--- a/drivers/accel/amdxdna/amdxdna_gem.c
+++ b/drivers/accel/amdxdna/amdxdna_gem.c
@@ -683,10 +683,16 @@ static int amdxdna_gem_obj_vmap(struct drm_gem_object *obj, struct iosys_map *ma
 
 	dma_resv_assert_held(obj->resv);
 
-	if (is_import_bo(abo))
+	if (is_import_bo(abo)) {
 		ret = dma_buf_vmap(abo->dma_buf, map);
-	else
+		/* Callers use mem.kva as an ordinary kernel address. */
+		if (!ret && map->is_iomem) {
+			dma_buf_vunmap(abo->dma_buf, map);
+			return -EOPNOTSUPP;
+		}
+	} else {
 		ret = drm_gem_shmem_object_vmap(obj, map);
+	}
 	if (ret)
 		return ret;
 	if (!map->vaddr)
-- 
2.55.0

[PATCH v3 1/3] accel/amdxdna: refuse an I/O memory mapping of an imported BO

[Taimuraz Kaitmazov]

amdxdna_gem_obj_vmap() takes whatever dma_buf_vmap() returns and only
rejects a NULL vaddr. iosys_map is discriminated by is_iomem, so an
exporter answering with an I/O mapping leaves a void __iomem pointer in
abo->mem.kva, which amdxdna_cmd_set_error() memsets and memcpys through.

amdxdna_drm_va_tbl takes a dmabuf_fd, so such a BO can be any exporter's
buffer. amdgpu cannot reach this: its .pin forces GTT for a non peer to
peer attachment like ours. An exporter on drm_gem_prime_dmabuf_ops has
no .pin, and drm_gem_ttm_vmap() answers iomem for a VRAM resident
object, so an NPU paired with nouveau or radeon does.

Refuse the mapping. vmw_gem_vmap() does the same; unlike that one this
path is reachable from an unprivileged ioctl, so it does not warn.

Signed-off-by: Taimuraz Kaitmazov <taimuraz@kaitmazov.com>
---
 drivers/accel/amdxdna/amdxdna_gem.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
index 1f190b319bb..b66ec9e4828 100644
--- a/drivers/accel/amdxdna/amdxdna_gem.c
+++ b/drivers/accel/amdxdna/amdxdna_gem.c
@@ -683,10 +683,16 @@ static int amdxdna_gem_obj_vmap(struct drm_gem_object *obj, struct iosys_map *ma
 
 	dma_resv_assert_held(obj->resv);
 
-	if (is_import_bo(abo))
+	if (is_import_bo(abo)) {
 		ret = dma_buf_vmap(abo->dma_buf, map);
-	else
+		/* Callers use mem.kva as an ordinary kernel address. */
+		if (!ret && map->is_iomem) {
+			dma_buf_vunmap(abo->dma_buf, map);
+			return -EOPNOTSUPP;
+		}
+	} else {
 		ret = drm_gem_shmem_object_vmap(obj, map);
+	}
 	if (ret)
 		return ret;
 	if (!map->vaddr)
-- 
2.55.0

Re: [PATCH v3 1/3] accel/amdxdna: refuse an I/O memory mapping of an imported BO

[sashiko-bot]

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

Pre-existing issues:
- [High] dma_buf_vmap() is called without holding the required dma_resv lock.
--

--- Patch [1]: [PATCH v3 1/3] accel/amdxdna: refuse an I/O memory mapping of an imported BO ---
commit a77f8bd3c9728d98291116a86706c7b30d341618
Author: Taimuraz Kaitmazov <taimuraz@kaitmazov.com>

accel/amdxdna: refuse an I/O memory mapping of an imported BO

This patch modifies amdxdna_gem_obj_vmap() to refuse I/O memory mappings
from imported buffer objects. If an exporter returns an IOMEM mapping,
the mapping is rejected with -EOPNOTSUPP. This prevents invalid memory
accesses when later code assumes a regular kernel virtual address.

[ ... ]

> diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
> index b01f5d95fac83..d7d36c9523253 100644
> --- a/drivers/accel/amdxdna/amdxdna_gem.c
> +++ b/drivers/accel/amdxdna/amdxdna_gem.c
> @@ -710,10 +710,16 @@ static int amdxdna_gem_obj_vmap(struct drm_gem_object *obj, struct iosys_map *ma
>  
>  	dma_resv_assert_held(obj->resv);
>  
> -	if (is_import_bo(abo))
> +	if (is_import_bo(abo)) {
>  		ret = dma_buf_vmap(abo->dma_buf, map);

[Severity: High]
This is a pre-existing issue, but while looking at dma_buf_vmap() usage,
I noticed a missing lock in drivers/accel/amdxdna/amdxdna_cbuf.c.

Is dma_buf_vmap() being called without holding the required dma_resv lock
during carveout initialization?

When amdxdna_get_cbuf() exports a new dma_buf, it returns an unlocked buffer.
It then immediately calls amdxdna_cbuf_clear(), which invokes dma_buf_vmap()
on the dbuf without taking its reservation lock:

drivers/accel/amdxdna/amdxdna_cbuf.c:amdxdna_cbuf_clear() {
	...
	struct iosys_map vmap = IOSYS_MAP_INIT_VADDR(NULL);

	dma_buf_vmap(dbuf, &vmap);
	if (!vmap.vaddr)
		return -EFAULT;
	...
}

Because dma_buf_vmap() unconditionally asserts that this lock is held
(dma_resv_assert_held(dmabuf->resv)), does this unlocked call violate the
dma-buf API contract and produce kernel warnings?

> -	else
> +		/* Callers use mem.kva as an ordinary kernel address. */
> +		if (!ret && map->is_iomem) {
> +			dma_buf_vunmap(abo->dma_buf, map);
> +			return -EOPNOTSUPP;
> +		}
> +	} else {
>  		ret = drm_gem_shmem_object_vmap(obj, map);
> +	}
>  	if (ret)
>  		return ret;

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

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

[Taimuraz Kaitmazov]

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.

Signed-off-by: Taimuraz Kaitmazov <taimuraz@kaitmazov.com>
---
 drivers/accel/amdxdna/amdxdna_gem.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
index b66ec9e4828..813b212a53f 100644
--- a/drivers/accel/amdxdna/amdxdna_gem.c
+++ b/drivers/accel/amdxdna/amdxdna_gem.c
@@ -1232,6 +1232,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))
-- 
2.55.0

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

[Taimuraz Kaitmazov]

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.

Signed-off-by: Taimuraz Kaitmazov <taimuraz@kaitmazov.com>
---
 drivers/accel/amdxdna/amdxdna_gem.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
index b66ec9e4828..813b212a53f 100644
--- a/drivers/accel/amdxdna/amdxdna_gem.c
+++ b/drivers/accel/amdxdna/amdxdna_gem.c
@@ -1232,6 +1232,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))
-- 
2.55.0

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

[Taimuraz Kaitmazov]

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.

Signed-off-by: Taimuraz Kaitmazov <taimuraz@kaitmazov.com>
---
 drivers/accel/amdxdna/amdxdna_gem.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
index b66ec9e4828..813b212a53f 100644
--- a/drivers/accel/amdxdna/amdxdna_gem.c
+++ b/drivers/accel/amdxdna/amdxdna_gem.c
@@ -1232,6 +1232,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))
-- 
2.55.0

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

[sashiko-bot]

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 <taimuraz@kaitmazov.com>

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/20260813164700.43960-1-taimuraz@kaitmazov.com?part=2

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

[Taimuraz Kaitmazov]

amdxdna_drm_sync_bo_ioctl() forms the range for a device BO by adding the
caller's offset and size to the BO address without checking either, while
amdxdna_flush_bo() one call down guards the same arithmetic with
check_add_overflow().

A size that wraps flush_end leaves it below the heap it is clamped
against, so every heap fails the start >= end test, and a sync that asked
for more than the address space holds reports success having flushed
nothing. Reject it instead.

Signed-off-by: Taimuraz Kaitmazov <taimuraz@kaitmazov.com>
---
 drivers/accel/amdxdna/amdxdna_gem.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
index 813b212a53f..85ce709a366 100644
--- a/drivers/accel/amdxdna/amdxdna_gem.c
+++ b/drivers/accel/amdxdna/amdxdna_gem.c
@@ -1275,8 +1275,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;
+		}
 
 		xa_for_each_range(&client->dev_heap_xa, heap_id, heap,
 				  abo->heap_start_id, abo->heap_end_id) {
-- 
2.55.0

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

[Taimuraz Kaitmazov]

amdxdna_drm_sync_bo_ioctl() forms the range for a device BO by adding the
caller's offset and size to the BO address without checking either, while
amdxdna_flush_bo() one call down guards the same arithmetic with
check_add_overflow().

A size that wraps flush_end leaves it below the heap it is clamped
against, so every heap fails the start >= end test, and a sync that asked
for more than the address space holds reports success having flushed
nothing. Reject it instead.

Signed-off-by: Taimuraz Kaitmazov <taimuraz@kaitmazov.com>
---
 drivers/accel/amdxdna/amdxdna_gem.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
index 813b212a53f..85ce709a366 100644
--- a/drivers/accel/amdxdna/amdxdna_gem.c
+++ b/drivers/accel/amdxdna/amdxdna_gem.c
@@ -1275,8 +1275,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;
+		}
 
 		xa_for_each_range(&client->dev_heap_xa, heap_id, heap,
 				  abo->heap_start_id, abo->heap_end_id) {
-- 
2.55.0

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

[Taimuraz Kaitmazov]

amdxdna_drm_sync_bo_ioctl() forms the range for a device BO by adding the
caller's offset and size to the BO address without checking either, while
amdxdna_flush_bo() one call down guards the same arithmetic with
check_add_overflow().

A size that wraps flush_end leaves it below the heap it is clamped
against, so every heap fails the start >= end test, and a sync that asked
for more than the address space holds reports success having flushed
nothing. Reject it instead.

Signed-off-by: Taimuraz Kaitmazov <taimuraz@kaitmazov.com>
---
 drivers/accel/amdxdna/amdxdna_gem.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
index 813b212a53f..85ce709a366 100644
--- a/drivers/accel/amdxdna/amdxdna_gem.c
+++ b/drivers/accel/amdxdna/amdxdna_gem.c
@@ -1275,8 +1275,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;
+		}
 
 		xa_for_each_range(&client->dev_heap_xa, heap_id, heap,
 				  abo->heap_start_id, abo->heap_end_id) {
-- 
2.55.0

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

[sashiko-bot]

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

Pre-existing issues:
- [High] Unprivileged userspace can intentionally trigger a kernel WARN by passing out-of-bounds offset/size values for non-device BOs.
- [Medium] Missing bounds validation against `abo->mem.size` for `AMDXDNA_BO_DEV` objects allows out-of-bounds cache flushes within the same heap and silent failures.
- [Medium] If virtual mapping fails, the fallback logic flushes the entire BO regardless of the requested offset and size.
--

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

This commit adds overflow checks to the BO sync ioctl in the AMDXDNA_BO_DEV
path. It ensures that the caller's offset and size do not overflow when
calculating the flush start and end addresses.

> 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 should there also be a bounds check against
abo->mem.size here?

While the integer overflow is caught, if userspace passes an offset or size
exceeding abo->mem.size, it seems we could still perform out-of-bounds
cache flushes into other sub-allocations within the same client heap, and
return success for out-of-bounds requests.

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

[ ... ]

In the else branch of amdxdna_drm_sync_bo_ioctl() (handling non-device
BOs), there is a call to amdxdna_flush_bo() followed by a warning:

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;
		}
    ...
}

[Severity: High]
This is a pre-existing issue, but does this allow unprivileged userspace
to intentionally trigger a kernel WARN?

If userspace calls the sync IOCTL on a non-device BO with
args->offset >= abo->mem.size, amdxdna_flush_bo() legitimately returns
-EINVAL. The caller unconditionally catches this error from
amdxdna_flush_bo() and triggers drm_WARN(), which could spam the kernel
log or panic the system if panic_on_warn is enabled.

Also within amdxdna_flush_bo(), there is fallback logic if the virtual
mapping fails:

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);
    ...
}

[Severity: Medium]
This is a pre-existing issue, but if the virtual mapping fails, does this
fallback logic flush the entire BO regardless of the requested offset and
size?

Under heavy memory pressure, falling back to flushing the entire buffer
instead of the requested range could cause severe latency spikes and
performance degradation due to unnecessary over-flushing of potentially large
BOs.

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

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

[sashiko-bot]

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