Re: [PATCH v15 6/6] 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] Eager evaluation of `Devres::new` and `SGTableMap::new` inside `sg_table()` causes a use-after-free of the scatterlist and leaking of page references.
- [High] Concurrent device unbind and GEM object free causes Use-After-Free of the GEM object in `SGTableMap::drop`.
- [Low] The patch imports and uses `LazyInit`, `new_lazy_init`, and `LazyInitError` from `crate::sync`, but these are neither defined nor exported anywhere in the tree.
--

commit 2d2d3346ec26b2ca8eba7a05f0577c63628921d0
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 494e0d0d8d0d6..5655c2a1ae8fb 100644
> --- a/rust/kernel/drm/gem/shmem.rs
> +++ b/rust/kernel/drm/gem/shmem.rs

[ ... ]

>      sync::{
>          aref::ARef,
>          new_lazy_init,
>          LazyInit,
>          LazyInitError, //
>      },

[Severity: Low]
Are these primitives defined somewhere else?  The patch imports and uses
LazyInit, new_lazy_init, and LazyInitError from crate::sync, but these don't
seem to be present in the tree or introduced anywhere in this patch series.
Will this lead to a module resolution failure and compilation error?

[ ... ]

>      extern "C" fn free_callback(obj: *mut bindings::drm_gem_object) {
>          // SAFETY:
>          // - 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 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>`
>          let this = unsafe { container_of!(Opaque::cast_from(base), Self, obj) }.cast_mut();
>  
>          // We need to reset `sgt_res` first, since doing so requires that the GEM object is still
>          // alive.
>          // SAFETY:
>          // - We verified above that `this` is valid.
>          // - `&mut (*this)` does not implement Unpin.
>          unsafe { Pin::new_unchecked(&mut (*this).sgt_res) }.reset();

[Severity: High]
Does this code introduce a race condition causing a use-after-free?

If drm_gem_object_put() triggers free_callback() concurrently with a device
unbind, the device unbind thread's devres release callback calls
Revocable::revoke().  This swaps is_available to false and blocks on
synchronize_rcu() before dropping SGTableMap.

Concurrently, free_callback() calls sgt_res.reset(), which drops Devres.
Devres::drop() calls revoke_nosync(), which sees is_available is already false
and returns immediately without waiting for the unbind thread.

Then free_callback() proceeds to call drm_gem_shmem_release() and frees the GEM
object memory.

When the unbind thread finishes synchronize_rcu() and executes
SGTableMap::drop(), could it dereference the freed GEM object memory
to lock its dma_resv?

>  
>          // 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) };

[ ... ]

>      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);
>          }
>  
>          match self.sgt_res.init(Devres::new(dev, SGTableMap::new(self))) {

[Severity: High]
Can eager evaluation cause a use-after-free and leaked page references here?

In Rust, function arguments are evaluated eagerly.  Devres::new allocates and
executes its initializer eagerly.  SGTableMap::new(self) evaluates its
initializer, causing drm_gem_shmem_get_pages_sgt() to be called synchronously
every time sg_table() is executed, incrementing the page use count.

If sgt_res is already initialized, init() returns LazyInitError::AlreadyInit
and drops the newly created Devres argument.

When Devres is dropped, it drops the inner SGTableMap, calling
SGTableMap::drop(). This calls __drm_gem_shmem_free_sgt_locked(), which
unconditionally unmaps and frees the shared shmem->sgt memory and sets it to
NULL without decrementing the page use count.

Does this leave the originally cached SGTableMap holding a reference to a NULL
or freed shmem->sgt, which would cause a NULL pointer dereference or
use-after-free the next time it's accessed?

>              Ok(ret) | Err(LazyInitError::AlreadyInit(ret)) => Ok(ret.access(dev)?),
>              Err(LazyInitError::DuringInit(e)) => Err(e),
>          }
>      }
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260529183702.677677-1-lyude@redhat.com?part=6