Re: [PATCH v4 0/5] accel/amdxdna: SYNC_BO correctness fixes

Re: [PATCH v4 0/5] accel/amdxdna: SYNC_BO correctness fixes

[Lizhi Hou]

On 8/18/26 14:40, Taimuraz Kaitmazov wrote:
> Patch 1 needs a prerequisite. It makes amdxdna_gem_vmap() answer NULL 
> on an iomem exporter, and the eight amdxdna_cmd_get_payload() callers 
> in aie2_message.c check neither the pointer nor the length it leaves 
> unwritten there. A command BO can be an import, so patch 1 alone turns 
> a silent __iomem write into a NULL deref with an uninitialised length.
> https://lore.kernel.org/all/20260818002459.377641-1-taimuraz@kaitmazov.com/ 
>

amdxdna_cmd_get_op() is always called before amdxdna_cmd_get_payload() 
for the same BO, and since amdxdna_gem_vmap() caches its results, the 
vmap inside get_payload() is currently guaranteed to succeed. The patch 
is therefore defensive rather than fixing a currently reachable crash.

Lizhi

>
> fixes the callers. Happy to respin on top if you prefer them together.
>
> Taimuraz
>
> On 8/18/26 02:07, Taimuraz Kaitmazov wrote:
>> Five fixes in and around amdxdna_drm_sync_bo_ioctl().
>>
>> Patch 1 refuses an I/O memory mapping the driver would otherwise 
>> store as
>> if it were an ordinary kernel address. Patch 2 checks the device-BO 
>> range
>> for overflow. Patch 4 refuses a flush of an imported BO, which is why
>> patch 3 comes first: the ioctl answers a rejected flush with drm_WARN(),
>> so without it an ordinary sync on an imported BO splats. Patch 5 stops
>> the ioctl reporting failure for a flush that succeeded.
>>
>> v3's zero-length patch has left this series. On hardware it turns out to
>> be a page fault in drm_clflush_virt_range() rather than the tidy-up its
>> commit message described, so it is a -fixes patch now, sent 
>> separately as
>> "accel/amdxdna: return early from a zero-length flush" with Fixes: and
>> Cc: stable. Patch 4 here needs its hunk, so this series wants that one
>> first.
>>
>> Changes in v4:
>>    - patch 1: the is_iomem check moved from the .vmap callback into
>>      amdxdna_gem_vmap(), per Lizhi, and it logs at debug level rather 
>> than
>>      error, since an unprivileged caller can repeat it.
>>    - new patch 3: an unprivileged SYNC_BO with an out-of-range offset
>>      already reaches that drm_WARN() today. Sashiko's review of v3 3/3
>>      flagged the same thing.
>>    - new patch 4: refuses the flush for every imported BO, as asked.
>>    - new patch 5: the debug-BO sync I mentioned on the v2 thread. 
>> Only a BO
>>      attached with ATTACH_DEBUG_BO has an assigned hwctx, so every other
>>      FROM_DEVICE sync ends in -EINVAL with its flush already done. The
>>      -EINVAL reproduces on a Strix Point NPU.
>>
>> On patch 4, one consequence worth deciding before it lands. A heap BO is
>> created through amdxdna_drm_create_share_bo(), so a device running on
>> carveout memory reaches its heap through a cbuf, is_import_bo() is 
>> true of
>> it, and SYNC_BO on every AMDXDNA_BO_DEV now answers -EOPNOTSUPP. Today
>> that path flushes nothing anyway -- amdxdna_cbuf_map() fills in only the
>> DMA address and length, so drm_clflush_sg() walks zero pages -- so the
>> change is silent no-op to hard error, and XRT's dbg_buffer::sync() 
>> reaches
>> it without Debug.force_driver_sync. If you would rather keep our own
>> exporters working, I have the variant keyed on the exporter's ops, which
>> confines the refusal to foreign buffers. Say which you prefer.
>>
>> v3: 
>> https://lore.kernel.org/all/20260813164700.43960-1-taimuraz@kaitmazov.com/
>>
>> Built on drm-misc-next, each commit on its own: x86_64 with
>> DRM_ACCEL_AMDXDNA=m, clang 22.1.8, no warnings. Patch 5's reproducer was
>> run against 7.1.8's in-tree driver, where that call is unchanged.
>>
>> Taimuraz Kaitmazov (5):
>>    accel/amdxdna: refuse an I/O memory mapping of an imported BO
>>    accel/amdxdna: check the sync range for overflow on a device BO
>>    accel/amdxdna: do not warn when a sync request is rejected
>>    accel/amdxdna: refuse to flush an imported BO
>>    accel/amdxdna: do not fail a sync for a BO with no debug context
>>
>>   drivers/accel/amdxdna/amdxdna_ctx.c |  3 ++-
>>   drivers/accel/amdxdna/amdxdna_gem.c | 27 +++++++++++++++++++--------
>>   2 files changed, 21 insertions(+), 9 deletions(-)
>>
>
>

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

