Re: [PATCH 5/7] rust: drm: wrap fops open with RegistrationGuard

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Re-acquiring the DRM minor by ID in `fops_open` is vulnerable to a TOCTOU race, breaking type safety invariants.
--

--- Patch [5]: [PATCH 5/7] rust: drm: wrap fops open with RegistrationGuard ---
commit f19fe1023e3626e0af6b5b4f7cdaf8f59acc5f98
Author: Danilo Krummrich <[email protected]>

rust: drm: wrap fops open with RegistrationGuard

Add a Minor abstraction with RAII release and a fops_open() wrapper that
holds a RegistrationGuard (drm_dev_enter / drm_dev_exit) across the
entire drm_open() call. This guarantees that drm_dev_unplug() in 
Registration::drop() waits for the full open sequence to complete.

> diff --git a/rust/kernel/drm/device.rs b/rust/kernel/drm/device.rs
> index a2940e172073a..09903ed783e15 100644
> --- a/rust/kernel/drm/device.rs
> +++ b/rust/kernel/drm/device.rs

[ ... ]

> +    /// Wrapper for `fops.open` that holds a [`RegistrationGuard`] across the entire `drm_open()`
> +    /// call. This guarantees that `drm_dev_unplug()` in `Registration::drop()` waits for the full
> +    /// open sequence.
> +    extern "C" fn fops_open(inode: *mut bindings::inode, filp: *mut bindings::file) -> c_int {
> +        let f = || -> Result<c_int> {
> +            // SAFETY: `inode` is valid.
> +            let minor_id = unsafe { bindings::iminor(inode) };
> +            let minor = Minor::acquire(minor_id)?;
> +
> +            // SAFETY: `fops_open` is only installed for devices of type `T` (via `FOPS`).
> +            let _guard = (unsafe { minor.device::<T>() })

[Severity: High]
Can this sequence cause type confusion if the DRM minor ID is reassigned?

If a device T is unplugged concurrently with the open call, the minor_id
can be freed and reassigned to a different device U.

Since the VFS already resolved filp->f_op to T::fops_open during
drm_stub_open, the following sequence could occur:

1. VFS calls chrdev_open which resolves to drm_stub_open
2. drm_stub_open looks up minor_id, gets device T, and sets filp->f_op to
   T::FOPS
3. Device T is unplugged, and its minor_id is freed
4. Device U is hot-plugged, and drm_minor_alloc reuses the same minor_id
5. T::fops_open is called by VFS and invokes Minor::acquire(minor_id)
6. The returned minor now belongs to device U, not T

When minor.device::<T>() is called here, it unsafely downcasts Device<U>
to Device<T>. This violates Rust's type safety and aliasing invariants.
Is there a way to avoid looking up the minor by ID again to prevent this
race condition?

> +                .registration_guard()
> +                .ok_or(ENODEV)?;
> +
> +            // SAFETY: `inode` and `filp` are valid. The RegistrationGuard ensures the entire
> +            // `drm_open()` runs within the SRCU critical section.
> +            Ok(unsafe { bindings::drm_open(inode, filp) })
> +        };
> +
> +        match f() {
> +            Ok(ret) => ret,
> +            Err(e) => e.to_errno(),
> +        }
> +    }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5