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 <[email protected]> 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/[email protected]?part=14
