Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] Type inference hole in declare_drm_ioctls! allows safe handlers to trigger UB by inferring a Registered context before registration is verified. - [High] Unbounded lifetimes for `data` and `file` in declare_drm_ioctls! allow safe ioctl handlers to stash references, causing use-after-free. -- commit 28720068d91e18a10855651aab56d3a9e81f5d62 Author: Danilo Krummrich <[email protected]> rust: drm: Wrap ioctl dispatch in RegistrationGuard This commit wraps every ioctl handler inside a drm_dev_enter/exit critical section using RegistrationGuard. It returns ENODEV if the device is unplugged and anchors the driver type with a never-called closure to assist compiler type inference. > diff --git a/rust/kernel/drm/ioctl.rs b/rust/kernel/drm/ioctl.rs > index cf328101dde4e..c6a03be018e6d 100644 > --- a/rust/kernel/drm/ioctl.rs > +++ b/rust/kernel/drm/ioctl.rs [ ... ] > @@ -135,6 +138,16 @@ macro_rules! declare_drm_ioctls { > // dev/file match the current driver these > ioctls are being declared > // for, and it's not clear how to enforce this > within the type system. > let dev = > $crate::drm::device::Device::from_raw(raw_dev); > + > + // Type-inference anchor: the closure is never > called but ties `dev`'s > + // type to `$func`'s first parameter, which the > compiler cannot infer > + // through method resolution and associated-type > projections alone. > + #[allow(unreachable_code)] > + let _ = || $func(dev, unreachable!(), > unreachable!()); > + > + let Some(_guard) = dev.registration_guard() else > { > + return > $crate::error::code::ENODEV.to_errno(); > + }; [Severity: High] This is a pre-existing issue, but does this type inference anchor create a scenario where safe driver code can trigger undefined behavior? If a driver author writes a safe ioctl handler that expects a registered context, such as &Device<T, Registered>, the type-inference anchor will tie the type of dev to that first parameter. This causes Device::from_raw() to be evaluated with C = Registered. Since Device::from_raw() is called before dev.registration_guard() can verify the device is actually bound, doesn't this create a Registered reference for an unplugged device? Would this violate the type invariant and safety contract of from_raw() before the guard is even checked? [Severity: High] This is also a pre-existing issue, but are the lifetimes of the data and file arguments passed to the user's $func properly constrained? While the guard bounds the lifetime of the device access, the data and file parameters instantiated from raw pointers later in this macro don't appear to have explicit lifetime bounds. If $func demands a 'static lifetime for these arguments, will the compiler incorrectly infer them as 'static? If so, could a safe ioctl handler stash these references in a global variable or async context, causing a use-after-free once the ioctl callback returns and the C API invalidates the pointers? > // SAFETY: The ioctl argument has size > `_IOC_SIZE(cmd)`, which we > // asserted above matches the size of this type, > and all bit patterns of > // UAPI structs must be valid. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=11