[Lizhi Hou]

On 8/17/26 16:07, Taimuraz Kaitmazov wrote:
> amdxdna_gem_vmap() flattens the iosys_map drm_gem_vmap() fills in down to
> the void * in abo->mem.kva, and iosys_map is discriminated by is_iomem, so
> an exporter answering with an I/O mapping leaves a void __iomem pointer
> there, 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.
>
> Drop such a mapping and answer NULL. Checking here rather than in the
> .vmap callback leaves that callback's iosys_map contract intact for a
> caller equipped to read I/O memory, and covers everything that takes a
> plain kernel address through this helper. vmw_gem_vmap() refuses the
> same case; unlike that one this path is reachable from an unprivileged
> ioctl, so it neither warns nor logs at error level.
>
> 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 cca84fa07e9d..f88b5349cd4b 100644
> --- a/drivers/accel/amdxdna/amdxdna_gem.c
> +++ b/drivers/accel/amdxdna/amdxdna_gem.c
> @@ -209,10 +209,15 @@ void *amdxdna_gem_vmap(struct amdxdna_gem_obj *abo)
>   
>   	if (!abo->mem.kva) {
>   		ret = drm_gem_vmap(to_gobj(abo), &map);
> -		if (ret)
> +		if (ret) {
>   			XDNA_ERR(abo->client->xdna, "Vmap bo failed, ret %d", ret);
> -		else
> +		} else if (map.is_iomem) {
> +			/* Callers use the result as an ordinary kernel address. */
> +			XDNA_DBG(abo->client->xdna, "Vmap bo returned I/O memory");
> +			drm_gem_vunmap(to_gobj(abo), &map);
> +		} else {
>   			abo->mem.kva = map.vaddr;
> +		}
Reviewed-by: Lizhi Hou <lizhi.hou@amd.com>
>   	}
>   	return abo->mem.kva;
>   }

Re: [PATCH v4 2/5] accel/amdxdna: check the sync range for overflow on a device BO

[Lizhi Hou]

