Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Critical] A concurrent device unbind and GEM object destruction results in a use-after-free and double-free when SGTableMap::drop accesses the freed GEM object. - [High] SGTableMap::drop unconditionally calls __drm_gem_shmem_free_sgt_locked(), causing NULL pointer dereferences for purged objects, memory corruption for PRIME-imported objects, and leaked page references. --
commit 8c8b7df9b9589f955da172bd30324598c3ed5764 Author: Lyude Paul <[email protected]> rust: drm: gem: Introduce shmem::Object::sg_table() In order to do this, we need to be careful to ensure that any interface we expose for scatterlists ensures that any mappings created from one are destroyed on driver-unbind. To do this, we introduce a Devres resource into shmem::Object that we use in order to ensure that we release any SGTable mappings on driver-unbind. > diff --git a/rust/kernel/drm/gem/shmem.rs b/rust/kernel/drm/gem/shmem.rs > index d4afc06d1be50..f44665cc24119 100644 > --- a/rust/kernel/drm/gem/shmem.rs > +++ b/rust/kernel/drm/gem/shmem.rs [ ... ] > @@ -488,6 +560,64 @@ unsafe fn io_write(&self, value: $ty, address: usize) { > #[cfg(CONFIG_64BIT)] > impl_vmap_io_capable!(u64); > > +/// A reference to a GEM object that is known to have a mapped [`SGTable`]. > +/// > +/// This is used by the Rust bindings with [`Devres`] in order to ensure > that mappings for SGTables > +/// on GEM shmem objects are revoked on driver-unbind. > +/// > +/// # Invariants > +/// > +/// - `self.obj` always points to a valid GEM object. > +/// - This object is proof that `self.obj.owner.sgt_res` has an initialized > and valid pointer to an > +/// [`SGTable`]. > +/// > +/// [`SGTable`]: scatterlist::SGTable > +pub struct SGTableMap<T: DriverObject, C: DeviceContext> { > + obj: NonNull<Object<T, C>>, > +} > + > +impl<T: DriverObject, C: DeviceContext> Deref for SGTableMap<T, C> { > + type Target = scatterlist::SGTable; > + > + fn deref(&self) -> &Self::Target { > + // SAFETY: > + // - The NonNull is guaranteed to be valid via our type invariants. > + // - The sgt field is guaranteed to be initialized and valid via our > type invariants. > + unsafe { > scatterlist::SGTable::from_raw((*self.obj.as_ref().as_raw_shmem()).sgt) } > + } > +} > + > +impl<T: DriverObject, C: DeviceContext> Drop for SGTableMap<T, C> { > + fn drop(&mut self) { > + // SAFETY: `obj` is always valid via our type invariants > + let obj = unsafe { self.obj.as_ref() }; > + let _lock = DmaResvGuard::new(obj); [Severity: Critical] Can a concurrent device unbind and GEM object destruction result in a use-after-free here? When a GEM object's final reference is dropped, free_callback() is executed and it drops the SetOnce wrapper: unsafe { ManuallyDrop::drop(&mut (*this).sgt_res) }; This invokes Devres::drop(), which calls revoke_nosync(). If the device is concurrently unbound, the devres unbind thread will call revoke() on the Devres object, setting its availability flag to false and waiting via synchronize_rcu() before dropping SGTableMap. Because revoke_nosync() on the free_callback thread sees the flag is already false, it returns immediately without synchronizing. free_callback() then proceeds to free the GEM object memory via drm_gem_shmem_release(). Once synchronize_rcu() finishes in the unbind thread, this SGTableMap::drop runs and dereferences the now-freed self.obj pointer, resulting in a use-after-free. > + > + // SAFETY: We acquired the lock needed for calling this function > above > + unsafe { > bindings::__drm_gem_shmem_free_sgt_locked(obj.as_raw_shmem()) }; [Severity: High] Does unconditionally calling __drm_gem_shmem_free_sgt_locked() handle purged or PRIME-imported objects correctly? If a Rust Object is PRIME-imported, shmem->sgt points to an SGT owned by the exporting driver, and calling this could free and corrupt the exporter's memory. Additionally, if the object was purged by the shrinker, shmem->sgt is freed and set to NULL. Calling __drm_gem_shmem_free_sgt_locked() does not check for NULL, which could lead to a NULL pointer dereference in dma_unmap_sgtable(). Finally, for native objects, drm_gem_shmem_get_pages_sgt() pins the pages and increments pages_use_count. By only freeing the SGT and skipping the required drm_gem_shmem_put_pages_locked() call, does this leak page references until the object's file descriptor is closed? > + } > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
