linux-kernel - Re: [PATCH v2] 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: <DC1E72OCYMMU.1C1EWX0YPDEOT@kernel.org>
Date: Wed, 13 Aug 2025 17:07:41 +0200
From: "Danilo Krummrich" <dakr@...nel.org>
To: "Alice Ryhl" <aliceryhl@...gle.com>
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>, <lossin@...nel.org>, <a.hindborg@...nel.org>,
 <tmgross@...ch.edu>, <rust-for-linux@...r.kernel.org>,
 <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] rust: devres: fix leaking call to devm_add_action()

On Wed Aug 13, 2025 at 9:16 AM CEST, Alice Ryhl wrote:
> On Tue, Aug 12, 2025 at 03:09:06PM +0200, 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.
>> 
>> Fixes: f5d3ef25d238 ("rust: devres: get rid of Devres' inner Arc")
>> Signed-off-by: Danilo Krummrich <dakr@...nel.org>
>
> This looks ok:
>
> Reviewed-by: Alice Ryhl <aliceryhl@...gle.com>
>
> But can't we do it like this instead?
> 1. Allocate devm job.
> 2. Initialize inner field.
> 3. Use allocation from (1.) to devm_add_action() infallibly.

Theoretically, we could with a few additions to the C API. But I don't think
it's worth and I don't think we should do it in the context of this patch.

> This way, there's no risk that the inner value may get dropped, which
> could be an expensive operation.

If we actually fail to allocate a devres node on the C side, I'm not that
concerned about having to drop data.

However, there's also another reason why I think there's no need to consider it
now: I still have the rework on my list to get devres callbacks in place such
that we can first revoke the Revocable objects of all corresponding Devres
objects, call synchronize_rcu() once, and then drop the contained data in-place.

In this context I also plan to directly embedd a struct devres_node in the Rust
Devres type, such that the *only* allocation that remains is the final one when
the user of Devres allocates for the final impl PinInit, that directly or
indirectly contains the Devres.

Once we have that, adding the devres node will also always be infallible.