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

[sashiko-bot@kernel.org]

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