Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [Critical] Casting 64-bit physical and virtual addresses to `usize` causes silent truncation on 32-bit platforms with LPAE, leading to incorrect mappings and memory corruption. - [High] Insufficient `GpuVaAlloc` preallocation causes map/unmap operations to fail midway with `EINVAL`, leaving hardware page tables out of sync with the GPUVM tree. - [High] Unmapping regions backed by non-2MB-aligned physical addresses fails and leaks the mappings because `pt_unmap` incorrectly assumes the mapped page size is 2MB. - [High] Missing bounds check on GEM object size in `map_bo_range` allows creating overgrown mappings, leading to permanent memory leaks and GPUVM tree corruption. - [Medium] Successfully mapped pages during a failing `pt.map_pages` call are not unmapped, permanently leaking page table entries. --
--- Patch [4]: [PATCH v7 4/7] drm/tyr: add GPU virtual memory (VM) support --- commit 809ab9f421f459ef56a480ccf227ec9341b24878 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 space is backed by ARM64 LPAE Stage 1 page tables and can be mapped into hardware address space slots for GPU execution. > diff --git a/drivers/gpu/drm/tyr/vm.rs b/drivers/gpu/drm/tyr/vm.rs > index 0000000000000..2358096afc7e8 100644 > --- /dev/null > +++ b/drivers/gpu/drm/tyr/vm.rs [ ... ] > pub(crate) fn map_bo_range( > &self, > bo: &Bo, > bo_offset: u64, > size: u64, > va: u64, > flags: VmMapFlags, > ) -> Result { > let req = VmOpRequest { > op_type: VmOpType::Map(VmMapArgs { > vm_bo: self.gpuvm.obtain(bo, ())?, > flags, > bo_offset, > }), > region: va..(va + size), > }; [Severity: High] Does this code allow mapping a region larger than the actual GEM object? If bo_offset + size exceeds the object's size, sm_step_map() will finish when the scatter-gather table is exhausted but register a GPUVA node for the full oversized range. When this oversized virtual address range is later unmapped, the driver might attempt to unmap page table entries that were never populated, which can cause pt.unmap_pages() to return 0 and abort the unmap operation. > let mut resources = VmOpResources { > preallocated_gpuvas: [ > Some(GpuVaAlloc::<GpuVmData<'bound>>::new(GFP_KERNEL)?), > Some(GpuVaAlloc::<GpuVmData<'bound>>::new(GFP_KERNEL)?), > Some(GpuVaAlloc::<GpuVmData<'bound>>::new(GFP_KERNEL)?), > ], > }; [Severity: High] Is preallocating 3 VAs sufficient for a map operation? If the new mapping overlaps and splits two existing mappings, it can trigger two remaps. A single remap consumes 2 preallocated VAs, plus 1 for the new mapping, requiring up to 5 VAs in total. Can this cause the map operation to fail midway with EINVAL and leave the software GPUVM tracking tree out of sync with the hardware page tables? [ ... ] > pub(crate) fn unmap_range(&self, va: u64, size: u64) -> Result { > let req = VmOpRequest { > op_type: VmOpType::Unmap, > region: va..(va + size), > }; > let mut resources = VmOpResources { > preallocated_gpuvas: [ > Some(GpuVaAlloc::<GpuVmData<'bound>>::new(GFP_KERNEL)?), > Some(GpuVaAlloc::<GpuVmData<'bound>>::new(GFP_KERNEL)?), > None, > ], > }; [Severity: High] Will this array have enough VAs for a complex unmap? An unmap operation that splits two existing mappings could require up to 4 VAs for the leftover segments, but this initialization only allocates 2. [ ... ] > fn pt_map( > pt: &IoPageTable<'_, ARM64LPAES1>, > iova: u64, > paddr: u64, > len: u64, > prot: u32, > ) -> Result<u64> { [ ... ] > let (mapped, result) = unsafe { > pt.map_pages( > curr_iova as usize, > (curr_paddr as usize).try_into().unwrap(), > pgsize as usize, > pgcount as usize, > prot, > GFP_KERNEL, > ) > }; [Severity: Critical] Does casting curr_paddr and curr_iova to usize truncate 64-bit addresses on 32-bit platforms with LPAE? This could cause incorrect physical pages to be mapped into the GPU's address space, potentially leading to unauthorized memory access or data corruption. > if let Err(e) = result { > pr_err!("pt.map_pages failed at iova {:#x}: {:?}\n", curr_iova, > e); > if segment_mapped > 0 { > let _ = pt_unmap(pt, iova..(iova + segment_mapped)); > } > return Err(e); > } [Severity: Medium] If pt.map_pages() succeeds in mapping some pages but returns an error for a subsequent page, does this error path leak the pages that were just successfully mapped? The cleanup unmaps segment_mapped (the bytes mapped in previous loop iterations) but doesn't appear to include the mapped bytes returned by the failing pt.map_pages() call. [ ... ] > fn pt_unmap(pt: &IoPageTable<'_, ARM64LPAES1>, range: Range<u64>) -> Result { > let mut iova = range.start; > let mut bytes_left_to_unmap = range.end - range.start; > > while bytes_left_to_unmap > 0 { > let (pgsize, pgcount) = get_pgsize(iova, bytes_left_to_unmap); > > let unmapped = unsafe { pt.unmap_pages(iova as usize, pgsize as > usize, pgcount as usize) }; [Severity: High] Can this fail to unmap regions if they were originally backed by physical addresses that were not 2MB-aligned? In pt_map(), the page size is calculated using both virtual and physical addresses: get_pgsize(curr_iova | curr_paddr, remaining); However, pt_unmap() calculates the page size using only the virtual address: get_pgsize(iova, bytes_left_to_unmap); If a user maps a physical address that is not 2MB-aligned into a virtual address that is 2MB-aligned, it appears pt_map() will fall back to 4KB pages, but pt_unmap() will attempt to unmap using 2MB blocks. [Severity: Critical] Does casting iova to usize here also cause silent truncation on 32-bit platforms? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
