Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] `map_bo_range` lacks a bounds check against the VM's virtual address range, allowing out-of-bounds MMU update commands to be sent to the hardware. - [High] Partial unmap of 2MB page table blocks causes the ARM LPAE io-pgtable backend to reject the operation, breaking drm_gpuvm unmap and remap. --
--- Patch [4]: [PATCH v10 4/7] drm/tyr: add GPU virtual memory (VM) support --- commit 7f9a396490c23472f40e677be97d921859c50864 Author: Boris Brezillon <[email protected]> drm/tyr: add GPU virtual memory (VM) support Add GPU virtual address space management using the DRM GPUVM framework. Each virtual memory (VM) space is backed by ARM64 LPAE Stage 1 page tables and can be mapped into hardware address space (AS) slots for GPU execution. The implementation provides memory isolation and virtual address allocation. VMs support mapping GEM buffer objects with configurable protection flags (readonly, noexec, uncached) and handle both 4KB and 2MB page sizes. A new_dummy_object() helper is provided to create a dummy GEM object for use as a GPUVM root. > diff --git a/drivers/gpu/drm/tyr/vm.rs b/drivers/gpu/drm/tyr/vm.rs > new file mode 100644 > --- /dev/null > +++ b/drivers/gpu/drm/tyr/vm.rs [ ... ] > + pub(crate) fn map_bo_range( > + &self, > + bo: &Bo, > + bo_offset: u64, > + map_size: u64, > + va: u64, > + flags: VmMapFlags, > + ) -> Result { [ ... ] > + let bo_end = bo_offset.checked_add(map_size).ok_or(EINVAL)?; > + > + if bo_end > bo_size { > + dev_err!( > + self.dev, > + "BO mapping range {:#x}..{:#x} exceeds BO size {:#x}", > + bo_offset, > + bo_end, > + bo_size > + ); > + return Err(EINVAL); > + } > + > + let va_end: u64 = va.checked_add(map_size).ok_or(EINVAL)?; > + > + let req = VmOpRequest { [Severity: High] Does this code need to check if the calculated va_end exceeds the VM's virtual address range? If a user-space caller provides a very large map_size, va + map_size could exceed the VM's valid range. Because there's no bounds check against self.va_range.end here (unlike in unmap_range()), the out-of-bounds region will be passed to PtUpdateContext::new(). This might issue a hardware lock command to the MMU before drm_gpuvm rejects the mapping. Could this lead to unhandled hardware faults or device hangs? > + op_type: VmOpType::Map(VmMapArgs { > + vm_bo: self.gpuvm.obtain(bo, ())?, > + flags, > + bo_offset, > + }), > + region: va..va_end, > + }; [ ... ] > + fn sm_step_remap<'op>( > + &mut self, > + op: OpRemap<'op, Self>, > + context: &mut Self::SmContext<'_>, > + ) -> Result<OpRemapped<'op, Self>, Error> { [ ... ] > + let unmap_length = unmap_end - unmap_start; > + > + if unmap_length > 0 { > + let region = unmap_start..(unmap_start + unmap_length); > + pt_unmap(context.dev, context.pt, > region.clone()).inspect_err(|e| { [Severity: High] Will this fail if the region is a partial unmap of a 2MB page table block? If a buffer object is naturally aligned to 2MB, pt_map() may opportunistically use 2MB block descriptors. If a caller later attempts to unmap a 4KB sub-region, sm_step_remap() will pass this 4KB region to pt_unmap(). The ARM LPAE io-pgtable backend (arm_lpae_unmap_pages) does not automatically split huge pages, so it might encounter the 2MB leaf descriptor, reject the operation, and return 0, causing pt_unmap() to return EINVAL. Could this leave the DRM GPUVM tree out of sync with the hardware page tables and leak the mapping in the GPU? > + dev_err!( > + context.dev, > + "Failed to unmap remap region {:#x}..{:#x}: {:?}", > + region.start, > + region.end, > + e > + ); > + })?; > + } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
