Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [Critical] Unhandled `unmap_range` error during `KernelBo` drop leads to dangling page table entries and use-after-free. - [High] Silent truncation of `size` to `usize` on 32-bit architectures leads to mismatched buffer allocation and VM mapping. - [Medium] The virtual address range calculation `va + size` can overflow `u64`, causing a kernel panic or invalid state. --
--- Patch [5]: [PATCH v7 5/7] drm/tyr: add a kernel buffer object --- commit 63ba028b17f1b868f89f67c9bd905f6f1b2dc8cd Author: Deborah Brouwer <[email protected]> drm/tyr: add a kernel buffer object Introduce a buffer object type (KernelBo) for internal driver allocations that are managed by the kernel rather than userspace. KernelBo wraps a GEM shmem object and automatically handles GPU virtual address space mapping during creation and unmapping on drop. This provides a safe and convenient way for the driver to both allocate and clean up internal buffers for kernel-managed resources. > diff --git a/drivers/gpu/drm/tyr/gem.rs b/drivers/gpu/drm/tyr/gem.rs > index c28be61a01bb2..47a05a33388e3 100644 > --- a/drivers/gpu/drm/tyr/gem.rs > +++ b/drivers/gpu/drm/tyr/gem.rs [ ... ] > @@ -56,3 +67,85 @@ pub(crate) fn new_dummy_object(ddev: &TyrDrmDevice) -> > Result<ARef<Bo>> { > > Ok(bo) > } > + > +/// Specifies how to choose a GPU virtual address for a [`KernelBo`]. > +/// An automatic VA allocation strategy will be added in the future. > +pub(crate) enum KernelBoVaAlloc { > + /// Explicit VA address specified by the caller. > + #[expect(dead_code)] > + Explicit(u64), > +} > + > +/// A kernel-owned buffer object with automatic GPU virtual address mapping. > +/// > +/// This structure represents a buffer object that is created and managed > entirely > +/// by the kernel driver, as opposed to userspace-created GEM objects. It > combines > +/// a GEM object with automatic GPU virtual address (VA) space mapping and > cleanup. > +/// > +/// When dropped, the buffer is automatically unmapped from the GPU VA space. > +pub(crate) struct KernelBo<'bound> { [ ... ] > +impl<'bound> KernelBo<'bound> { > + /// Creates a new kernel-owned buffer object and maps it into GPU VA > space. > + /// > + /// This function allocates a new shmem-backed GEM object and > immediately maps > + /// it into the specified GPU virtual memory space. The mapping is > automatically > + /// cleaned up when the [`KernelBo`] is dropped. > + #[expect(dead_code)] > + pub(crate) fn new( > + ddev: &TyrDrmDevice, > + vm: Arc<Vm<'bound>>, > + size: u64, > + va_alloc: KernelBoVaAlloc, > + flags: VmMapFlags, > + ) -> Result<Self> { > + if size == 0 { > + pr_err!("Cannot create KernelBo with size 0\n"); > + return Err(EINVAL); > + } > + > + let KernelBoVaAlloc::Explicit(va) = va_alloc; > + > + let bo = Bo::new( > + ddev, > + size as usize, [Severity: High] Does this silently truncate the allocation size on 32-bit architectures? If a size larger than 4GB is passed, it appears Bo::new() will allocate the truncated size, but vm.map_bo_range() will be called with the full u64 size below. Could this lead to out-of-bounds GPU virtual memory mappings if the mapping size isn't strictly validated against the actual allocation? > + shmem::ObjectConfig { > + map_wc: true, > + parent_resv_obj: None, > + }, > + BoCreateArgs { flags: 0 }, > + )?; > + > + vm.map_bo_range(&bo, 0, size, va, flags)?; > + > + Ok(KernelBo { > + bo, > + vm, > + va_range: va..(va + size), [Severity: Medium] Could this addition overflow u64? If va + size exceeds u64::MAX, it will either panic or wrap around. If it wraps, va_range.end will be smaller than va_range.start, which could later cause an underflow panic during drop when calculating self.va_range.end - self.va_range.start. > + }) > + } > +} > + > +impl Drop for KernelBo<'_> { > + fn drop(&mut self) { > + let va = self.va_range.start; > + let size = self.va_range.end - self.va_range.start; > + > + if let Err(e) = self.vm.unmap_range(va, size) { [Severity: Critical] Is it safe to continue dropping the buffer object if unmap_range() fails? If unmap_range() fails (e.g., due to OOM when splitting page tables or a hardware timeout), the error is only logged. The underlying self.bo will still be dropped, potentially freeing its physical pages back to the system. Since the unmap failed, the GPU's page tables might still contain valid entries pointing to those freed pages. Could this allow the GPU to access memory reallocated to other kernel subsystems or userspace, causing a use-after-free? > + pr_err!( > + "Failed to unmap KernelBo range {:#x}..{:#x}: {:?}\n", > + self.va_range.start, > + self.va_range.end, > + e > + ); > + } > + } > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=5
