Re: [PATCH v15 3/6] rust: drm: gem: Add vmap functions to shmem bindings

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Critical] Calling `Object::sg_table()` multiple times sequentially or concurrently causes a Use-After-Free and Double Free of the GEM object's SG table due to eager evaluation.
- [High] `SGTableMap::drop` can access the `Object` after it has been freed if the device is unbound concurrently with the GEM object's destruction (Use-After-Free).
- [High] The `IoCapable` implementation for `VMap` uses non-volatile memory accesses, enabling dangerous compiler optimizations on memory shared with hardware.
- [Medium] Using uninterruptible lock `DmaResvGuard` for `vmap` operations.
- [Low] The patch introduces a call to an undefined C function `__drm_gem_shmem_free_sgt_locked`, breaking bisectability.
- [Low] Returning `ENOTSUPP` instead of `EOPNOTSUPP` for unsupported I/O memory allocations.
--

commit ff0ae9dbbc71a4912f8e6e6c487db7df45d250fb
Author: Lyude Paul <lyude@redhat.com>

rust: drm: gem: Add vmap functions to shmem bindings

This commit adds vmap bindings for gem shmem objects in Rust, introducing
owned and borrowed mappings similar to SGTables.

> diff --git a/rust/kernel/drm/gem/shmem.rs b/rust/kernel/drm/gem/shmem.rs
> index a116b0752e46a..494e0d0d8d0d6 100644
> --- a/rust/kernel/drm/gem/shmem.rs
> +++ b/rust/kernel/drm/gem/shmem.rs

[ ... ]

> @@ -170,22 +196,123 @@ 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>`
> -        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 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]
If the device is unbound concurrently with the GEM object's destruction, does
this reset() allow the devres callback to access the Object after it has
been freed? Since the devres callback might already be executing and has
extracted the value, reset() could complete without waiting, proceeding to
free the Object memory while SGTableMap::drop later dereferences the freed
pointer.

> +
> +        // 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) };
>      }
> +
> +    /// Attempt to create a vmap from the gem object, and confirm the size of said vmap.
> +    fn make_vmap<'a, R, const SIZE: usize>(&'a self) -> Result<VMap<T, R, SIZE>>
> +    where
> +        R: Deref<Target = Self> + From<&'a Self>,
> +    {
> +        // INVARIANT: We check here that the gem object is at least as large as `SIZE`.
> +        if self.size() < SIZE {
> +            return Err(ENOSPC);
> +        }
> +
> +        let mut map: MaybeUninit<bindings::iosys_map> = MaybeUninit::uninit();
> +        let guard = DmaResvGuard::new(self);

[Severity: Medium]
Is it safe to use the uninterruptible DmaResvGuard::new() here? This is a
user-triggerable path (creating a vmap), and acquiring the dma_resv lock
uninterruptibly might cause unkillable tasks during heavy lock contention.
The C helper drm_gem_shmem_vmap uses the interruptible version.

> +
> +        // SAFETY: drm_gem_shmem_vmap can be called with the DMA reservation lock held
> +        to_result(unsafe {
> +            bindings::drm_gem_shmem_vmap_locked(self.as_raw_shmem(), map.as_mut_ptr())
> +        })?;
> +
> +        // Drop the guard explicitly here, since we may need to call raw_vunmap() (which re-acquires
> +        // the lock).
> +        drop(guard);
> +
> +        // SAFETY: The call to drm_gem_shmem_vmap_locked succeeded above, so we are guaranteed that
> +        // map is properly initialized.
> +        let map = unsafe { map.assume_init() };
> +
> +        // XXX: We don't currently support iomem allocations
> +        if map.is_iomem {
> +            // SAFETY:
> +            // - The vmap operation above succeeded, guaranteeing that `map` points to a valid
> +            //   memory mapping.
> +            // - We checked that this is an iomem allocation, making it safe to read vaddr_iomem
> +            unsafe { self.raw_vunmap(map) };
> +
> +            Err(ENOTSUPP)

[Severity: Low]
Should this return EOPNOTSUPP instead? ENOTSUPP is an internal kernel
error code that shouldn't be returned to userspace, as it can cause
unexpected behavior in libc if propagated through an ioctl.

> +        } else {
> +            Ok(VMap {
> +                // SAFETY: We checked that this is not an iomem allocation, making it safe to read
> +                // vaddr
> +                addr: unsafe { map.__bindgen_anon_1.vaddr },
> +                owner: self.into(),
> +            })
> +        }
> +    }

[ ... ]

> +    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: Critical]
Could this lead to a double-free of the SG table? In Rust, function
arguments are evaluated eagerly. SGTableMap::new(self) executes
unconditionally before LazyInit::init checks if it is already initialized.

If sgt_res is already initialized, init() will return AlreadyInit, and the
newly instantiated SGTableMap argument will be dropped. The Drop impl for
SGTableMap unconditionally frees the shared shmem->sgt, which would leave
the first SGTableMap instance holding a dangling reference.

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

[ ... ]

> +macro_rules! impl_vmap_io_capable {
> +    ($impl:ident, $ty:ty) => {
> +        impl<D, R, const SIZE: usize> IoCapable<$ty> for $impl<D, R, SIZE>
> +        where
> +            D: DriverObject,
> +            R: Deref<Target = Object<D>>,
> +        {
> +            #[inline(always)]
> +            unsafe fn io_read(&self, address: usize) -> $ty {
> +                let ptr = address as *mut $ty;
> +
> +                // SAFETY: The safety contract of `io_read` guarantees that address is a valid
> +                // address within the bounds of `Self` of at least the size of $ty, and is properly
> +                // aligned.
> +                unsafe { ptr::read(ptr) }

[Severity: High]
Does using standard non-volatile ptr::read() enable dangerous compiler
optimizations here? The memory mapped via a GEM object is shared with
hardware and is often mapped as Write-Combine. Standard accesses might
allow the compiler to hoist reads out of loops or split reads. The C
implementation uses READ_ONCE() to prevent this.

> +            }
> +
> +            #[inline(always)]
> +            unsafe fn io_write(&self, value: $ty, address: usize) {
> +                let ptr = address as *mut $ty;
> +
> +                // SAFETY: The safety contract of `io_write` guarantees that address is a valid
> +                // address within the bounds of `Self` of at least the size of $ty, and is properly
> +                // aligned.
> +                unsafe { ptr::write(ptr, value) }

[Severity: High]
Similarly, does ptr::write() risk the compiler eliminating stores it
deems dead? Using WRITE_ONCE() would ensure the memory accesses happen
exactly as specified for hardware-shared memory.

> +            }
> +        }
> +    };
> +}

[ ... ]

> +impl<T: DriverObject> Drop for SGTableMap<T> {
> +    fn drop(&mut self) {
> +        // SAFETY: `obj` is always valid via our type invariants
> +        let obj = unsafe { self.obj.as_ref() };

[Severity: High]
If the devres callback executes this concurrently with
Object::free_callback resetting the sgt_res, is it possible that this
dereferences a raw pointer to an Object that has just been freed?

> +        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: Low]
Does this break bisectability? The function
__drm_gem_shmem_free_sgt_locked() does not seem to be defined in the C
headers at this commit.

> +    }
> +}

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