| 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: <Z3_4vQPiZGRZ5F7X@phenom.ffwll.local>
Date: Thu, 9 Jan 2025 17:26:37 +0100
From: Simona Vetter <simona.vetter@...ll.ch>
To: Boqun Feng <boqun.feng@...il.com>
Cc: Danilo Krummrich <dakr@...nel.org>, 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 Thu, Jan 09, 2025 at 07:20:44AM -0800, Boqun Feng wrote:
> On Thu, Jan 09, 2025 at 10:50:38AM +0100, Simona Vetter wrote:
> > On Wed, Jan 08, 2025 at 02:53:23PM +0100, Simona Vetter wrote:
> > > 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
>
> I also learned this in a hard way ;-) let me try to explain my
> understanding.
>
> First the difference between here and `DevresInner::devres_callback()`
> is "how the pointer was derived" or "where did the pointer come from".
>
> * In `DevresInner::devres_callback()` case, it uses a pointer value
> directly, and that value came from a previous `Arc::into_raw()` which
> uses the pointer directly with an offset (note that offsetting doesn't
> change the pointer provenance), so that pointer has the same
> provenance as an `Arc`.
>
> * In here, what we did was getting a pointer to `DevresInner` from a
> `&DevresInner` and that pointer doesn't have the provenane to the
> whole `ArcInner<DervesInner>`, nor does it have the write permission
> because it comes from an immutable reference.
Yeah I think I got that part of understanding provenance, it's essentially
all the things the compiler keeps track of for optimization passes.
> > > 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.
> > >
>
> Other than `Arc::from_raw()` there is another side in this picture: the
> one that provides the immutable reference, so if you do:
>
> let p: &Arc<T> = ...; // say you have an `Arc<T>` reference.
> let o = p.deref(); // you got a reference to the internal object.
>
> let ptr = o as *const T;
> let new_arc = Arc::from_raw(ptr); // use the pointer of `o` to
> // construct a new Arc. Even if
> // there is a spare refcount some
> // where due to a Arc::into_raw()
> // previously.
> do_something(new_arc);
>
> use(p); // here the compiler is within its right to assume if there
> // is no other access the underlying `Arc<T>`, the refcount
> // should be unchanged (more precisely, the whole `ArcInner`
> // should be unchanged).
>
> And by voliating pointer provenance, the assumption is broken and there
> is no way to tell compiler about that (at least in the current
> operational semantics of Rust IIUC). Intuitively, `o` only has the
> permission to read the object, so we cannot use it for construct an
> `Arc`.
>
> This to me is how pointer provenance actually works: it's a model that
> describes how compilers can make assumptions so that programmers and
> compilers have the same picture about whether there could be an
> optimization based on an assumption.
>
> Does this make sense to you?
Yes I think so, but your example above has the crucial issue that we've
picked the wrong raw pointer to pass to from_raw, because it's one that we
didn't get back from into_raw. So with my understanding of this all from
that pov, the from_raw in v1 of this patch is broken.
The issue is that the fix still doesn't get things right, because while we
supply a pointer with enough provenance to hopefully prevent the compiler
from doing bad things, it's still not the pointer we've received from
into_raw. They happen to bit bit identical ofc, but the entire provenance
thing is about all the stuff besides the raw pointer value.
There's two cases for how I think the provenance is passed on:
Arc::into_raw -> devm_add_action -> devm_callback -> Arc::from_raw
This one is correct, because we pass the right raw pointer into
Arc::from_raw.
Arc::into_raw -> devm_add_action -> (in remove_actio)
devm_remove_action_nowarn -> Arc::from_raw
Now the thing to note is that devm_remove_action_nowarn does not return a
void * with the data we need to clean up, because the void * data happens
to bit-vise identical to the lookup key we pass in. But what it does
return is the provenance of the bitwise identical pointer that was passed
into devm_add_action, and that provenance is the right one because it's
the pointer we got from Arc::into_raw. On the other hand the lookup key
does not have the right provenance, because we did not get that one from
into_raw.
I think there's a few options here:
- we hope the compiler isn't smart enough
- we pass some provenance that should allow us to do all the right access
as a stand-in, and hope the compiler isn't smart enough. This only works
if we have a suitable reference we can pass down the call chain to where
we need it (like is done in v2), and I don't think this is possible in
all cases.
I can think of two examples where this might happen:
- We only get an immutable reference, and the C ffi call is what gives
us the provenance/proof for write access.
- We only have a pointer to an inner type (probably gotten from C ffi),
and the C ffi call does a runtime upcast and so supplies the proof
that our reference is part of something much bigger.
Neither of these make sense with pure rust code, but I do think it's
stuff that can pop up when interfacing kernel C functions.
- we add a fake void * out parameter to the ffi wrapper so that we can
pretend to get the provenance back from the C side (it's just a copy of
the lookup key we passed in when ignoring the non-value parts of a
pointer). Maybe could even do that properly with the new strict
provenance extensions that rust has, so that it's a true phantom
value.
- we have a much more magic replacement for into_raw/from_raw which
magically transfers the provenance behind the scenes. So kinda like
free() consumes provenance and malloc() creates new provenance (at least
for mutable references, for immutable ones it's a bit different), except
the pointer value stays the same and the memory contents of the object
also stays the same. So something like Arc::into_raw_c_void and
Arc::from_raw_c_void, since this magic provenance transfer would only
really make sense for ffi with C code where the void * is just an opaque
reference that's passed around, and the C code never looks at what's
behind it. Well except for runtime type casting, where the type is
probably a bit more specific than void *.
I don't think we can ignore this issue, because using void * as the lookup
key is fairly common in the kernel, and these lookup functions don't
bother with returning that pointer again. But semantically they do return
the provenance for the full object that you pass in and cast to void *,
it's just that C totally sucks at type safety.
Or maybe I'm just really confused about all this still ...
Cheers, Sima
>
> Regards,
> Boqun
>
> > > 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?
> >
> > Trying to sharpen my argument a bit after chatting with Danilo on irc:
> >
> > My take is that Arc::from_raw must discard provenance of the argument, and
> > instead entirely relies on Arc::into_raw having been call beforehand with
> > a return value that bitwise matches what we stuff into from_raw. So it
> > should inherit the provenance of the reference we passed to Arc::into_raw.
> >
> > If that's not the case then from_raw is busted, and it only happens to
> > work for ffi callbacks because the compiler cannot see behind the ffi
> > curtain.
> > -Sima
> >
> > >
> > > 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
> >
> > --
> > Simona Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
--
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Powered by blists - more mailing lists