Re: [PATCH 6.18.y] accel/amdxdna: Bound the sync_bo flush range to the BO size

[Lizhi Hou <lizhi.hou@amd.com>]

On 10/8/26 20:26, Eva Crystal wrote:
> [ Upstream commit dbc8fd7a03cbc0704e8e558a448015f620547a02 ]
>
> amdxdna_drm_sync_bo_ioctl() passes the caller's offset and size to
> drm_clflush_virt_range() on the BO's kernel mapping without checking
> either against the BO size. The ioctl has no permission flags: any
> process that can open the accel node controls the offset. Any BO with a
> kernel mapping reaches that call: AMDXDNA_BO_DEV_HEAP, AMDXDNA_BO_DEV
> and AMDXDNA_BO_CMD. A DEV_HEAP is vmapped, so an offset equal to its
> size lands the flush on the vmap guard page.
>
> Measured on 6.18.55 on an AMD Ryzen AI 9 365 NPU, firmware 1.0.0.63,
> as a non-root user with access to the accel node: unpatched, an offset
> equal to the 64 MiB DEV_HEAP size oopses in drm_clflush_virt_range()
> under amdxdna_drm_sync_bo_ioctl(), 2 of 2 runs; patched, the same call
> returns -EINVAL, 2 of 2, and an in-bounds sync returns 0, 2 of 2.
> DEV_HEAP is the type tested; DEV and CMD are by code reading.
>
> Upstream bounds the range in amdxdna_flush_bo(), added by commit
> dbc8fd7a03cb ("accel/amdxdna: Add expandable device heap support").
> This applies only that check: an offset at or past the BO's end, or
> an overflowing offset plus size, returns -EINVAL.
>
> Deviations from upstream: that commit adds a feature and is too large
> for stable as a whole. It calls drm_WARN() when the check rejects a BO
> whose type is not AMDXDNA_BO_DEV, which userspace can trigger; this
> fix does not. Upstream bounds AMDXDNA_BO_DEV by the heap chunks it
> spans and flushes only the overlap, reporting no error for an offset
> past the end; 6.18 has one heap per client, so this fix uses the BO's
> own abo->mem.size and rejects such an offset instead. As in upstream,
> a size past the BO's end is clamped, not rejected.
>
> Built from v6.18.55 with the test system's distro-derived config,
> DRM_ACCEL_AMDXDNA=m. The touched file also compiles W=1 clean.
>
> Fixes: e252e3f3488a ("accel/amdxdna: Revise device bo creation and free")
> Cc: stable@vger.kernel.org # 6.18.x
> Assisted-by: LLM
> Signed-off-by: Eva Crystal <0xiviel@gmail.com>
> ---
>   drivers/accel/amdxdna/amdxdna_gem.c | 10 +++++++++-
>   1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
> index ca747457cec7..95d04af2b993 100644
> --- a/drivers/accel/amdxdna/amdxdna_gem.c
> +++ b/drivers/accel/amdxdna/amdxdna_gem.c
> @@ -934,6 +934,7 @@ int amdxdna_drm_sync_bo_ioctl(struct drm_device *dev,
>   	struct amdxdna_drm_sync_bo *args = data;
>   	struct amdxdna_gem_obj *abo;
>   	struct drm_gem_object *gobj;
> +	u64 end, size;
>   	int ret;
>   
>   	gobj = drm_gem_object_lookup(filp, args->handle);
> @@ -943,6 +944,13 @@ int amdxdna_drm_sync_bo_ioctl(struct drm_device *dev,
>   	}
>   	abo = to_xdna_obj(gobj);
>   
> +	if (args->offset >= abo->mem.size ||
> +	    check_add_overflow(args->offset, args->size, &end)) {
> +		ret = -EINVAL;
> +		goto put_obj;
> +	}
> +	size = min(abo->mem.size, end) - args->offset;
> +

Thanks for the patch. It needs to check if size is zero. Please see

         dc1475366424 ("accel/amdxdna: return early from a zero-length 
flush")

Could you help to backport this as well?

Acked-by: Lizhi Hou <lizhi.hou@amd.com>

>   	ret = amdxdna_gem_pin(abo);
>   	if (ret) {
>   		XDNA_ERR(xdna, "Pin BO %d failed, ret %d", args->handle, ret);
> @@ -955,7 +963,7 @@ int amdxdna_drm_sync_bo_ioctl(struct drm_device *dev,
>   	if (is_import_bo(abo))
>   		drm_clflush_sg(abo->base.sgt);
>   	else if (abo->mem.kva)
> -		drm_clflush_virt_range(abo->mem.kva + args->offset, args->size);
> +		drm_clflush_virt_range(abo->mem.kva + args->offset, size);
>   	else if (abo->base.pages)
>   		drm_clflush_pages(abo->base.pages, gobj->size >> PAGE_SHIFT);
>   	else