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: <DBZZBNP278NH.2DR4PMWX9HKST@kernel.org>
Date: Tue, 12 Aug 2025 01:15:43 +0200
From: "Danilo Krummrich" <dakr@...nel.org>
To: "Benno Lossin" <lossin@...nel.org>
Cc: <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>,
 <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 Tue Aug 12, 2025 at 12:45 AM CEST, Benno Lossin wrote:
> 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.

Good catch! For some reason I already had UnsafePinned in mind, but it's still a
TODO to replace the Opaque with UnsafePinned.

> 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.

Yeah, the problem should go away with UnsafePinned. Maybe, until we have it, we
can just do the following:

diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
index bfccf4177644..1981201fa7f9 100644
--- a/rust/kernel/devres.rs
+++ b/rust/kernel/devres.rs
@@ -161,6 +161,9 @@ pub fn new<'a, E>(
                 //    live at least as long as the returned `impl PinInit<Self, Error>`.
                 to_result(unsafe {
                     bindings::devm_add_action(dev.as_raw(), Some(callback), inner.cast())
+                }).inspect_err(|_| {
+                    // SAFETY: `inner` is valid for dropping.
+                    unsafe { core::ptr::drop_in_place(inner) };
                 })?;
             },
         })