Re: [PATCH v4 14/16] rust: drm: Add RegistrationData to drm::Driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Unsound lifetime shortening of `RegistrationData` allows safe code to trigger a use-after-free.
- [High] Data race and use-after-free on `RegistrationData` when `drm_dev_register()` fails.
--

commit 4c4fafa27855b4e677d80a9c3bf6cb9acd303180
Author: Danilo Krummrich <dakr@kernel.org>

rust: drm: Add RegistrationData to drm::Driver

This patch introduces a RegistrationData associated type for drm::Driver
whose lifetime is tied to the parent bus device. It manages the registration
scope and provides safe access to this data within a RegistrationGuard.

> diff --git a/rust/kernel/drm/device.rs b/rust/kernel/drm/device.rs
> index 8f63276c9b62f..fc79f90a197d9 100644
> --- a/rust/kernel/drm/device.rs
> +++ b/rust/kernel/drm/device.rs

[ ... ]

> +    #[inline]
> +    unsafe fn registration_data_unchecked(&self) -> &<T::RegistrationData as ForLt>::Of<'_> {
> +        // SAFETY:
> +        // - Caller guarantees the parent bus device is bound, hence the pointer is valid.
> +        // - The pointer cast from `Of<'static>` to `Of<'_>` is layout-compatible since lifetimes
> +        //   are erased at runtime.
> +        // - Caller guarantees the reference is only used behind an HRTB, making the lifetime
> +        //   shortening sound regardless of variance.
> +        unsafe { (*self.registration_data.get()).cast::<_>().as_ref() }
> +    }

[ ... ]

> +    pub fn registration_data_with<R, F>(&self, f: F) -> R
> +    where
> +        F: for<'a> FnOnce(&'a <T::RegistrationData as ForLt>::Of<'a>) -> R,
> +    {
> +        // SAFETY: `Registered` guarantees the device is registered and the parent bus device is
> +        // bound. The closure's HRTB `for<'a>` prevents the caller from smuggling in references
> +        // with a concrete short lifetime, satisfying the lifetime requirement of
> +        // `registration_data_unchecked`.
> +        f(unsafe { self.registration_data_unchecked() })
> +    }

[Severity: Critical]
Does this lifetime shortening cast violate soundness if the Of<'a> type is
invariant (e.g., Cell<&'a u32> or Mutex<Option<&'a u32>>)?

Even with the HRTB bound in registration_data_with(), safe driver code could
do something like device.registration_data_with(|data| data.set(&local_var))
where local_var outlives the closure but not the device's 'bound lifetime.
Because the lifetime of the invariant container was shortened to the closure's
scope, the compiler will accept this and allow writing a short-lived reference
into it.

When the function returns and local_var is dropped, the physical
RegistrationData container living in the device will hold a dangling pointer,
leading to a use-after-free when the data is accessed again.

> diff --git a/rust/kernel/drm/driver.rs b/rust/kernel/drm/driver.rs
> index ceb2829985c78..e42b48e5cad28 100644
> --- a/rust/kernel/drm/driver.rs
> +++ b/rust/kernel/drm/driver.rs

[ ... ]

> +        // SAFETY: `drm` is a valid, initialized but not yet registered DRM device.
> +        let ret = unsafe { bindings::drm_dev_register(drm.as_raw(), flags) };
> +        if let Err(e) = to_result(ret) {
> +            // SAFETY: No concurrent access; registration failed.
> +            unsafe { *drm.registration_data.get() = NonNull::dangling() };
> +            return Err(e);
> +        }

[Severity: High]
Can this error path trigger a use-after-free if drm_dev_register() fails
partway through its initialization?

During drm_dev_register(), minor nodes (such as the render node) are
registered sequentially via drm_minor_register(). As soon as a minor node is
registered, userspace can open it and begin executing ioctls. These concurrent
ioctls will successfully enter the RegistrationGuard critical section since
dev->unplugged is false.

If drm_dev_register() subsequently fails on the primary node, it returns an
error without calling drm_dev_unplug() (which would otherwise perform an SRCU
barrier to wait for existing readers). Returning Err(e) here causes the
RegistrationData to be dropped and freed, while a concurrent ioctl still
inside its drm_dev_enter() critical section could access this freed memory.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260620184924.2247517-1-dakr@kernel.org?part=14