Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
Pre-existing issues: - [High] Calling drm_clflush_virt_range with length 0 causes a page fault, and this can be triggered from aie2_cmdlist_multi_execbuf. - [High] drm_clflush_virt_range() performs an unconditional out-of-bounds read when called with a length of 0, triggering a kernel panic. - [High] Flushing a DEV BO iterates over its heaps and flushes them without pinning them, which can lead to a use-after-free if the DRM shrinker reclaims the heap's pages. - [Medium] Userspace can trigger a kernel WARN (and potential panic if panic_on_warn is set) by passing invalid parameters to the SYNC_BO ioctl. -- --- Patch [1]: [PATCH] accel/amdxdna: return early from a zero-length flush --- commit 0c882e06a9bf65285c4bb50a3200aa5bf9fa60e9 Author: Taimuraz Kaitmazov <[email protected]> accel/amdxdna: return early from a zero-length flush This commit fixes a kernel panic triggered when a zero-length flush request reaches drm_clflush_virt_range(), which accesses the byte before the mapping. It avoids the crash by returning early from amdxdna_flush_bo() if the size is zero. > diff --git a/drivers/accel/amdxdna/amdxdna_gem.c > b/drivers/accel/amdxdna/amdxdna_gem.c > index b01f5d95fac83..d18de7eb7af47 100644 > --- a/drivers/accel/amdxdna/amdxdna_gem.c > +++ b/drivers/accel/amdxdna/amdxdna_gem.c > @@ -1253,6 +1253,9 @@ static int amdxdna_flush_bo(struct amdxdna_gem_obj > *abo, u64 offset, u64 size) > return -EINVAL; > > size = min(abo->mem.size, end) - offset; > + if (!size) > + return 0; > + [Severity: High] This is a pre-existing issue, but by only guarding the SYNC_BO path here, is there still a vulnerable path in aie2_cmdlist_multi_execbuf()? If unprivileged userspace submits an ERT_CMD_CHAIN command with payload->command_count == 0, the offset remains 0. The function aie2_cmdlist_multi_execbuf() in drivers/accel/amdxdna/aie2_message.c bypasses its inner loop and directly calls: drm_clflush_virt_range(cmd_buf, offset); Since offset is 0, this will bypass amdxdna_flush_bo() entirely and still trigger the kernel panic in drm_clflush_virt_range(). [Severity: High] This isn't a bug introduced by this patch, but while fixing the zero-length flush here, does the root cause in drm_clflush_virt_range() need addressing as well? drm_clflush_virt_range() in drivers/gpu/drm/drm_cache.c does: clflushopt(end - 1); /* force serialisation */ Since it unconditionally accesses memory prior to the buffer, calling it with a length of 0 causes a guard page fault. Any kernel caller that legitimately flushes a zero-length region can crash the system. [Severity: High] This is a pre-existing issue, but can flushing a DEV BO lead to a use-after-free? When flushing a DEV BO, amdxdna_drm_sync_bo_ioctl() iterates over its heaps and calls: ret = amdxdna_flush_bo(heap, start - heap_start, end - start); This happens without first pinning the heap BO. Later in amdxdna_flush_bo(): drm_clflush_pages(abo->base.pages, abo->mem.size >> PAGE_SHIFT); This accesses the unpinned pages array locklessly. If memory pressure invokes the DRM shmem shrinker concurrently, it could unpopulate and free the BO's pages, causing the flush operation to access freed memory. [Severity: Medium] This is a pre-existing issue, but can userspace trigger a kernel WARN by passing an out-of-bounds offset? If an unprivileged user calls the SYNC_BO ioctl with an args->offset larger than the BO size, amdxdna_flush_bo() safely catches the bounds violation and returns -EINVAL. However, back in amdxdna_drm_sync_bo_ioctl() in drivers/accel/amdxdna/amdxdna_gem.c: if (ret) { drm_WARN(&xdna->ddev, 1, "Can not get flush memory"); } This translates the user-provided bounds violation into a kernel WARN, which can result in a Denial of Service on systems configured with panic_on_warn. > if (is_import_bo(abo)) > drm_clflush_sg(abo->base.sgt); > else if (amdxdna_gem_vmap(abo)) -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
