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: <CAH5fLgi-CJCveLmc6hFdAYOhpRULovDh_Cix4FyV=kBeg9oiFA@mail.gmail.com>
Date: Mon, 27 Jan 2025 11:27:30 +0100
From: Alice Ryhl <aliceryhl@...gle.com>
To: Christian Schrefl <chrisi.schrefl@...il.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

On Fri, Jan 24, 2025 at 12:26 AM Christian Schrefl
<chrisi.schrefl@...il.com> wrote:
>
> On 19.01.25 11:11 PM, Christian Schrefl wrote:
> > When using the Rust miscdevice bindings, you generally embed the
> > MiscDeviceRegistration within another struct:
> >
> > struct MyDriverData {
> >     data: SomeOtherData,
> >     misc: MiscDeviceRegistration<MyMiscFile>
> > }
> >
> > <snip>
> >
> >  impl<T: MiscDevice> MiscDeviceRegistration<T> {
> >      /// Register a misc device.
> > -    pub fn register(opts: MiscDeviceOptions) -> impl PinInit<Self, Error> {
> > +    pub fn register(
> > +        opts: MiscDeviceOptions,
> > +        data: impl PinInit<T::RegistrationData, Error>,
> > +    ) -> impl PinInit<Self, Error> {
> >          try_pin_init!(Self {
> >              inner <- Opaque::try_ffi_init(move |slot: *mut bindings::miscdevice| {
> >                  // SAFETY: The initializer can write to the provided `slot`.
> > @@ -79,6 +85,7 @@ pub fn register(opts: MiscDeviceOptions) -> impl PinInit<Self, Error> {
> >                  // misc device.
> >                  to_result(unsafe { bindings::misc_register(slot) })
> >              }),
> > +            data <- Aliased::try_pin_init(data),
> >              _t: PhantomData,
> >          })
>
> After some thought I think `register` needs to be something like:
>
> pub fn register(
>     opts: MiscDeviceOptions,
>     data: impl PinInit<T::RegistrationData, Error>,
> ) -> impl PinInit<Self, Error> {
>     try_pin_init!(Self {
>         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>()) };
>             Ok::<(), Error>(())
>         }),
>         data <- UnsafePinned::try_pin_init(data),
>         _t: PhantomData,
>     })
>     .pin_chain(|slot| {
>         // 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.as_raw()) }).map_err(|err| {
>             // Drop the Data in case misc_register fails.
>             // SAFETY:
>             // - We just initialized `data`.
>             // - We are about to return `Err(err)`, so it is valid for us to drop `data`.
>             unsafe {
>                 ptr::drop_in_place(slot.data.get());
>             }
>             err
>         })
>     })
> }
>
> 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.

Alice