linux-kernel - Re: [PATCH] rust: devres: fix leaking call to devm_add

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <DBZYO8O9YTO3.10MKWPYN8YEOB@kernel.org>
Date: Tue, 12 Aug 2025 00:45:08 +0200
From: "Benno Lossin" <lossin@...nel.org>
To: "Danilo Krummrich" <dakr@...nel.org>, <gregkh@...uxfoundation.org>,
 <rafael@...nel.org>, <ojeda@...nel.org>, <alex.gaynor@...il.com>,
 <boqun.feng@...il.com>, <gary@...yguo.net>, <bjorn3_gh@...tonmail.com>,
 <a.hindborg@...nel.org>, <aliceryhl@...gle.com>, <tmgross@...ch.edu>
Cc: <rust-for-linux@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
 <stable@...r.kernel.org>
Subject: Re: [PATCH] rust: devres: fix leaking call to devm_add_action()

On Mon Aug 11, 2025 at 11:44 PM CEST, Danilo Krummrich wrote:
> When the data argument of Devres::new() is Err(), we leak the preceding
> call to devm_add_action().
>
> In order to fix this, call devm_add_action() in a unit type initializer in
> try_pin_init!() after the initializers of all other fields.
>
> Cc: stable@...r.kernel.org
> Fixes: f5d3ef25d238 ("rust: devres: get rid of Devres' inner Arc")
> Signed-off-by: Danilo Krummrich <dakr@...nel.org>
> ---
>  rust/kernel/devres.rs | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
> index da18091143a6..bfccf4177644 100644
> --- a/rust/kernel/devres.rs
> +++ b/rust/kernel/devres.rs
> @@ -119,6 +119,7 @@ pub struct Devres<T: Send> {
>      // impls can be removed.
>      #[pin]
>      inner: Opaque<Inner<T>>,
> +    _add_action: (),
>  }
>  
>  impl<T: Send> Devres<T> {
> @@ -140,7 +141,15 @@ pub fn new<'a, E>(
>              dev: dev.into(),
>              callback,
>              // INVARIANT: `inner` is properly initialized.
> -            inner <- {
> +            inner <- Opaque::pin_init(try_pin_init!(Inner {
> +                    devm <- Completion::new(),
> +                    revoke <- Completion::new(),
> +                    data <- Revocable::new(data),
> +            })),
> +            // TODO: Replace with "initializer code blocks" [1] once available.
> +            //
> +            // [1] https://github.com/Rust-for-Linux/pin-init/pull/69
> +            _add_action: {
>                  // SAFETY: `this` is a valid pointer to uninitialized memory.
>                  let inner = unsafe { &raw mut (*this.as_ptr()).inner };
>  
> @@ -153,12 +162,6 @@ pub fn new<'a, E>(
>                  to_result(unsafe {
>                      bindings::devm_add_action(dev.as_raw(), Some(callback), inner.cast())
>                  })?;

I have some bad news, I think this is also wrong: if the
`devm_add_action` fails, we never drop the contents of `inner`, since
the destructor of `Opaque` does nothing and we never finished
construction of `Devres`, so its `Drop` will never be called.

One solution would be to use `pin_chain` on the initializer for `Inner`
(not opaque). Another one would be to not use opaque, `UnsafePinned`
actually looks like the better fit for this use-case.

This also made me re-think `Opaque::pin_init`. It seems wrong and
probably shouldn't exist, as `Opaque` violates the drop guarantee
required by pinned data. So it cannot structurally pin the data inside.

---
Cheers,
Benno

> -
> -                Opaque::pin_init(try_pin_init!(Inner {
> -                    devm <- Completion::new(),
> -                    revoke <- Completion::new(),
> -                    data <- Revocable::new(data),
> -                }))
>              },
>          })
>      }
>
> base-commit: 8f5ae30d69d7543eee0d70083daf4de8fe15d585