Re: [PATCH v20 4/4] rust: drm: gem: Introduce shmem::Object::sg_table()

[sashiko-bot@kernel.org]

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 <lyude@redhat.com>

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/20260610162433.923550-1-lyude@redhat.com?part=4