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

[sashiko-bot@kernel.org]

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