Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] The `unmap_range` function evaluates `va + size` directly without overflow checks, which will panic and cause a Denial of Service if large values are provided. - [High] Vm::kill can fail to unmap the VM address space if memory allocation fails, leading to permanent resource leaks. - [Medium] Early returns on error paths in map_bo_range and unmap_range skip deferred_cleanup(), leaking drm_gpuva objects queued by successful partial steps. - [Low] The condition `if gem_offset != 0` inside the SG table iteration loop in `sm_step_map` is mathematically unreachable. --
--- Patch [4]: [PATCH v8 4/7] drm/tyr: add GPU virtual memory (VM) support --- commit e1f0e3f5135d057c6b2d458da0a1a699b4be92cf 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, virtual address allocation, and supports mapping GEM buffer objects with configurable protection flags. > diff --git a/drivers/gpu/drm/tyr/vm.rs b/drivers/gpu/drm/tyr/vm.rs > new file mode 100644 > index 0000000000000..5431b04a72795 > --- /dev/null > +++ b/drivers/gpu/drm/tyr/vm.rs [ ... ] > + pub(crate) fn kill(&self) { > + // TODO: Turn the VM into a state where it can't be used. > + let _ = self.deactivate(); > + let _ = self > + .unmap_range(self.va_range.start, self.va_range.end - > self.va_range.start) [Severity: High] Can this leak memory permanently if the system is under memory pressure? In Vm::kill(), we are ignoring the result of unmap_range(). However, unmap_range() unconditionally allocates GpuVaAlloc objects with GFP_KERNEL. If a process is being out-of-memory killed or the system is under heavy memory pressure, these allocations can fail, causing unmap_range() to abort. If this fails during teardown, will the GPUVM tree remain populated and cause drm_gpuvm_destroy() to leak the mappings when the VM is dropped? > + .inspect_err(|e| { > + dev_err!( > + self.dev, > + "Failed to unmap range during deactivate: {:?}\n", > + e > + ); > + }); > + } [ ... ] > + pub(crate) fn map_bo_range( [ ... ] > + { > + let mut gpuvm_unique = self.gpuvm_unique.lock(); > + self.exec_op(gpuvm_unique.as_mut().get_mut(), req, &mut > resources)?; [Severity: Medium] Does this leak drm_gpuva objects if exec_op() returns an error? If an earlier step in the drm_gpuvm_sm_map state machine (like unmapping an overlap) succeeds, it queues the old objects for deferred cleanup. However, the ? operator here causes an early return if a subsequent step fails. This early return bypasses the self.gpuvm.deferred_cleanup() call below, leaving the queued objects in the deferred list. Could a Drop guard or similar mechanism be used to ensure deferred cleanup happens even on error paths? > + } > + // We flush the defer cleanup list now. Things will be different in > + // the asynchronous VM_BIND path, where we want the cleanup to > + // happen outside the DMA signalling path. > + self.gpuvm.deferred_cleanup(); > + Ok(()) > + } > + > + /// Unmaps a virtual address range from the VM. > + /// > + /// This removes any existing mappings in the specified range, freeing > the > + /// virtual address space for reuse. > + pub(crate) fn unmap_range(&self, va: u64, size: u64) -> Result { > + let req = VmOpRequest { > + op_type: VmOpType::Unmap, > + region: va..(va + size), [Severity: High] Can this overflow and panic if large values are provided for va and size? The addition va + size is evaluated directly here. If an attacker supplies large values (e.g., va = u64::MAX - 10), this will overflow. In Rust, this causes a panic (leading to a denial of service) or a corrupted range underflow if compiled without overflow checks. Should this use va.checked_add(size) as is correctly done in map_bo_range()? > + }; > + 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)?), > + ], > + }; > + { > + let mut gpuvm_unique = self.gpuvm_unique.lock(); > + self.exec_op(gpuvm_unique.as_mut().get_mut(), req, &mut > resources)?; > + } [ ... ] > + fn sm_step_map<'op>( [ ... ] > + for sgt_entry in sgt.iter() { > + // Expressly convert to u64 to work with arm 32-bit builds. > + #[allow(clippy::useless_conversion)] > + let mut paddr = u64::from(sgt_entry.dma_address()); > + #[allow(clippy::useless_conversion)] > + let mut sgt_entry_length = u64::from(sgt_entry.dma_len()); > + > + if bytes_left_to_map == 0 { > + break; > + } > + > + if gem_offset > 0 { > + // Skip the entire SGT entry if the gem_offset exceeds its > length. > + let skip = u64::min(sgt_entry_length, gem_offset); > + paddr += skip; > + sgt_entry_length -= skip; > + gem_offset -= skip; > + } > + > + if sgt_entry_length == 0 { > + continue; > + } > + > + if gem_offset != 0 { [Severity: Low] Is this condition mathematically reachable? Earlier in the loop, if gem_offset > 0, the code subtracts skip = u64::min(sgt_entry_length, gem_offset) from both variables. If gem_offset >= sgt_entry_length, sgt_entry_length becomes 0, and the loop continues before reaching this check. If gem_offset < sgt_entry_length, gem_offset becomes exactly 0. In all paths that reach this point, gem_offset seems guaranteed to be 0, making this check and its error return dead code. > + dev_err!( > + context.dev, > + "Invalid gem_offset {} in page table mapping.\n", > + gem_offset > + ); > + return Err(EINVAL); > + } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
