| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Z36DU2Rf1O6cJbzG@phenom.ffwll.local>
Date: Wed, 8 Jan 2025 14:53:23 +0100
From: Simona Vetter <simona.vetter@...ll.ch>
To: Danilo Krummrich <dakr@...nel.org>
Cc: Boqun Feng <boqun.feng@...il.com>, gregkh@...uxfoundation.org,
rafael@...nel.org, ojeda@...nel.org, alex.gaynor@...il.com,
gary@...yguo.net, bjorn3_gh@...tonmail.com, benno.lossin@...ton.me,
a.hindborg@...nel.org, aliceryhl@...gle.com, tmgross@...ch.edu,
linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org
Subject: Re: [PATCH 2/2] rust: devres: remove action in `Devres::drop`
On Tue, Jan 07, 2025 at 10:49:40AM +0100, Danilo Krummrich wrote:
> On Mon, Jan 06, 2025 at 08:51:33AM -0800, Boqun Feng wrote:
> > On Fri, Jan 03, 2025 at 05:44:31PM +0100, Danilo Krummrich wrote:
> > > So far `DevresInner` is kept alive, even if `Devres` is dropped until
> > > the devres callback is executed to avoid a WARN() when the action has
> > > been released already.
> > >
> > > With the introduction of devm_remove_action_nowarn() we can remove the
> > > action in `Devres::drop`, handle the case where the action has been
> > > released already and hence also free `DevresInner`.
> > >
> > > Signed-off-by: Danilo Krummrich <dakr@...nel.org>
> > > ---
> > > rust/kernel/devres.rs | 56 +++++++++++++++++++++++++++++++++----------
> > > 1 file changed, 44 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
> > > index 9c9dd39584eb..7d3daac92109 100644
> > > --- a/rust/kernel/devres.rs
> > > +++ b/rust/kernel/devres.rs
> > > @@ -10,15 +10,19 @@
> > > bindings,
> > > device::Device,
> > > error::{Error, Result},
> > > + ffi::c_void,
> > > prelude::*,
> > > revocable::Revocable,
> > > sync::Arc,
> > > + types::ARef,
> > > };
> > >
> > > use core::ops::Deref;
> > >
> > > #[pin_data]
> > > struct DevresInner<T> {
> > > + dev: ARef<Device>,
> > > + callback: unsafe extern "C" fn(*mut c_void),
> > > #[pin]
> > > data: Revocable<T>,
> > > }
> > > @@ -98,6 +102,8 @@ impl<T> DevresInner<T> {
> > > fn new(dev: &Device, data: T, flags: Flags) -> Result<Arc<DevresInner<T>>> {
> > > let inner = Arc::pin_init(
> > > pin_init!( DevresInner {
> > > + dev: dev.into(),
> > > + callback: Self::devres_callback,
> > > data <- Revocable::new(data),
> > > }),
> > > flags,
> > > @@ -109,9 +115,8 @@ fn new(dev: &Device, data: T, flags: Flags) -> Result<Arc<DevresInner<T>>> {
> > >
> > > // SAFETY: `devm_add_action` guarantees to call `Self::devres_callback` once `dev` is
> > > // detached.
> > > - let ret = unsafe {
> > > - bindings::devm_add_action(dev.as_raw(), Some(Self::devres_callback), data as _)
> > > - };
> > > + let ret =
> > > + unsafe { bindings::devm_add_action(dev.as_raw(), Some(inner.callback), data as _) };
> > >
> > > if ret != 0 {
> > > // SAFETY: We just created another reference to `inner` in order to pass it to
> > > @@ -124,6 +129,41 @@ fn new(dev: &Device, data: T, flags: Flags) -> Result<Arc<DevresInner<T>>> {
> > > Ok(inner)
> > > }
> > >
> > > + fn as_ptr(&self) -> *const Self {
> > > + self as _
> > > + }
> > > +
> > > + fn remove_action(&self) {
> > > + // SAFETY:
> > > + // - `self.inner.dev` is a valid `Device`,
> > > + // - the `action` and `data` pointers are the exact same ones as given to devm_add_action()
> > > + // previously,
> > > + // - `self` is always valid, even if the action has been released already.
> > > + let ret = unsafe {
> > > + bindings::devm_remove_action_nowarn(
> > > + self.dev.as_raw(),
> > > + Some(self.callback),
> > > + self.as_ptr() as _,
> > > + )
> > > + };
> > > +
> > > + if ret != 0 {
> > > + // The devres action has been released already - nothing to do.
> > > + return;
> > > + }
> > > +
> > > + // SAFETY: We leaked an `Arc` reference to devm_add_action() in `DevresInner::new`; if
> > > + // devm_remove_action_nowarn() was successful we can (and have to) claim back ownership of
> > > + // this reference.
> > > + let _ = unsafe { Arc::from_raw(self.as_ptr()) };
> >
> > There is a pointer provenance issue here I think. `self` is a immutable
> > reference to `DevresInner<..>`, so the pointer derived from it doesn't
> > have the provenance for writing nor does it have the provenance for the
> > `refcount` field in `ArcInner`. Therefore it cannot be used to
> > reconstruct an `Arc`.
> >
> > We probably want to make `remove_action()` take an
> > `&Arc<DevresInner<T>>`. Or am I missing something subtle?
>
> Indeed, good catch!
Just for my own learning I've tried to understand why there's an issue
here, but no in DevresInner.devres_callback. In both cases we take the
exact same bag of bits and convert it into an Arc, relying on the C side
guaranteeing to us that we exclusively own whatever object that bag of
bits points to when converted into a real reference.
I don't think we rely on the provance of self here at all, because we just
pass that bag of bits to devm_remove_action_nowarn as a magic lookup key,
and in the ret == 0 case the C side guarantee is that we own the resulting
object if we convert it into one using Arc::from_raw.
I think you could replace self.as_ptr in this function with a random bit
value, and aside from being functionally nonsense and resulting in a
randomized leak on the C side I dont think it would be unsafe/unsound.
What am I missing?
Cheers, Sima
>
> >
> > Regards,
> > Boqun
> >
> > > +
> > > + // Revoke the data, such that it gets dropped and the actual resource is freed.
> > > + //
> > > + // SAFETY: When `drop` runs, it's guaranteed that nobody is accessing the revocable data
> > > + // anymore, hence it is safe not to wait for the grace period to finish.
> > > + unsafe { self.data.revoke_nosync() };
> > > + }
> > > +
> > > #[allow(clippy::missing_safety_doc)]
> > > unsafe extern "C" fn devres_callback(ptr: *mut kernel::ffi::c_void) {
> > > let ptr = ptr as *mut DevresInner<T>;
> > > @@ -165,14 +205,6 @@ fn deref(&self) -> &Self::Target {
> > >
> > > impl<T> Drop for Devres<T> {
> > > fn drop(&mut self) {
> > > - // Revoke the data, such that it gets dropped already and the actual resource is freed.
> > > - //
> > > - // `DevresInner` has to stay alive until the devres callback has been called. This is
> > > - // necessary since we don't know when `Devres` is dropped and calling
> > > - // `devm_remove_action()` instead could race with `devres_release_all()`.
> > > - //
> > > - // SAFETY: When `drop` runs, it's guaranteed that nobody is accessing the revocable data
> > > - // anymore, hence it is safe not to wait for the grace period to finish.
> > > - unsafe { self.revoke_nosync() };
> > > + self.0.remove_action();
> > > }
> > > }
> > > --
> > > 2.47.1
> > >
--
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Powered by blists - more mailing lists