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

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

[Eva Crystal]

[ 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;
+
 	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
-- 
2.53.0

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

[Sasha Levin]

> This applies only that check: an offset at or past the BO's end, or
> an overflowing offset plus size, returns -EINVAL.

Thanks for the fix. Ideally a maintainer could ack it.

-- 
Thanks,
Sasha

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

[Lizhi Hou]

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

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

[Eva Crystal]

On 10/10/26, Lizhi Hou wrote:
> It needs to check if size is zero.

Thanks for the ack.

The zero-length guard from dc1475366424 is already on 6.18.y as
45962da5821d, shipped in 6.18.52, moved into
amdxdna_drm_sync_bo_ioctl() because amdxdna_flush_bo() does not exist
on this branch.

My check at drivers/accel/amdxdna/amdxdna_gem.c:947 runs before it,
and the clamped size cannot reach the drm_clflush_virt_range() at
drivers/accel/amdxdna/amdxdna_gem.c:966 as 0: with args->offset below
abo->mem.size, min(abo->mem.size, end) exceeds args->offset whenever
args->size is above 0, and an args->size of 0 already returns 0 at the
guard.

That is by code reading. The arms I measured on this branch all used
size 0x40, so I have no zero-size run.

A zero-length sync at an offset past the end returns -EINVAL from my
check, where upstream reports no error for AMDXDNA_BO_DEV, as my
commit message notes.

So no second patch is needed. v1 applies as is with your Acked-by, or
I will resend as v2 carrying the tag if stable prefers.

Eva Crystal (0xiviel)
XSource Security
https://xsourcesec.com