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 <[email protected]> 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/[email protected]?part=3
