I think these are (almost) all false positives?
On Mon, 2026-06-08 at 18:48 +0000, [email protected] wrote:
> 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.
This doesn't make sense - `sgt_res` doesn't get cleared on driver
unbind, the Devres simply just no longer can be used to access its
contents.
>
> 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.
I don't think the drop order here matters?
>
> > +
> > + // 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?
Driver "rebind" is definitely not something I've ever heard before, is
that even possible for a driver to do?
>
> 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.
I don't think this is correct either, but I'm a bit stumped on this
one? Or if it is, it's not a bug in this patch. It looks like it's
pulling tokens from other functions that look like they have similar
purposes (drm_gem_shmem_prime_import_no_map(), or
drm_gem_shmem_prime_import_sg_table()) but I can't tell.
>
> [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.
I think this one may actually be correct, but it's not about this
patch.
>
> > +
> > +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.
Looks correct, but not our bug again.
