Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] `SGTableMap::drop()` unconditionally calls `__drm_gem_shmem_free_sgt_locked()`, which improperly frees the `sg_table` of imported DMA-BUF objects using `kfree()` instead of returning it to the exporter. - [High] `Object::sg_table()` does not pin the GEM object, allowing the shrinker to purge it and free the scatter-gather table while Rust still holds references to it. - [High] Concurrent execution of device unbind and GEM object free causes a Use-After-Free of the GEM object inside `SGTableMap::drop`. --
commit 91daf37fb73a3ed3254590bfb60a617c06f3950d Author: Lyude Paul <[email protected]> Subject: rust: drm: gem: Introduce shmem::Object::sg_table() This commit introduces a devres-managed SGTable mapping for GEM shmem objects in Rust, ensuring mappings are released on driver unbind. > diff --git a/rust/kernel/drm/gem/shmem.rs b/rust/kernel/drm/gem/shmem.rs > --- 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) { > // - 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) }; > + > // 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) }; > + unsafe { bindings::drm_gem_shmem_release(base) }; [Severity: High] Can this sequence cause a use-after-free if device unbind runs concurrently with the last reference drop? If the device unbind thread changes the Devres state to revoked and waits in rcu::synchronize_rcu(), and free_callback() runs concurrently: free_callback() ManuallyDrop::drop(&mut (*this).sgt_res) Because the state is already revoked, will Devres::drop() return immediately without waiting for the unbind thread? free_callback() would then proceed to call drm_gem_shmem_release() and free the GEM object memory. When the unbind thread finishes synchronize_rcu(), it will call SGTableMap::drop() which dereferences the now-freed GEM object: SGTableMap::drop() let obj = unsafe { self.obj.as_ref() }; [ ... ] > +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: High] Does this unconditionally free the scatter-gather table, even for imported DMA-BUF objects? The Rust GEM shmem abstraction supports imported DMA-BUF objects. For these objects, the scatter-gather table is allocated and mapped by the exporter. Calling __drm_gem_shmem_free_sgt_locked() here will unmap and call kfree() on a pointer not allocated by this driver. Could this corrupt the exporter's heap? > + } > +} [ ... ] > +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()) })?; [Severity: High] Is the GEM object properly pinned here? The documentation for Object::sg_table() states "This will pin the object in memory." However, drm_gem_shmem_get_pages_sgt() increments pages_use_count but does not increment pages_pin_count. If the user marks the object with MADV_DONTNEED, could the shrinker purge the object, freeing the scatter-gather table while Rust still hands out references to it? This might also lead to a NULL pointer dereference later in SGTableMap::drop() when it passes the freed table to __drm_gem_shmem_free_sgt_locked(). > + > + Ok(Self { obj: obj.into() }) > + } > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
