Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] `DevresGuard` implements `Deref` exposing the inner lifetime `'a` tied to `DevresLt` rather than the guard's lifetime (`'guard`), causing an API soundness issue that can lead to Use-After-Free. - [Low] Missing `#[inline]` annotations on small forwarding abstraction functions. --
commit 20febe7248eef3fe1ba711b99c2b480ed7284b88 Author: Danilo Krummrich <[email protected]> rust: devres: add DevresLt for ForLt-aware device resource access This commit introduces DevresLt, a wrapper around Devres that provides lifetime-aware access to device resources. It adds accessors that correctly shorten the stored static lifetime to the caller's borrow lifetime, preventing unsound static lifetimes from being exposed. > diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs > index 11ce500e9b76f..e11deff3e1be5 100644 > --- a/rust/kernel/devres.rs > +++ b/rust/kernel/devres.rs [ ... ] > @@ -365,6 +367,104 @@ fn drop(&mut self) { > } > } > > +/// Guard returned by [`DevresLt::try_access`]. > +/// > +/// Dereferences to `F::Of<'a>`, shortening the lifetime of the stored data > to the guard's borrow > +/// lifetime. > +pub struct DevresGuard<'a, F: CovariantForLt>(RevocableGuard<'a, > F::Of<'static>>); > + > +impl<'a, F: CovariantForLt> core::ops::Deref for DevresGuard<'a, F> { > + type Target = F::Of<'a>; > + > + fn deref(&self) -> &Self::Target { [Severity: Low] This isn't a bug, but shouldn't this trivial forwarding function be annotated with #[inline]? [Severity: High] Could implementing Deref here expose the outer lifetime 'a instead of the guard's lifetime, potentially leading to a use-after-free? Because F::Of<'a> is exposed with its outer 'a lifetime, a caller can trivially extract references with lifetime 'a from it. When DevresGuard is dropped, the internal RCU guard is released, ending the RCU read-side critical section. However, the extracted 'a reference is still valid from the compiler's perspective because it is tied to DevresLt. If the device is concurrently unbound, devres will revoke and drop the internal data. Accessing the extracted 'a reference afterwards results in a use-after-free. Would it be safer to remove Deref and provide an explicit get method that yields &'guard F::Of<'guard>? > + F::cast_ref(&*self.0) > + } > +} [ ... ] > + /// Return a reference of the [`Device`] this [`DevresLt`] instance has > been created with. > + pub fn device(&self) -> &Device { [Severity: Low] This isn't a bug, but should this small forwarding function have an #[inline] annotation as per the Rust subsystem guidelines? This also applies to other trivial forwarding methods added in this commit, such as access_with, try_access_with, and access. > + self.0.device() > + } [ ... ] > + /// [`DevresLt`] accessor for [`Revocable::try_access`]. > + pub fn try_access(&self) -> Option<DevresGuard<'_, F>> { [Severity: High] Is there a risk that tying the guard to the borrow lifetime of &self allows references to outlive the RCU read-side critical section? This method borrows &self for lifetime 'a and returns Option<DevresGuard<'a, F>>. Combined with the Deref implementation, this exposes the longer lifetime 'a to the caller, bypassing the RCU lock's guard lifetime. [Severity: Low] This isn't a bug, but would it be appropriate to add an #[inline] annotation to this forwarding function? > + self.0.data().try_access().map(DevresGuard) > + } > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=5