On 8/17/26 16:07, Taimuraz Kaitmazov wrote:
> 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 f88b5349cd4b..77a9493cd7ba 100644
> --- a/drivers/accel/amdxdna/amdxdna_gem.c
> +++ b/drivers/accel/amdxdna/amdxdna_gem.c
> @@ -1274,8 +1274,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;
> +		}
Reviewed-by: Lizhi Hou <lizhi.hou@amd.com>
>   
>   		xa_for_each_range(&client->dev_heap_xa, heap_id, heap,
>   				  abo->heap_start_id, abo->heap_end_id) {

Re: [PATCH v4 3/5] accel/amdxdna: do not warn when a sync request is rejected

[Lizhi Hou]

On 8/17/26 16:07, Taimuraz Kaitmazov wrote:
> amdxdna_drm_sync_bo_ioctl() answers a failed amdxdna_flush_bo() with
> drm_WARN(). Both of that function's error returns are decided by the
> ioctl's arguments, so SYNC_BO with an offset past the end of the BO
> splats and taints the kernel from an unprivileged caller.
>
> Log it like the pin failure above it.
>
> Signed-off-by: Taimuraz Kaitmazov <taimuraz@kaitmazov.com>
> ---
>   drivers/accel/amdxdna/amdxdna_gem.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
> index 77a9493cd7ba..4f38f985c74e 100644
> --- a/drivers/accel/amdxdna/amdxdna_gem.c
> +++ b/drivers/accel/amdxdna/amdxdna_gem.c
> @@ -1310,7 +1310,7 @@ int amdxdna_drm_sync_bo_ioctl(struct drm_device *dev,
>   		amdxdna_gem_unpin(abo);
>   
>   		if (ret) {
> -			drm_WARN(&xdna->ddev, 1, "Can not get flush memory");
> +			XDNA_ERR(xdna, "Flush BO %d failed, ret %d", args->handle, ret);

Should it be XDNA_DBG?


Lizhi

>   			goto put_obj;
>   		}
>   	}

Re: [PATCH v4 4/5] accel/amdxdna: refuse to flush an imported BO

[Lizhi Hou]

On 8/17/26 16:07, Taimuraz Kaitmazov wrote:
> SYNC_BO clflushes an imported BO's scatterlist. An importer may not do
> that: the memory belongs to the exporter, and dma-buf gives the importer
> no interface to ask for maintenance on it. Refuse the request instead.
>
> is_import_bo() is (obj)->attach, which covers more than foreign buffers.
> A userptr BO arrives through a ubuf, and on a carveout device every share
> BO and the device heap arrive through a cbuf, so SYNC_BO answers
> -EOPNOTSUPP for those too, including the AMDXDNA_BO_DEV path that flushes
> through its heap.
>
> Only the ubuf case gives up maintenance it was getting: on a 64 MiB
> userptr BO a 4 KiB sync and a full sync both cost 659 us, this arm having
> ignored the range. amdxdna_cbuf_map() fills in only the DMA address and
> length, so drm_clflush_sg() already walks zero pages on carveout memory.
> Userspace maintains these through the mapping it already holds, as XRT's
> buffer::sync() does unless it is told to sync through the driver.
>
> Suggested-by: Lizhi Hou <lizhi.hou@amd.com>
> Signed-off-by: Taimuraz Kaitmazov <taimuraz@kaitmazov.com>
> ---
>   drivers/accel/amdxdna/amdxdna_gem.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
> index 4f38f985c74e..a713a9982d34 100644
> --- a/drivers/accel/amdxdna/amdxdna_gem.c
> +++ b/drivers/accel/amdxdna/amdxdna_gem.c
> @@ -1224,6 +1224,9 @@ static int amdxdna_flush_bo(struct amdxdna_gem_obj *abo, u64 offset, u64 size)
>   {
>   	u64 end;
>   
> +	if (is_import_bo(abo))
> +		return -EOPNOTSUPP;
> +
>   	if (offset >= abo->mem.size)
>   		return -EINVAL;
>   
> @@ -1234,9 +1237,7 @@ static int amdxdna_flush_bo(struct amdxdna_gem_obj *abo, u64 offset, u64 size)
>   	if (!size)
>   		return 0;
>   
> -	if (is_import_bo(abo))
> -		drm_clflush_sg(abo->base.sgt);
> -	else if (amdxdna_gem_vmap(abo))
> +	if (amdxdna_gem_vmap(abo))
Reviewed-by: Lizhi Hou <lizhi.hou@amd.com>
>   		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);

Re: [PATCH v4 5/5] accel/amdxdna: do not fail a sync for a BO with no debug context

[Lizhi Hou]

On 8/17/26 16:07, Taimuraz Kaitmazov wrote:
> amdxdna_drm_sync_bo_ioctl() calls amdxdna_hwctx_sync_debug_bo() for every
> FROM_DEVICE sync, which answers -EINVAL when the BO has no assigned hwctx.
> Only a BO attached with ATTACH_DEBUG_BO ever gets one, so an ordinary
> read-back sync reports failure after its flush has already run.
>
> There is no debug buffer to sync in that case, so answer success.
>
> Signed-off-by: Taimuraz Kaitmazov <taimuraz@kaitmazov.com>
> ---
>   drivers/accel/amdxdna/amdxdna_ctx.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/accel/amdxdna/amdxdna_ctx.c b/drivers/accel/amdxdna/amdxdna_ctx.c
> index 855da8c79a1c..c0d0aa53c596 100644
> --- a/drivers/accel/amdxdna/amdxdna_ctx.c
> +++ b/drivers/accel/amdxdna/amdxdna_ctx.c
> @@ -416,7 +416,8 @@ int amdxdna_hwctx_sync_debug_bo(struct amdxdna_client *client, u32 debug_bo_hdl)
>   	guard(mutex)(&xdna->dev_lock);
>   	hwctx = xa_load(&client->hwctx_xa, abo->assigned_hwctx);
>   	if (!hwctx) {
> -		ret = -EINVAL;
> +		/* Not attached as a debug BO, so there is nothing to sync. */
> +		ret = 0;

It should check assigned_hwctx before entering this function:

    if (abo->assigned_hwctx != AMDXDNA_INVALID_CTX_HANDLE && 
args->direction == SYNC_DIRECT_FROM_DEVICE)

        ret = amdxdna_hwctx_sync_debug_bo(client, args->handle);


Thanks,

Lizhi

>   		goto put_obj;
>   	}
>