Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [Critical] Unhandled error path leaks BOs and leaves dangling pointers to a stack-allocated list, leading to stack corruption. - [High] Sub-page offset is dropped when mapping contiguous system memory on non-4K page kernels, causing data corruption. -- --- Patch [2]: [PATCH 2/3] drm/amdgpu: fix VM update overrun on non-4K page kernels --- commit 250ebbca695a7882625e4e4f1a9f6af7c33fde76 Author: Junrui Luo <[email protected]> drm/amdgpu: fix VM update overrun on non-4K page kernels This commit fixes a VM update overrun bug in amdgpu_vm_update_range() on systems where the CPU page size is larger than the GPU page size. It clamps num_entries against the rounded-up value during the contiguity scan. This prevents an issue where an unprivileged caller could trigger a BUG_ON by mapping memory with a non-multiple GPU page count. > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > index dc6a9d7dd0b28..365a1c4a45275 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > @@ -1193,8 +1193,9 @@ int amdgpu_vm_update_range(struct amdgpu_device *adev, > struct amdgpu_vm *vm, > } > if (!contiguous) > count--; > - num_entries = count * > - AMDGPU_GPU_PAGES_IN_CPU_PAGE; > + num_entries = min(count * > + AMDGPU_GPU_PAGES_IN_CPU_PAGE, > + num_entries); > } > > if (!contiguous) { [Severity: High] This is a pre-existing issue, but does this code drop the sub-page offset when mapping contiguous memory? In amdgpu_vm_update_range(), if a user maps system memory where PAGE_SIZE > AMDGPU_GPU_PAGE_SIZE at a sub-CPU-page offset and the pages are physically contiguous, addr is set to pages_addr[cursor.start >> PAGE_SHIFT]. Because params.pages_addr is set to NULL, the sub-page offset (cursor.start & ~PAGE_MASK) is dropped. Could this cause the GPU to map memory starting from the beginning of the CPU page instead of the requested offset, returning incorrect data to the user? [Severity: Critical] This is a pre-existing issue, but does the error_free path in this function leave dangling pointers to the stack-allocated params.tlb_flush_waitlist? During amdgpu_vm_ptes_update() later in this loop, replaced page table entries are added to params.tlb_flush_waitlist, which is allocated on the stack in amdgpu_vm_update_range(). If a subsequent operation fails, the code jumps to error_free: r = amdgpu_vm_ptes_update(¶ms, start, tmp, addr, flags); if (r) goto error_free; This bypasses the call to amdgpu_vm_pt_free_list() which would normally process and free the list. When the function returns, its stack frame is reclaimed, but the amdgpu_vm_bo_base entries in the software tree still have their vm_status list nodes pointing to this reclaimed stack memory. When the VM is later destroyed, amdgpu_vm_pt_free_root() traverses the tree and calls list_del() on these entries. Could this write to the dangling stack pointers and cause arbitrary stack corruption? -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
