Re: [PATCH] accel/amdxdna: return early from a zero-length flush

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