linux-kernel - Re: [PATCH v2 0/2] rust: lockdep: Fix soundness issue affecting LockClassKeys

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] [day] [month] [year] [list]

Message-ID: <Z4Eq0qoZaIt7j9zW@boqun-archlinux>
Date: Fri, 10 Jan 2025 06:12:34 -0800
From: Boqun Feng <boqun.feng@...il.com>
To: Mitchell Levy <levymitchell0@...il.com>
Cc: Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>,
	Wedson Almeida Filho <wedsonaf@...il.com>,
	Gary Guo <gary@...yguo.net>,
	Björn Roy Baron <bjorn3_gh@...tonmail.com>,
	Benno Lossin <benno.lossin@...ton.me>,
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
	Andreas Hindborg <a.hindborg@...nel.org>,
	linux-block@...r.kernel.org, rust-for-linux@...r.kernel.org,
	linux-kernel@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH v2 0/2] rust: lockdep: Fix soundness issue affecting
 LockClassKeys

On Thu, Dec 19, 2024 at 12:58:54PM -0800, Mitchell Levy wrote:
> This series is aimed at fixing a soundness issue with how dynamically
> allocated LockClassKeys are handled. Currently, LockClassKeys can be
> used without being Pin'd, which can break lockdep since it relies on
> address stability. Similarly, these keys are not automatically
> (de)registered with lockdep.
> 
> At the suggestion of Alice Ryhl, this series includes a patch for
> -stable kernels that disables dynamically allocated keys. This prevents
> backported patches from using the unsound implementation.
> 
> Currently, this series requires that all dynamically allocated
> LockClassKeys have a lifetime of 'static (i.e., they must be leaked
> after allocation). This is because Lock does not currently keep a
> reference to the LockClassKey, instead passing it to C via FFI. This
> causes a problem because the rust compiler would allow creating a
> 'static Lock with a 'a LockClassKey (with 'a < 'static) while C would
> expect the LockClassKey to live as long as the lock. This problem
> represents an avenue for future work.
> 

Thanks for doing this! I found some clippy warnings with the current
version, but overall it looks good to me. That said, appreciate it if
patch #2 gets more reviews on the interface changes, thanks!

Regards,
Boqun

> ---
> Changes from RFC:
> - Split into two commits so that dynamically allocated LockClassKeys are
> removed from stable kernels. (Thanks Alice Ryhl)
> - Extract calls to C lockdep functions into helpers so things build
> properly when LOCKDEP=n. (Thanks Benno Lossin)
> - Remove extraneous `get_ref()` calls. (Thanks Benno Lossin)
> - Provide better documentation for `new_dynamic()`. (Thanks Benno
> Lossin)
> - Ran rustfmt to fix formatting and some extraneous changes. (Thanks
> Alice Ryhl and Benno Lossin)
> - Link to RFC: https://lore.kernel.org/r/20240905-rust-lockdep-v1-1-d2c9c21aa8b2@gmail.com
> 
> ---
> Changes in v2:
> - Dropped formatting change that's already fixed upstream (Thanks Dirk
>   Behme).
> - Moved safety comment to the right point in the patch series (Thanks
>   Dirk Behme and Boqun Feng).
> - Added an example of dynamic LockClassKey usage (Thanks Boqun Feng).
> - Link to v1: https://lore.kernel.org/r/20241004-rust-lockdep-v1-0-e9a5c45721fc@gmail.com
> 
> ---
> Mitchell Levy (2):
>       rust: lockdep: Remove support for dynamically allocated LockClassKeys
>       rust: lockdep: Use Pin for all LockClassKey usages
> 
>  rust/helpers/helpers.c          |  1 +
>  rust/helpers/sync.c             | 13 +++++++++
>  rust/kernel/sync.rs             | 63 ++++++++++++++++++++++++++++++++++-------
>  rust/kernel/sync/condvar.rs     |  5 ++--
>  rust/kernel/sync/lock.rs        |  9 ++----
>  rust/kernel/sync/lock/global.rs |  5 ++--
>  rust/kernel/sync/poll.rs        |  2 +-
>  rust/kernel/workqueue.rs        |  3 +-
>  8 files changed, 78 insertions(+), 23 deletions(-)
> ---
> base-commit: 0c5928deada15a8d075516e6e0d9ee19011bb000
> change-id: 20240905-rust-lockdep-d3e30521c8ba
> 
> Best regards,
> -- 
> Mitchell Levy <levymitchell0@...il.com>
>