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

[Andreas Hindborg <a.hindborg@kernel.org>]

"Andreas Hindborg" <a.hindborg@kernel.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@kernel.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