linux-kernel - Re: [PATCH 3/4] rust: configfs: introduce rust support for configfs

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: <87a5azjlqk.fsf@kernel.org>
Date: Thu, 06 Feb 2025 13:33:55 +0100
From: Andreas Hindborg <a.hindborg@...nel.org>
To: "Danilo Krummrich" <dakr@...nel.org>
Cc: "Miguel Ojeda" <ojeda@...nel.org>,  "Alex Gaynor"
 <alex.gaynor@...il.com>,  "Boqun Feng" <boqun.feng@...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>,  "Joel Becker" <jlbec@...lplan.org>,
  "Christoph Hellwig" <hch@....de>,  <rust-for-linux@...r.kernel.org>,
  <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/4] rust: configfs: introduce rust support for configfs

"Andreas Hindborg" <a.hindborg@...nel.org> writes:

> This patch adds a rust API for configfs, thus allowing rust modules to use
> configfs for configuration. The implementation is a shim on top of the C
> configfs implementation allowing safe use of the C infrastructure from
> rust.
>
> The patch enables the `const_mut_refs` feature on compilers before rustc
> 1.83. The feature was stabilized in rustc 1.83 and is not required to be
> explicitly enabled on later versions.
>
> Signed-off-by: Andreas Hindborg <a.hindborg@...nel.org>
>
> ---

[...]

> +    /// # Safety
> +    ///
> +    /// If `this` does not represent the root group of a `configfs` subsystem,
> +    /// `this` must be a pointer to a `bindings::config_group` embedded in a
> +    /// `Group<PAR>`.
> +    ///
> +    /// Otherwise, `this` must be a pointer to a `bindings::config_group` that
> +    /// is embedded in a `bindings::configfs_subsystem` that is embedded in a
> +    /// `Subsystem<PAR>`.
> +    ///
> +    /// `item` must point to a `bindings::config_item` within a
> +    /// `bindings::config_group` within a `Group<CHLD>`.
> +    unsafe extern "C" fn drop_item(
> +        this: *mut bindings::config_group,
> +        item: *mut bindings::config_item,
> +    ) {
> +        // SAFETY: By function safety requirements of this function, this call
> +        // is safe.
> +        let parent_data = unsafe { get_group_data(this) };
> +
> +        // SAFETY: By function safety requirements, `item` is embedded in a
> +        // `config_group`.
> +        let c_child_group_ptr =
> +            unsafe { kernel::container_of!(item, bindings::config_group, cg_item) };
> +        // SAFETY: By function safety requirements, `c_child_group_ptr` is
> +        // embedded within a `Group<CHLD>`.
> +        let r_child_group_ptr = unsafe { Group::<CHLD>::container_of(c_child_group_ptr) };
> +
> +        if PAR::HAS_DROP_ITEM {
> +            PAR::drop_item(
> +                parent_data,
> +                // SAFETY: We called `into_foreign` to produce `r_child_group_ptr` in
> +                // `make_group`. There are not other borrows of this pointer in existence.
> +                unsafe { PCPTR::borrow(r_child_group_ptr.cast_mut()) },
> +            );
> +        }
> +
> +        // SAFETY: By C API contract, `configfs` is not going to touch `item`
> +        // again.
> +        unsafe { bindings::config_item_put(item) };

This turned out to be wrong. We _do_ have to let go of a refcount here,
but we are not allowed to free the item.

> +
> +        // SAFETY: We called `into_foreign` on `r_chilc_group_ptr` in
> +        // `make_group`.
> +        let pin_child: PCPTR = unsafe { PCPTR::from_foreign(r_child_group_ptr.cast_mut()) };
> +        drop(pin_child);

So this is wrong and will cause UAF. We have to wait for a call to
ct_item_ops.release and do the cleanup there. I will address this in the
next version. Removing directories is likely to cause trouble with this
patch.


Best regards,
Andreas Hindborg