linux-kernel - Re: [PATCH 2/3] rust: miscdevice: Add additional data to MiscDeviceRegistration

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: <912d9ba8-4cd1-434c-ba32-5c74ef4c12be@gmail.com>
Date: Mon, 27 Jan 2025 14:27:51 +0100
From: Christian Schrefl <chrisi.schrefl@...il.com>
To: Alice Ryhl <aliceryhl@...gle.com>
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>,
 Andreas Hindborg <a.hindborg@...nel.org>, Trevor Gross <tmgross@...ch.edu>,
 Arnd Bergmann <arnd@...db.de>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Lee Jones <lee@...nel.org>,
 rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] rust: miscdevice: Add additional data to
 MiscDeviceRegistration

Hi Alice

On 27.01.25 11:27 AM, Alice Ryhl wrote:
><snip>
>>
>> to make sure that `misc_register` is called after data is initialized and to that
>> `data` will be dropped correctly in case `misc_register` fails.
>>
>> But I'm not very familiar with `(try_)pin_init!` so this might be unnecessary?
> 
> Using pin_chain is definitely incorrect because it will run the
> destructor of MiscDeviceRegistration if the misc_register call fails,
> but calling misc_deregister is incorrect in that case.
> 
> You should be able to just move the `data <-
> UnsafePinned::try_pin_init(data)` line to the top so that the field is
> initialized first.
> 

So if I understand correctly, the following should be correct:

pub fn register(
    opts: MiscDeviceOptions,
    data: impl PinInit<T::RegistrationData, Error>,
) -> impl PinInit<Self, Error> {
    try_pin_init!(Self {
        data <- UnsafePinned::try_pin_init(data),
        inner <- Opaque::try_ffi_init(move |slot: *mut bindings::miscdevice| {
            // SAFETY: The initializer can write to the provided `slot`.
            unsafe { slot.write(opts.into_raw::<T>()) };
            // SAFETY: We just wrote the misc device options to the slot. The miscdevice will
            // get unregistered before `slot` is deallocated because the memory is pinned and
            // the destructor of this type deallocates the memory.
            // `data` is Initialized before `misc_register` so no race with `fops->open()`
            // is possible.
            // INVARIANT: If this returns `Ok(())`, then the `slot` will contain a registered
            // misc device.
            to_result(unsafe { bindings::misc_register(slot) })
        }),
        _t: PhantomData,
    })
}

Sorry I don't know the details of `(try_)pin_init` and the docs only say:
> The fields are initialized in the order that they appear in the initializer. So it is possible
> to read already initialized fields using raw pointers.
> 
> IMPORTANT: You are not allowed to create references to fields of the struct inside of the 
> initializer.

This says its invalid to create references, but as soon as `misc_register` its theoretically
possible that a `open()` call happens and creates a reference to the Registration. I assume
that is fine, because the Registration would be fully initialized at that point, but that's
technically against the Docs.

Also does `try_pin_init!()` automatically drop `data` when `bindings::misc_register` fails?

I couldn't find anything in the Docs about when and what is dropped.

Is there a equivalent to `cargo expand` for the kernel?
It would be nice to be able to look at the code generated by the macros.

Cheers
Christian