Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [Critical] `SGTableMap::drop()` unconditionally frees the scatter/gather table, causing a double-free and memory corruption for imported GEM objects whose SGT is owned by the DMA-BUF exporter. - [Critical] Caching `Devres` in a `SetOnce` field on a reference-counted GEM object causes a Use-After-Free if the driver is unbound and rebound. - [Critical] Concurrent execution of device unbind (`devres_node_release`) and GEM object freeing (`free_callback`) causes a fatal Use-After-Free due to missing synchronization. - [High] SGTableMap fails to pin the GEM object, allowing the shrinker to purge the object and free the sg_table out from under the Rust driver, leading to a UAF and NULL pointer dereference. - [High] `SGTableMap::drop` destroys the SGT but fails to release the backing pages reference, permanently leaking the memory and triggering a kernel warning on rebinds. --
commit ca164753ace4f901b6f5bd1ab132aa0d035c5a2b Author: Lyude Paul <[email protected]> rust: drm: gem: Introduce shmem::Object::sg_table() This patch introduces sg_table() to shmem::Object, utilizing Devres to ensure SGTable mappings are released on driver-unbind. > diff --git a/rust/kernel/drm/gem/shmem.rs b/rust/kernel/drm/gem/shmem.rs > index e0bd0695ba1f8..ae97498500afa 100644 > --- a/rust/kernel/drm/gem/shmem.rs > +++ b/rust/kernel/drm/gem/shmem.rs [ ... ] > @@ -183,18 +207,26 @@ extern "C" fn free_callback(obj: *mut > bindings::drm_gem_object) { > // - DRM always passes a valid gem object here > // - We used drm_gem_shmem_create() in our create_gem_object > callback, so we know that > // `obj` is contained within a drm_gem_shmem_object > - let this = unsafe { container_of!(obj, > bindings::drm_gem_shmem_object, base) }; > - > - // SAFETY: > - // - We're in free_callback - so this function is safe to call. > - // - We won't be using the gem resources on `this` after this call. > - unsafe { bindings::drm_gem_shmem_release(this) }; > + let base = unsafe { container_of!(obj, > bindings::drm_gem_shmem_object, base) }; > > // SAFETY: > // - We verified above that `obj` is valid, which makes `this` valid > // - This function is set in AllocOps, so we know that `this` is > contained within a > // `Object<T, C>` > - let this = unsafe { container_of!(Opaque::cast_from(this), Self, > obj) }.cast_mut(); > + let this = unsafe { container_of!(Opaque::cast_from(base), Self, > obj) }.cast_mut(); > + > + // We need to drop `sgt_res` first, since doing so requires that the > GEM object is still > + // alive. > + // SAFETY: > + // - We verified above that `this` is valid. > + // - We are in free_callback, guaranteeing we have exclusive access > to `this` and that > + // `sgt_res` will not be used after dropping it here. > + unsafe { ManuallyDrop::drop(&mut (*this).sgt_res) }; [Severity: Critical] Could this cause a race condition with a concurrent device unbind? If the drop races with devres_node_release, Devres::drop's internal atomic revoke might return false, but free_callback proceeds to free the Rust Object memory without blocking. When the concurrent unbind thread continues and invokes SGTableMap::drop, it dereferences self.obj to acquire the dma_resv lock, which would access already freed memory. > + > + // SAFETY: > + // - We're in free_callback - so this function is safe to call. > + // - We won't be using the gem resources on `this` after this call. > + unsafe { bindings::drm_gem_shmem_release(base) }; > > // SAFETY: We're recovering the Kbox<> we created in > gem_create_object() > let _ = unsafe { KBox::from_raw(this) }; [ ... ] > @@ -270,6 +302,46 @@ pub fn vmap<const SIZE: usize>(&self) -> > Result<VMapRef<'_, T, C, SIZE>> { > pub fn owned_vmap<const SIZE: usize>(&self) -> Result<VMapOwned<T, C, > SIZE>> { > self.make_vmap() > } > + > + /// Creates (if necessary) and returns an immutable reference to a > scatter-gather table of DMA > + /// pages for this object. > + /// > + /// This will pin the object in memory. It is expected that `dev` should > be a pointer to the > + /// same [`device::Device`] which `self` belongs to, otherwise this > function will return > + /// `Err(EINVAL)`. > + pub fn sg_table<'a>( > + &'a self, > + dev: &'a device::Device<Bound>, > + ) -> Result<&'a scatterlist::SGTable> { > + if dev.as_raw() != self.dev().as_ref().as_raw() { > + return Err(EINVAL); > + } > + > + let sgt_res = 'out: { > + // Fast path: sgt_res is already initialized > + if let Some(sgt_res) = self.sgt_res.as_ref() { > + break 'out sgt_res; > + } > + > + // Slow path: Grab the lock and see if we need to initialize > sgt_res. > + let _guard = self.sgt_lock.lock(); > + > + // If someone initialized it while we were waiting, we can exit > early. > + if let Some(sgt_res) = self.sgt_res.as_ref() { > + break 'out sgt_res; > + } > + > + // If not, finish initializing and return. `populate()` cannot > return false, as > + // `sgt_res` must be unpopulated, and we must hold `sgt_lock` to > reach this point. > + self.sgt_res > + .populate(Devres::new(dev, SGTableMap::new(self))?); > + > + // SAFETY: We just populated sgt_res above. > + unsafe { self.sgt_res.as_ref().unwrap_unchecked() } > + }; > + > + Ok(sgt_res.access(dev)?) > + } [Severity: Critical] Does caching Devres in a SetOnce field on a reference-counted GEM object cause a use-after-free on driver rebind? On device unbind, the Devres is revoked and its internal SGTableMap is dropped, clearing the SGT. Since the GEM object outlives the binding and SetOnce is never cleared, a subsequent rebind with the same device pointer would hit the fast path here. The call to access() bypasses revocation checks, returning a reference to the dropped SGTableMap and accessing a NULL shmem->sgt. > } > > impl<T: DriverObject, C: DeviceContext> Deref for Object<T, C> { [ ... ] > @@ -468,6 +540,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); > + > + // SAFETY: We acquired the lock needed for calling this function > above > + unsafe { > bindings::__drm_gem_shmem_free_sgt_locked(obj.as_raw_shmem()) }; > + } > +} [Severity: Critical] Is it safe to unconditionally free the SGT here? For imported DMA-BUF objects, the SGT is owned by the exporter and must be freed via dma_buf_unmap_attachment. Calling __drm_gem_shmem_free_sgt_locked on an imported object frees the exporter's table, leading to memory corruption and a double-free when the DMA-BUF is eventually destroyed. [Severity: High] Does this leak the backing pages reference? SGTableMap::new calls drm_gem_shmem_get_pages_sgt, which increments pages_use_count. When this drop destroys the SGT, it omits the paired call to drm_gem_shmem_put_pages_locked. If a driver unbinds and rebinds, this will leak the extra reference and trigger a kernel warning. > + > +impl<T: DriverObject, C: DeviceContext> SGTableMap<T, C> { > + fn new(obj: &Object<T, C>) -> impl Init<Self, Error> { > + // INVARIANT: > + // - We call drm_gem_shmem_get_pages_sgt below and check whether or > not it succeeds, > + // fulfilling the invariant of SGTableMap that the object's `sgt` > field is initialized. > + // SAFETY: > + // - `obj` is fully initialized, making this function safe to call. > + from_err_ptr(unsafe { > bindings::drm_gem_shmem_get_pages_sgt(obj.as_raw_shmem()) })?; > + > + Ok(Self { obj: obj.into() }) > + } > +} [Severity: High] Should this pin the GEM object? The documentation for sg_table mentions it pins the object, but drm_gem_shmem_get_pages_sgt only increments pages_use_count, not pages_pin_count. If userspace calls madvise(DONTNEED), the shrinker could purge the object, freeing shmem->sgt. When SGTableMap is dropped, passing the object to __drm_gem_shmem_free_sgt_locked will dereference a NULL pointer. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
