When using the Rust miscdevice bindings, you generally embed the
MiscDeviceRegistration within another struct:

struct MyDriverData {
    data: SomeOtherData,
    misc: MiscDeviceRegistration<MyMiscFile>
}

In the `fops->open` callback of the miscdevice, you are given a
reference to the registration, which allows you to access its fields.
For example, as of commit 284ae0be4dca ("rust: miscdevice: Provide
accessor to pull out miscdevice::this_device") you can access the
internal `struct device`. However, there is still no way to access the
`data` field in the above example, because you only have a reference to
the registration.

Using container_of is also not possible to do safely. For example, if
the destructor of `MyDriverData` runs, then the destructor of `data`
would run before the miscdevice is deregistered, so using container_of
to access `data` from `fops->open` could result in a UAF. A similar
problem can happen on initialization if `misc` is not the last field to
be initialized.

To provide a safe way to access user-defined data stored next to the
`struct miscdevice`, make `MiscDeviceRegistration` into a container that
can store a user-provided piece of data. This way, `fops->open` can
access that data via the registration, since the data is stored inside
the registration.

The container enforces that the additional user data is initialized
before the miscdevice is registered, and that the miscdevice is
deregistered before the user data is destroyed. This ensures that access
to the userdata is safe.

For the same reasons as in commit 88441d5c6d17 ("rust: miscdevice:
access the `struct miscdevice` from fops->open()"), you cannot access
the user data in any other fops callback than open. This is because a
miscdevice can be deregistered while there are still open files.

A situation where this user data might be required is when a platform
driver acquires a resource in `probe` and wants to use this resource in
the `fops` implementation of a `MiscDevice`.

Suggested-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Christian Schrefl <chrisi.schrefl@gmail.com>
---
 rust/kernel/miscdevice.rs        | 37 ++++++++++++++++++++++++++++++-------
 samples/rust/rust_misc_device.rs |  4 +++-
 2 files changed, 33 insertions(+), 8 deletions(-)

diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs
index dfb363630c70b7187cae91f692d38bcf42d56a0a..3a756de27644e8a14e5bbd6b8abd9604e05faed4 100644
--- a/rust/kernel/miscdevice.rs
+++ b/rust/kernel/miscdevice.rs
@@ -16,7 +16,7 @@
     prelude::*,
     seq_file::SeqFile,
     str::CStr,
-    types::{ForeignOwnable, Opaque},
+    types::{Aliased, ForeignOwnable, Opaque},
 };
 use core::{
     ffi::{c_int, c_long, c_uint, c_ulong},
@@ -49,24 +49,30 @@ pub const fn into_raw<T: MiscDevice>(self) -> bindings::miscdevice {
 /// # Invariants
 ///
 /// `inner` is a registered misc device.
-#[repr(transparent)]
+#[repr(C)]
 #[pin_data(PinnedDrop)]
-pub struct MiscDeviceRegistration<T> {
+pub struct MiscDeviceRegistration<T: MiscDevice> {
     #[pin]
     inner: Opaque<bindings::miscdevice>,
+    #[pin]
+    data: Aliased<T::RegistrationData>,
     _t: PhantomData<T>,
 }
 
 // SAFETY: It is allowed to call `misc_deregister` on a different thread from where you called
 // `misc_register`.
-unsafe impl<T> Send for MiscDeviceRegistration<T> {}
+unsafe impl<T: MiscDevice<RegistrationData: Send>> Send for MiscDeviceRegistration<T> {}
 // SAFETY: All `&self` methods on this type are written to ensure that it is safe to call them in
 // parallel.
-unsafe impl<T> Sync for MiscDeviceRegistration<T> {}
+// MiscDevice::RegistrationData is always Sync.
+unsafe impl<T: MiscDevice> Sync for MiscDeviceRegistration<T> {}
 
 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,
         })
     }
@@ -97,10 +104,18 @@ pub fn device(&self) -> &Device {
         // before the underlying `struct miscdevice` is destroyed.
         unsafe { Device::as_ref((*self.as_raw()).this_device) }
     }
+
+    /// Access the additional data stored in this registration.
+    pub fn data(&self) -> &T::RegistrationData {
+        // SAFETY:
+        // No mutable reference to the value contained by self.data can ever be created.
+        // The value contained by self.data is valid for the entire lifetime of self.
+        unsafe { &*self.data.get() }
+    }
 }
 
 #[pinned_drop]
-impl<T> PinnedDrop for MiscDeviceRegistration<T> {
+impl<T: MiscDevice> PinnedDrop for MiscDeviceRegistration<T> {
     fn drop(self: Pin<&mut Self>) {
         // SAFETY: We know that the device is registered by the type invariants.
         unsafe { bindings::misc_deregister(self.inner.get()) };
@@ -113,6 +128,11 @@ pub trait MiscDevice: Sized {
     /// What kind of pointer should `Self` be wrapped in.
     type Ptr: ForeignOwnable + Send + Sync;
 
+    /// The additional data carried by the `MiscDeviceRegistration` for this `MiscDevice`.
+    /// If no additional data is required than () should be used.
+    /// This data can be accessed in `open()` using `MiscDeviceRegistration::data()`.
+    type RegistrationData: Sync;
+
     /// Called when the misc device is opened.
     ///
     /// The returned pointer will be stored as the private data for the file.
@@ -218,6 +238,9 @@ impl<T: MiscDevice> VtableHelper<T> {
     // SAFETY: This is a miscdevice, so `misc_open()` set the private data to a pointer to the
     // associated `struct miscdevice` before calling into this method. Furthermore, `misc_open()`
     // ensures that the miscdevice can't be unregistered and freed during this call to `fops_open`.
+    // Since this the `MiscDeviceRegistration` struct uses `#[repr(C)]` and the miscdevice is the
+    // first entry it is guaranteed that the address of the miscdevice is the same as the address
+    // of the entire `MiscDeviceRegistration` struct.
     let misc = unsafe { &*misc_ptr.cast::<MiscDeviceRegistration<T>>() };
 
     // SAFETY:
diff --git a/samples/rust/rust_misc_device.rs b/samples/rust/rust_misc_device.rs
index 40ad7266c2252e5c0b4e91e501ef9ada2eda3b16..779fcfd64119bdd5b4f8be740f7e8336c652b4d3 100644
--- a/samples/rust/rust_misc_device.rs
+++ b/samples/rust/rust_misc_device.rs
@@ -136,7 +136,7 @@ fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
         };
 
         try_pin_init!(Self {
-            _miscdev <- MiscDeviceRegistration::register(options),
+            _miscdev <- MiscDeviceRegistration::register(options, ()),
         })
     }
 }
@@ -156,6 +156,8 @@ struct RustMiscDevice {
 impl MiscDevice for RustMiscDevice {
     type Ptr = Pin<KBox<Self>>;
 
+    type RegistrationData = ();
+
     fn open(_file: &File, misc: &MiscDeviceRegistration<Self>) -> Result<Pin<KBox<Self>>> {
         let dev = ARef::from(misc.device());
 

-- 
2.48.1

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? 

>      }
> @@ -97,10 +104,18 @@ pub fn device(&self) -> &Device {
>          // before the underlying `struct miscdevice` is destroyed.
>          unsafe { Device::as_ref((*self.as_raw()).this_device) }
>      }
> +
> +    /// Access the additional data stored in this registration.
> +    pub fn data(&self) -> &T::RegistrationData {
> +        // SAFETY:
> +        // No mutable reference to the value contained by self.data can ever be created.
> +        // The value contained by self.data is valid for the entire lifetime of self.
> +        unsafe { &*self.data.get() }
> +    }
>  }
>  
>  #[pinned_drop]
> -impl<T> PinnedDrop for MiscDeviceRegistration<T> {
> +impl<T: MiscDevice> PinnedDrop for MiscDeviceRegistration<T> {
>      fn drop(self: Pin<&mut Self>) {
>          // SAFETY: We know that the device is registered by the type invariants.
>          unsafe { bindings::misc_deregister(self.inner.get()) };
> @@ -113,6 +128,11 @@ pub trait MiscDevice: Sized {
>      /// What kind of pointer should `Self` be wrapped in.
>      type Ptr: ForeignOwnable + Send + Sync;
>  
> +    /// The additional data carried by the `MiscDeviceRegistration` for this `MiscDevice`.
> +    /// If no additional data is required than () should be used.
> +    /// This data can be accessed in `open()` using `MiscDeviceRegistration::data()`.
> +    type RegistrationData: Sync;
> +
>      /// Called when the misc device is opened.
>      ///
>      /// The returned pointer will be stored as the private data for the file.
> @@ -218,6 +238,9 @@ impl<T: MiscDevice> VtableHelper<T> {
>      // SAFETY: This is a miscdevice, so `misc_open()` set the private data to a pointer to the
>      // associated `struct miscdevice` before calling into this method. Furthermore, `misc_open()`
>      // ensures that the miscdevice can't be unregistered and freed during this call to `fops_open`.
> +    // Since this the `MiscDeviceRegistration` struct uses `#[repr(C)]` and the miscdevice is the
> +    // first entry it is guaranteed that the address of the miscdevice is the same as the address
> +    // of the entire `MiscDeviceRegistration` struct.
>      let misc = unsafe { &*misc_ptr.cast::<MiscDeviceRegistration<T>>() };
>  
>      // SAFETY:
> diff --git a/samples/rust/rust_misc_device.rs b/samples/rust/rust_misc_device.rs
> index 40ad7266c2252e5c0b4e91e501ef9ada2eda3b16..779fcfd64119bdd5b4f8be740f7e8336c652b4d3 100644
> --- a/samples/rust/rust_misc_device.rs
> +++ b/samples/rust/rust_misc_device.rs
> @@ -136,7 +136,7 @@ fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
>          };
>  
>          try_pin_init!(Self {
> -            _miscdev <- MiscDeviceRegistration::register(options),
> +            _miscdev <- MiscDeviceRegistration::register(options, ()),
>          })
>      }
>  }
> @@ -156,6 +156,8 @@ struct RustMiscDevice {
>  impl MiscDevice for RustMiscDevice {
>      type Ptr = Pin<KBox<Self>>;
>  
> +    type RegistrationData = ();
> +
>      fn open(_file: &File, misc: &MiscDeviceRegistration<Self>) -> Result<Pin<KBox<Self>>> {
>          let dev = ARef::from(misc.device());
>  
>

On Fri, Jan 24, 2025 at 12:26 AM Christian Schrefl
<chrisi.schrefl@gmail.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

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

On Mon, Jan 27, 2025 at 2:27 PM Christian Schrefl
<chrisi.schrefl@gmail.com> wrote:
>
> 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.

Creating a reference immediately after misc_register finishes should
be fine. I think that warning is more strict than necessary.

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

Yep. On failure, previous fields are cleaned up.

> 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.

You can expand Rust macros by invoking make on the file with an .i
extension, in exactly the same way as you expand C macros.

Alice

On Mon, Jan 27, 2025 at 2:33 PM Alice Ryhl <aliceryhl@google.com> wrote:
>
> You can expand Rust macros by invoking make on the file with an .i
> extension, in exactly the same way as you expand C macros.

`.i` for C, `.rsi` for Rust.

Also, Christian: the Rust one currently it works for leaves, e.g.
drivers and samples:

    make LLVM=1 samples/rust/rust_minimal.rsi

Eventually we will add support for others.

I hope that helps!

Cheers,
Miguel

On 27.01.25 2:33 PM, Alice Ryhl wrote:
> On Mon, Jan 27, 2025 at 2:27 PM Christian Schrefl
> <chrisi.schrefl@gmail.com> wrote:
>>
>> 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.
> 
> Creating a reference immediately after misc_register finishes should
> be fine. I think that warning is more strict than necessary.
> 
>> Also does `try_pin_init!()` automatically drop `data` when `bindings::misc_register` fails?
> 
> Yep. On failure, previous fields are cleaned up.
> 
>> 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.
> 
> You can expand Rust macros by invoking make on the file with an .i
> extension, in exactly the same way as you expand C macros.

Thanks!

On Sun, Jan 19, 2025 at 11:11:14PM +0100, Christian Schrefl wrote:
> When using the Rust miscdevice bindings, you generally embed the
> MiscDeviceRegistration within another struct:
> 
> struct MyDriverData {
>     data: SomeOtherData,
>     misc: MiscDeviceRegistration<MyMiscFile>
> }
> 
> In the `fops->open` callback of the miscdevice, you are given a
> reference to the registration, which allows you to access its fields.
> For example, as of commit 284ae0be4dca ("rust: miscdevice: Provide
> accessor to pull out miscdevice::this_device") you can access the
> internal `struct device`. However, there is still no way to access the
> `data` field in the above example, because you only have a reference to
> the registration.

What's wrong with the driver_data pointer in the misc device structure?
Shouldn't you be in control of that as you are a misc driver owner?  Or
does the misc core handle this I can't recall at the moment, sorry.

> Using container_of is also not possible to do safely. For example, if
> the destructor of `MyDriverData` runs, then the destructor of `data`
> would run before the miscdevice is deregistered, so using container_of
> to access `data` from `fops->open` could result in a UAF. A similar
> problem can happen on initialization if `misc` is not the last field to
> be initialized.
> 
> To provide a safe way to access user-defined data stored next to the
> `struct miscdevice`, make `MiscDeviceRegistration` into a container that
> can store a user-provided piece of data. This way, `fops->open` can
> access that data via the registration, since the data is stored inside
> the registration.

"next to" feels odd, that's what a container_of is for, but be careful
as to who owns the lifecycle of the object you are trying to get to.
You can't have multiple objects with different lifecycles in the same
structure (i.e. don't mix a misc device and a platform device together).

So a real example here would be good to see, can you post your driver at
the same time so that we can see what you are doing and perhaps provide
a better way to do it?

thanks,

greg k-h

On 22.01.25 10:28 AM, Greg Kroah-Hartman wrote:
> On Sun, Jan 19, 2025 at 11:11:14PM +0100, Christian Schrefl wrote:
>> When using the Rust miscdevice bindings, you generally embed the
>> MiscDeviceRegistration within another struct:
>>
>> struct MyDriverData {
>>     data: SomeOtherData,
>>     misc: MiscDeviceRegistration<MyMiscFile>
>> }
>>
>> In the `fops->open` callback of the miscdevice, you are given a
>> reference to the registration, which allows you to access its fields.
>> For example, as of commit 284ae0be4dca ("rust: miscdevice: Provide
>> accessor to pull out miscdevice::this_device") you can access the
>> internal `struct device`. However, there is still no way to access the
>> `data` field in the above example, because you only have a reference to
>> the registration.
> 
> What's wrong with the driver_data pointer in the misc device structure?
> Shouldn't you be in control of that as you are a misc driver owner?  Or
> does the misc core handle this I can't recall at the moment, sorry.


I don't know the internals of (C) miscdevice good enough to know where I'm
allowed to store something, since there is no private_data field.

Not sure how the lifetimes of the whole device and device->driver_data are.
But even that instead we use that we will need a rust abstraction for that to
allow safe drivers.

> 
>> Using container_of is also not possible to do safely. For example, if
>> the destructor of `MyDriverData` runs, then the destructor of `data`
>> would run before the miscdevice is deregistered, so using container_of
>> to access `data` from `fops->open` could result in a UAF. A similar
>> problem can happen on initialization if `misc` is not the last field to
>> be initialized.
>>
>> To provide a safe way to access user-defined data stored next to the
>> `struct miscdevice`, make `MiscDeviceRegistration` into a container that
>> can store a user-provided piece of data. This way, `fops->open` can
>> access that data via the registration, since the data is stored inside
>> the registration.
> 
> "next to" feels odd, that's what a container_of is for, but be careful
> as to who owns the lifecycle of the object you are trying to get to.
> You can't have multiple objects with different lifecycles in the same
> structure (i.e. don't mix a misc device and a platform device together).
> 
> So a real example here would be good to see, can you post your driver at
> the same time so that we can see what you are doing and perhaps provide
> a better way to do it?


The `struct miscdevice` is currently the first item in the 
`MiscDeviceRegistration` so the  `struct miscdevice` and the 
`MiscDeviceRegistration` have the same address.
I can use container_of! if people think that more understandable.

> 
> thanks,
> 
> greg k-h

On Thu, Jan 23, 2025 at 04:52:26PM +0100, Christian Schrefl wrote:
> 
> 
> On 22.01.25 10:28 AM, Greg Kroah-Hartman wrote:
> > On Sun, Jan 19, 2025 at 11:11:14PM +0100, Christian Schrefl wrote:
> >> When using the Rust miscdevice bindings, you generally embed the
> >> MiscDeviceRegistration within another struct:
> >>
> >> struct MyDriverData {
> >>     data: SomeOtherData,
> >>     misc: MiscDeviceRegistration<MyMiscFile>
> >> }
> >>
> >> In the `fops->open` callback of the miscdevice, you are given a
> >> reference to the registration, which allows you to access its fields.
> >> For example, as of commit 284ae0be4dca ("rust: miscdevice: Provide
> >> accessor to pull out miscdevice::this_device") you can access the
> >> internal `struct device`. However, there is still no way to access the
> >> `data` field in the above example, because you only have a reference to
> >> the registration.
> > 
> > What's wrong with the driver_data pointer in the misc device structure?
> > Shouldn't you be in control of that as you are a misc driver owner?  Or
> > does the misc core handle this I can't recall at the moment, sorry.
> 
> 
> I don't know the internals of (C) miscdevice good enough to know where I'm
> allowed to store something, since there is no private_data field.

You are right, I was wrong here, sorry.  A misc device either needs to
be "stand alone" or embedded into something else.

> Not sure how the lifetimes of the whole device and device->driver_data are.
> But even that instead we use that we will need a rust abstraction for that to
> allow safe drivers.

Agreed, so let's make it work properly :)

> > 
> >> Using container_of is also not possible to do safely. For example, if
> >> the destructor of `MyDriverData` runs, then the destructor of `data`
> >> would run before the miscdevice is deregistered, so using container_of
> >> to access `data` from `fops->open` could result in a UAF. A similar
> >> problem can happen on initialization if `misc` is not the last field to
> >> be initialized.
> >>
> >> To provide a safe way to access user-defined data stored next to the
> >> `struct miscdevice`, make `MiscDeviceRegistration` into a container that
> >> can store a user-provided piece of data. This way, `fops->open` can
> >> access that data via the registration, since the data is stored inside
> >> the registration.
> > 
> > "next to" feels odd, that's what a container_of is for, but be careful
> > as to who owns the lifecycle of the object you are trying to get to.
> > You can't have multiple objects with different lifecycles in the same
> > structure (i.e. don't mix a misc device and a platform device together).
> > 
> > So a real example here would be good to see, can you post your driver at
> > the same time so that we can see what you are doing and perhaps provide
> > a better way to do it?
> 
> 
> The `struct miscdevice` is currently the first item in the 
> `MiscDeviceRegistration` so the  `struct miscdevice` and the 
> `MiscDeviceRegistration` have the same address.
> I can use container_of! if people think that more understandable.

You always have to use container_of! in case things move around.  If the
location is the same place, then the compiler just optimizes it all away
and doesn't do any pointer math so it's fine.

thanks,

greg k-h

On 23.01.25 5:00 PM, Greg Kroah-Hartman wrote:
> On Thu, Jan 23, 2025 at 04:52:26PM +0100, Christian Schrefl wrote:
>>
>>
>> On 22.01.25 10:28 AM, Greg Kroah-Hartman wrote:
>>> On Sun, Jan 19, 2025 at 11:11:14PM +0100, Christian Schrefl wrote:
>>>> When using the Rust miscdevice bindings, you generally embed the
>>>> MiscDeviceRegistration within another struct:
>>>>
>>>> struct MyDriverData {
>>>>     data: SomeOtherData,
>>>>     misc: MiscDeviceRegistration<MyMiscFile>
>>>> }
>>>>
>>>> In the `fops->open` callback of the miscdevice, you are given a
>>>> reference to the registration, which allows you to access its fields.
>>>> For example, as of commit 284ae0be4dca ("rust: miscdevice: Provide
>>>> accessor to pull out miscdevice::this_device") you can access the
>>>> internal `struct device`. However, there is still no way to access the
>>>> `data` field in the above example, because you only have a reference to
>>>> the registration.
>>>
>>> What's wrong with the driver_data pointer in the misc device structure?
>>> Shouldn't you be in control of that as you are a misc driver owner?  Or
>>> does the misc core handle this I can't recall at the moment, sorry.
>>
>>
>> I don't know the internals of (C) miscdevice good enough to know where I'm
>> allowed to store something, since there is no private_data field.
> 
> You are right, I was wrong here, sorry.  A misc device either needs to
> be "stand alone" or embedded into something else.

The struct miscdevice is "embedded" in the Rust MiscDeviceRegistration, 
so it should be fine if I understand you correctly.

> 
>> Not sure how the lifetimes of the whole device and device->driver_data are.
>> But even that instead we use that we will need a rust abstraction for that to
>> allow safe drivers.
> 
> Agreed, so let's make it work properly :)

So keep the current approach?

> 
>>>
>>>> Using container_of is also not possible to do safely. For example, if
>>>> the destructor of `MyDriverData` runs, then the destructor of `data`
>>>> would run before the miscdevice is deregistered, so using container_of
>>>> to access `data` from `fops->open` could result in a UAF. A similar
>>>> problem can happen on initialization if `misc` is not the last field to
>>>> be initialized.
>>>>
>>>> To provide a safe way to access user-defined data stored next to the
>>>> `struct miscdevice`, make `MiscDeviceRegistration` into a container that
>>>> can store a user-provided piece of data. This way, `fops->open` can
>>>> access that data via the registration, since the data is stored inside
>>>> the registration.
>>>
>>> "next to" feels odd, that's what a container_of is for, but be careful
>>> as to who owns the lifecycle of the object you are trying to get to.
>>> You can't have multiple objects with different lifecycles in the same
>>> structure (i.e. don't mix a misc device and a platform device together).
>>>
>>> So a real example here would be good to see, can you post your driver at
>>> the same time so that we can see what you are doing and perhaps provide
>>> a better way to do it?
>>
>>
>> The `struct miscdevice` is currently the first item in the 
>> `MiscDeviceRegistration` so the  `struct miscdevice` and the 
>> `MiscDeviceRegistration` have the same address.
>> I can use container_of! if people think that more understandable.
> 
> You always have to use container_of! in case things move around.  If the
> location is the same place, then the compiler just optimizes it all away
> and doesn't do any pointer math so it's fine.

Sure I'll change it to use container_of.

> 
> thanks,
> 
> greg k-h

On Wed, Jan 22, 2025 at 10:28 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Sun, Jan 19, 2025 at 11:11:14PM +0100, Christian Schrefl wrote:
> > When using the Rust miscdevice bindings, you generally embed the
> > MiscDeviceRegistration within another struct:
> >
> > struct MyDriverData {
> >     data: SomeOtherData,
> >     misc: MiscDeviceRegistration<MyMiscFile>
> > }
> >
> > In the `fops->open` callback of the miscdevice, you are given a
> > reference to the registration, which allows you to access its fields.
> > For example, as of commit 284ae0be4dca ("rust: miscdevice: Provide
> > accessor to pull out miscdevice::this_device") you can access the
> > internal `struct device`. However, there is still no way to access the
> > `data` field in the above example, because you only have a reference to
> > the registration.
>
> What's wrong with the driver_data pointer in the misc device structure?
> Shouldn't you be in control of that as you are a misc driver owner?  Or
> does the misc core handle this I can't recall at the moment, sorry.

There's no such pointer in struct miscdevice?

> > Using container_of is also not possible to do safely. For example, if
> > the destructor of `MyDriverData` runs, then the destructor of `data`
> > would run before the miscdevice is deregistered, so using container_of
> > to access `data` from `fops->open` could result in a UAF. A similar
> > problem can happen on initialization if `misc` is not the last field to
> > be initialized.
> >
> > To provide a safe way to access user-defined data stored next to the
> > `struct miscdevice`, make `MiscDeviceRegistration` into a container that
> > can store a user-provided piece of data. This way, `fops->open` can
> > access that data via the registration, since the data is stored inside
> > the registration.
>
> "next to" feels odd, that's what a container_of is for, but be careful
> as to who owns the lifecycle of the object you are trying to get to.
> You can't have multiple objects with different lifecycles in the same
> structure (i.e. don't mix a misc device and a platform device together).

Ultimately, this is intended to solve the problem that container_of
solves in C. Actually using container_of runs into the challenge that
you have no way of knowing if those other fields are still valid. If
the destructor of the struct starts running, then it might have
destroyed some of the fields, but not yet have destroyed the
miscdevice field. Since you can't know if the rest of the struct is
still valid, it's not safe to use container_of.

Storing those other fields within the registration solves this issue.
The registration ensures that the miscdevice is deregistered before
the `data` field is destroyed. This means that if it's safe to access
the miscdevice field, then it's also safe to access the `data` field,
and that's the guarantee we need.

Alice

> So a real example here would be good to see, can you post your driver at
> the same time so that we can see what you are doing and perhaps provide
> a better way to do it?

On Wed, Jan 22, 2025 at 11:11:07AM +0100, Alice Ryhl wrote:
> On Wed, Jan 22, 2025 at 10:28 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Sun, Jan 19, 2025 at 11:11:14PM +0100, Christian Schrefl wrote:
> > > When using the Rust miscdevice bindings, you generally embed the
> > > MiscDeviceRegistration within another struct:
> > >
> > > struct MyDriverData {
> > >     data: SomeOtherData,
> > >     misc: MiscDeviceRegistration<MyMiscFile>
> > > }
> > >
> > > In the `fops->open` callback of the miscdevice, you are given a
> > > reference to the registration, which allows you to access its fields.
> > > For example, as of commit 284ae0be4dca ("rust: miscdevice: Provide
> > > accessor to pull out miscdevice::this_device") you can access the
> > > internal `struct device`. However, there is still no way to access the
> > > `data` field in the above example, because you only have a reference to
> > > the registration.
> >
> > What's wrong with the driver_data pointer in the misc device structure?
> > Shouldn't you be in control of that as you are a misc driver owner?  Or
> > does the misc core handle this I can't recall at the moment, sorry.
> 
> There's no such pointer in struct miscdevice?

struct miscdevice->struct device->driver_data;

But in digging, this might not be "allowed" for a misc driver to do, I
can't remember anymore, sorry.

> > > Using container_of is also not possible to do safely. For example, if
> > > the destructor of `MyDriverData` runs, then the destructor of `data`
> > > would run before the miscdevice is deregistered, so using container_of
> > > to access `data` from `fops->open` could result in a UAF. A similar
> > > problem can happen on initialization if `misc` is not the last field to
> > > be initialized.
> > >
> > > To provide a safe way to access user-defined data stored next to the
> > > `struct miscdevice`, make `MiscDeviceRegistration` into a container that
> > > can store a user-provided piece of data. This way, `fops->open` can
> > > access that data via the registration, since the data is stored inside
> > > the registration.
> >
> > "next to" feels odd, that's what a container_of is for, but be careful
> > as to who owns the lifecycle of the object you are trying to get to.
> > You can't have multiple objects with different lifecycles in the same
> > structure (i.e. don't mix a misc device and a platform device together).
> 
> Ultimately, this is intended to solve the problem that container_of
> solves in C. Actually using container_of runs into the challenge that
> you have no way of knowing if those other fields are still valid.

You "know" they are valid if you have a pointer to your structure,
right?  Someone sent that to you, so by virtue of that it has to be
valid.

> If
> the destructor of the struct starts running, then it might have
> destroyed some of the fields, but not yet have destroyed the
> miscdevice field. Since you can't know if the rest of the struct is
> still valid, it's not safe to use container_of.

That's a different issue, that's a lifetime issue of "can I look at
these other fields at this point in time and trust them", it's not a
"these are not valid thing to look at".

> Storing those other fields within the registration solves this issue.
> The registration ensures that the miscdevice is deregistered before
> the `data` field is destroyed. This means that if it's safe to access
> the miscdevice field, then it's also safe to access the `data` field,
> and that's the guarantee we need.

I'm confused.  A misc device should be embedded inside of something
else.  And the misc device controls the lifespan of that "something
else" structure, right?  So by virtue of the misc device being alive,
everything else in there should be ok.

Now individual misc devices might want to do different things but if
they are being torn down, that means all references are dropped so any
"container_of()" like cast-back should not even be possible as there
should not be a pointer that you can cast-back from here.

Or am I confused?  I think a real example would be good to see here.

thanks,

greg k-h

On Wed, Jan 22, 2025 at 1:40 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Wed, Jan 22, 2025 at 11:11:07AM +0100, Alice Ryhl wrote:
> > On Wed, Jan 22, 2025 at 10:28 AM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Sun, Jan 19, 2025 at 11:11:14PM +0100, Christian Schrefl wrote:
> > > > When using the Rust miscdevice bindings, you generally embed the
> > > > MiscDeviceRegistration within another struct:
> > > >
> > > > struct MyDriverData {
> > > >     data: SomeOtherData,
> > > >     misc: MiscDeviceRegistration<MyMiscFile>
> > > > }
> > > >
> > > > In the `fops->open` callback of the miscdevice, you are given a
> > > > reference to the registration, which allows you to access its fields.
> > > > For example, as of commit 284ae0be4dca ("rust: miscdevice: Provide
> > > > accessor to pull out miscdevice::this_device") you can access the
> > > > internal `struct device`. However, there is still no way to access the
> > > > `data` field in the above example, because you only have a reference to
> > > > the registration.
> > >
> > > What's wrong with the driver_data pointer in the misc device structure?
> > > Shouldn't you be in control of that as you are a misc driver owner?  Or
> > > does the misc core handle this I can't recall at the moment, sorry.
> >
> > There's no such pointer in struct miscdevice?
>
> struct miscdevice->struct device->driver_data;
>
> But in digging, this might not be "allowed" for a misc driver to do, I
> can't remember anymore, sorry.
>
> > > > Using container_of is also not possible to do safely. For example, if
> > > > the destructor of `MyDriverData` runs, then the destructor of `data`
> > > > would run before the miscdevice is deregistered, so using container_of
> > > > to access `data` from `fops->open` could result in a UAF. A similar
> > > > problem can happen on initialization if `misc` is not the last field to
> > > > be initialized.
> > > >
> > > > To provide a safe way to access user-defined data stored next to the
> > > > `struct miscdevice`, make `MiscDeviceRegistration` into a container that
> > > > can store a user-provided piece of data. This way, `fops->open` can
> > > > access that data via the registration, since the data is stored inside
> > > > the registration.
> > >
> > > "next to" feels odd, that's what a container_of is for, but be careful
> > > as to who owns the lifecycle of the object you are trying to get to.
> > > You can't have multiple objects with different lifecycles in the same
> > > structure (i.e. don't mix a misc device and a platform device together).
> >
> > Ultimately, this is intended to solve the problem that container_of
> > solves in C. Actually using container_of runs into the challenge that
> > you have no way of knowing if those other fields are still valid.
>
> You "know" they are valid if you have a pointer to your structure,
> right?  Someone sent that to you, so by virtue of that it has to be
> valid.
>
> > If
> > the destructor of the struct starts running, then it might have
> > destroyed some of the fields, but not yet have destroyed the
> > miscdevice field. Since you can't know if the rest of the struct is
> > still valid, it's not safe to use container_of.
>
> That's a different issue, that's a lifetime issue of "can I look at
> these other fields at this point in time and trust them", it's not a
> "these are not valid thing to look at".

The memory containing the field can't have been deallocated yet, but
if we provide a safe way to get access to those fields after their
destructor runs, then that's still a problem. If there's a field of
type `KBox<MyType>`, i.e. a pointer to a kmalloc allocation, then the
destructor will have freed that allocation. We cannot let you access
the field that holds the KBox after that happens because the pointer
will be dangling.

> > Storing those other fields within the registration solves this issue.
> > The registration ensures that the miscdevice is deregistered before
> > the `data` field is destroyed. This means that if it's safe to access
> > the miscdevice field, then it's also safe to access the `data` field,
> > and that's the guarantee we need.
>
> I'm confused.  A misc device should be embedded inside of something
> else.  And the misc device controls the lifespan of that "something
> else" structure, right?  So by virtue of the misc device being alive,
> everything else in there should be ok.

This patch proposes that the way you write

struct MyDriverData {
    // lifetime of these fields is controlled by misc
    field1: Foo,
    field2: Bar,
    misc: MiscDeviceRegistration<MyMiscFile>
}

is

struct MyDriverData {
    misc: MiscDeviceRegistration<MyMiscFile, Inner>
}
struct Inner {
    // lifetime of these fields is controlled by misc
    field1: Foo,
    field2: Bar,
}

in memory all three fields gets laid out next to each other.

> Now individual misc devices might want to do different things but if
> they are being torn down, that means all references are dropped so any
> "container_of()" like cast-back should not even be possible as there
> should not be a pointer that you can cast-back from here.
>
> Or am I confused?  I think a real example would be good to see here.

Christian, do you mind sharing the example you showed to me?

Alice

On 22.01.25 2:06 PM, Alice Ryhl wrote:
> On Wed, Jan 22, 2025 at 1:40 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
>>
>> On Wed, Jan 22, 2025 at 11:11:07AM +0100, Alice Ryhl wrote:
>>> On Wed, Jan 22, 2025 at 10:28 AM Greg Kroah-Hartman
>>> <gregkh@linuxfoundation.org> wrote:
>>>>
>>>> On Sun, Jan 19, 2025 at 11:11:14PM +0100, Christian Schrefl wrote:
>>>>> When using the Rust miscdevice bindings, you generally embed the
>>>>> MiscDeviceRegistration within another struct:
>>>>>
>>>>> struct MyDriverData {
>>>>>     data: SomeOtherData,
>>>>>     misc: MiscDeviceRegistration<MyMiscFile>
>>>>> }
>>>>>
>>>>> In the `fops->open` callback of the miscdevice, you are given a
>>>>> reference to the registration, which allows you to access its fields.
>>>>> For example, as of commit 284ae0be4dca ("rust: miscdevice: Provide
>>>>> accessor to pull out miscdevice::this_device") you can access the
>>>>> internal `struct device`. However, there is still no way to access the
>>>>> `data` field in the above example, because you only have a reference to
>>>>> the registration.
>>>>
>>>> What's wrong with the driver_data pointer in the misc device structure?
>>>> Shouldn't you be in control of that as you are a misc driver owner?  Or
>>>> does the misc core handle this I can't recall at the moment, sorry.
>>>
>>> There's no such pointer in struct miscdevice?
>>
>> struct miscdevice->struct device->driver_data;
>>
>> But in digging, this might not be "allowed" for a misc driver to do, I
>> can't remember anymore, sorry.
>>
>>>>> Using container_of is also not possible to do safely. For example, if
>>>>> the destructor of `MyDriverData` runs, then the destructor of `data`
>>>>> would run before the miscdevice is deregistered, so using container_of
>>>>> to access `data` from `fops->open` could result in a UAF. A similar
>>>>> problem can happen on initialization if `misc` is not the last field to
>>>>> be initialized.
>>>>>
>>>>> To provide a safe way to access user-defined data stored next to the
>>>>> `struct miscdevice`, make `MiscDeviceRegistration` into a container that
>>>>> can store a user-provided piece of data. This way, `fops->open` can
>>>>> access that data via the registration, since the data is stored inside
>>>>> the registration.
>>>>
>>>> "next to" feels odd, that's what a container_of is for, but be careful
>>>> as to who owns the lifecycle of the object you are trying to get to.
>>>> You can't have multiple objects with different lifecycles in the same
>>>> structure (i.e. don't mix a misc device and a platform device together).
>>>
>>> Ultimately, this is intended to solve the problem that container_of
>>> solves in C. Actually using container_of runs into the challenge that
>>> you have no way of knowing if those other fields are still valid.
>>
>> You "know" they are valid if you have a pointer to your structure,
>> right?  Someone sent that to you, so by virtue of that it has to be
>> valid.
>>
>>> If
>>> the destructor of the struct starts running, then it might have
>>> destroyed some of the fields, but not yet have destroyed the
>>> miscdevice field. Since you can't know if the rest of the struct is
>>> still valid, it's not safe to use container_of.
>>
>> That's a different issue, that's a lifetime issue of "can I look at
>> these other fields at this point in time and trust them", it's not a
>> "these are not valid thing to look at".
> 
> The memory containing the field can't have been deallocated yet, but
> if we provide a safe way to get access to those fields after their
> destructor runs, then that's still a problem. If there's a field of
> type `KBox<MyType>`, i.e. a pointer to a kmalloc allocation, then the
> destructor will have freed that allocation. We cannot let you access
> the field that holds the KBox after that happens because the pointer
> will be dangling.
> 
>>> Storing those other fields within the registration solves this issue.
>>> The registration ensures that the miscdevice is deregistered before
>>> the `data` field is destroyed. This means that if it's safe to access
>>> the miscdevice field, then it's also safe to access the `data` field,
>>> and that's the guarantee we need.
>>
>> I'm confused.  A misc device should be embedded inside of something
>> else.  And the misc device controls the lifespan of that "something
>> else" structure, right?  So by virtue of the misc device being alive,
>> everything else in there should be ok.
> 
> This patch proposes that the way you write
> 
> struct MyDriverData {
>     // lifetime of these fields is controlled by misc
>     field1: Foo,
>     field2: Bar,
>     misc: MiscDeviceRegistration<MyMiscFile>
> }
> 
> is
> 
> struct MyDriverData {
>     misc: MiscDeviceRegistration<MyMiscFile, Inner>
> }
> struct Inner {
>     // lifetime of these fields is controlled by misc
>     field1: Foo,
>     field2: Bar,
> }
> 
> in memory all three fields gets laid out next to each other.
> 
>> Now individual misc devices might want to do different things but if
>> they are being torn down, that means all references are dropped so any
>> "container_of()" like cast-back should not even be possible as there
>> should not be a pointer that you can cast-back from here.
>>
>> Or am I confused?  I think a real example would be good to see here.
> 
> Christian, do you mind sharing the example you showed to me?

Sure

#[pin_data(PinnedDrop)]
struct LedPwmDriver {
    pdev: platform::Device,
    mapping: Devres<IoMem<MAPPING_SIZE, true>>,
    #[pin]
    miscdev: MiscDeviceRegistration<RustMiscDevice>,
}

impl platform::Driver for LedPwmDriver {
    type IdInfo = ();
    const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>> = Some(&OF_TABLE);
    fn probe(pdev: &mut platform::Device, info: Option<&Self::IdInfo>) -> Result<Pin<KBox<Self>>> {
        dev_dbg!(pdev.as_ref(), "Probe Rust Platform driver sample.\n");
        if info.is_some() {
            dev_info!(pdev.as_ref(), "Probed with info\n");
        }
        let mapping = pdev
            .ioremap_resource_sized::<MAPPING_SIZE, true>(pdev.resource(0).ok_or(ENXIO)?)
            .map_err(|_| ENXIO)?;

        // Initialize the HW
        mapping.try_access().ok_or(ENXIO)?.writel(0xFF, 0x0);

        let options = MiscDeviceOptions { name: c_str!("led0") };
        let drvdata = KBox::try_pin_init(
            try_pin_init!(Self {
                pdev: pdev.clone(),
                mapping,
                miscdev <- MiscDeviceRegistration::register(options),
            }),
            GFP_KERNEL,
        )?;
        Ok(drvdata)
    }
}

#[pinned_drop]
impl PinnedDrop for LedPwmDriver {
    fn drop(self: Pin<&mut Self>) {
        dev_info!(self.pdev.as_ref(), "Remove Rust Platform driver sample.\n");

        if let Some(res) = self.mapping.try_access() {
            res.writel(0x00, 0x0);
        }
    }
}

#[pin_data(PinnedDrop)]
struct RustMiscDevice {
    dev: ARef<Device>,
    mapping: &'static Devres<IoMem<MAPPING_SIZE, true>>,
}
#[vtable]
impl MiscDevice for RustMiscDevice {
    type Ptr = Pin<KBox<Self>>;
    fn open(_file: &File, misc: &MiscDeviceRegistration<Self>) -> Result<Pin<KBox<Self>>> {
        let dev = ARef::from(misc.device());
        // SAFETY:
        // Stuff?
        let ledpwm = unsafe { &*container_of!(misc, LedPwmDriver, miscdev) };
        dev_info!(dev, "Opening Rust Misc Device Sample\n");
        let res = ledpwm.mapping.try_access().ok_or(ENXIO)?;
        res.writel(0x80, 0x0);
        KBox::try_pin_init(
            try_pin_init! {
                RustMiscDevice {
                    dev: dev,
                    mapping: &ledpwm.mapping
                }
            },
            GFP_KERNEL,
        )
    }
}

#[pinned_drop]
impl PinnedDrop for RustMiscDevice {
    fn drop(self: Pin<&mut Self>) {
        dev_info!(self.dev, "Exiting the Rust Misc Device Sample\n");

        if let Some(res) = self.mapping.try_access() {
            res.writel(0x10, 0x0);
        }
    }
}


I removed or simplified many irrelevant parts, the full driver can be found here:

https://github.com/onestacked/linux/blob/c9e2ef858e4537d33625cdf2b5d20ef4acfa9424/drivers/misc/ledpwm.rs

This driver was only an attempt to the assignment of a University course in Rust mostly to see if was 
possible or which abstractions are missing.

The driver using the patch can be found here:
https://github.com/onestacked/linux/blob/rust_ledpwm_driver/drivers/misc/ledpwm.rs


Cheers
Christian

Hi Christian,

kernel test robot noticed the following build errors:

[auto build test ERROR on 01b3cb620815fc3feb90ee117d9445a5b608a9f7]

url:    https://github.com/intel-lab-lkp/linux/commits/Christian-Schrefl/rust-add-Aliased-type/20250120-061340
base:   01b3cb620815fc3feb90ee117d9445a5b608a9f7
patch link:    https://lore.kernel.org/r/20250119-b4-rust_miscdevice_registrationdata-v1-2-edbf18dde5fc%40gmail.com
patch subject: [PATCH 2/3] rust: miscdevice: Add additional data to MiscDeviceRegistration
config: x86_64-rhel-9.4-rust (https://download.01.org/0day-ci/archive/20250121/202501211825.Vefi6H2q-lkp@intel.com/config)
compiler: clang version 19.1.3 (https://github.com/llvm/llvm-project ab51eccf88f5321e7c60591c5546b254b6afab99)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250121/202501211825.Vefi6H2q-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202501211825.Vefi6H2q-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from arch/x86/kernel/asm-offsets.c:14:
   In file included from include/linux/suspend.h:5:
   In file included from include/linux/swap.h:9:
   In file included from include/linux/memcontrol.h:21:
   In file included from include/linux/mm.h:2224:
   include/linux/vmstat.h:504:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
   504 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
   |                            ~~~~~~~~~~~~~~~~~~~~~ ^
   505 |                            item];
   |                            ~~~~
   include/linux/vmstat.h:511:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
   511 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
   |                            ~~~~~~~~~~~~~~~~~~~~~ ^
   512 |                            NR_VM_NUMA_EVENT_ITEMS +
   |                            ~~~~~~~~~~~~~~~~~~~~~~
   include/linux/vmstat.h:524:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
   524 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
   |                            ~~~~~~~~~~~~~~~~~~~~~ ^
   525 |                            NR_VM_NUMA_EVENT_ITEMS +
   |                            ~~~~~~~~~~~~~~~~~~~~~~
   3 warnings generated.
   ***
   *** Rust bindings generator 'bindgen' < 0.69.5 together with libclang >= 19.1
   *** may not work due to a bug (https://github.com/rust-lang/rust-bindgen/pull/2824),
   *** unless patched (like Debian's).
   ***   Your bindgen version:  0.65.1
   ***   Your libclang version: 19.1.3
   ***
   ***
   *** Please see Documentation/rust/quick-start.rst for details
   *** on how to set up the Rust support.
   ***
   In file included from rust/helpers/helpers.c:10:
   In file included from rust/helpers/blk.c:3:
   In file included from include/linux/blk-mq.h:5:
   In file included from include/linux/blkdev.h:9:
   In file included from include/linux/blk_types.h:10:
   In file included from include/linux/bvec.h:10:
   In file included from include/linux/highmem.h:8:
   In file included from include/linux/cacheflush.h:5:
   In file included from arch/x86/include/asm/cacheflush.h:5:
   In file included from include/linux/mm.h:2224:
   include/linux/vmstat.h:504:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
   504 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
   |                            ~~~~~~~~~~~~~~~~~~~~~ ^
   505 |                            item];
   |                            ~~~~
   include/linux/vmstat.h:511:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
   511 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
   |                            ~~~~~~~~~~~~~~~~~~~~~ ^
   512 |                            NR_VM_NUMA_EVENT_ITEMS +
   |                            ~~~~~~~~~~~~~~~~~~~~~~
   include/linux/vmstat.h:524:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
   524 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
   |                            ~~~~~~~~~~~~~~~~~~~~~ ^
   525 |                            NR_VM_NUMA_EVENT_ITEMS +
   |                            ~~~~~~~~~~~~~~~~~~~~~~
   3 warnings generated.
   clang diag: include/linux/vmstat.h:504:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
   clang diag: include/linux/vmstat.h:511:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
   clang diag: include/linux/vmstat.h:524:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
   clang diag: include/linux/vmstat.h:504:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
   clang diag: include/linux/vmstat.h:511:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
   clang diag: include/linux/vmstat.h:524:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
   clang diag: include/linux/vmstat.h:504:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
   clang diag: include/linux/vmstat.h:511:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
   clang diag: include/linux/vmstat.h:524:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
>> error[E0658]: associated type bounds are unstable
   --> rust/kernel/miscdevice.rs:64:27
   |
   64 | unsafe impl<T: MiscDevice<RegistrationData: Send>> Send for MiscDeviceRegistration<T> {}
   |                           ^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: see issue #52662 <https://github.com/rust-lang/rust/issues/52662> for more information
   = help: add `#![feature(associated_type_bounds)]` to the crate attributes to enable
   = note: this compiler was built on 2024-04-29; consider upgrading it if it is out of date

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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>
> }
> 
> In the `fops->open` callback of the miscdevice, you are given a
> reference to the registration, which allows you to access its fields.
> For example, as of commit 284ae0be4dca ("rust: miscdevice: Provide
> accessor to pull out miscdevice::this_device") you can access the
> internal `struct device`. However, there is still no way to access the
> `data` field in the above example, because you only have a reference to
> the registration.
> 
> Using container_of is also not possible to do safely. For example, if
> the destructor of `MyDriverData` runs, then the destructor of `data`
> would run before the miscdevice is deregistered, so using container_of
> to access `data` from `fops->open` could result in a UAF. A similar
> problem can happen on initialization if `misc` is not the last field to
> be initialized.
> 
> To provide a safe way to access user-defined data stored next to the
> `struct miscdevice`, make `MiscDeviceRegistration` into a container that
> can store a user-provided piece of data. This way, `fops->open` can
> access that data via the registration, since the data is stored inside
> the registration.
> 
> The container enforces that the additional user data is initialized
> before the miscdevice is registered, and that the miscdevice is
> deregistered before the user data is destroyed. This ensures that access
> to the userdata is safe.
> 
> For the same reasons as in commit 88441d5c6d17 ("rust: miscdevice:
> access the `struct miscdevice` from fops->open()"), you cannot access
> the user data in any other fops callback than open. This is because a
> miscdevice can be deregistered while there are still open files.
> 
> A situation where this user data might be required is when a platform
> driver acquires a resource in `probe` and wants to use this resource in
> the `fops` implementation of a `MiscDevice`.
> 
> Suggested-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: Christian Schrefl <chrisi.schrefl@gmail.com>
> ---
>  rust/kernel/miscdevice.rs        | 37 ++++++++++++++++++++++++++++++-------
>  samples/rust/rust_misc_device.rs |  4 +++-
>  2 files changed, 33 insertions(+), 8 deletions(-)
> 
> diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs
> index dfb363630c70b7187cae91f692d38bcf42d56a0a..3a756de27644e8a14e5bbd6b8abd9604e05faed4 100644
> --- a/rust/kernel/miscdevice.rs
> +++ b/rust/kernel/miscdevice.rs
> @@ -16,7 +16,7 @@
>      prelude::*,
>      seq_file::SeqFile,
>      str::CStr,
> -    types::{ForeignOwnable, Opaque},
> +    types::{Aliased, ForeignOwnable, Opaque},
>  };
>  use core::{
>      ffi::{c_int, c_long, c_uint, c_ulong},
> @@ -49,24 +49,30 @@ pub const fn into_raw<T: MiscDevice>(self) -> bindings::miscdevice {
>  /// # Invariants
>  ///
>  /// `inner` is a registered misc device.
> -#[repr(transparent)]
> +#[repr(C)]
>  #[pin_data(PinnedDrop)]
> -pub struct MiscDeviceRegistration<T> {
> +pub struct MiscDeviceRegistration<T: MiscDevice> {
>      #[pin]
>      inner: Opaque<bindings::miscdevice>,
> +    #[pin]
> +    data: Aliased<T::RegistrationData>,
>      _t: PhantomData<T>,
>  }
>  
>  // SAFETY: It is allowed to call `misc_deregister` on a different thread from where you called
>  // `misc_register`.
> -unsafe impl<T> Send for MiscDeviceRegistration<T> {}
> +unsafe impl<T: MiscDevice<RegistrationData: Send>> Send for MiscDeviceRegistration<T> {}
>  // SAFETY: All `&self` methods on this type are written to ensure that it is safe to call them in
>  // parallel.
> -unsafe impl<T> Sync for MiscDeviceRegistration<T> {}
> +// MiscDevice::RegistrationData is always Sync.
> +unsafe impl<T: MiscDevice> Sync for MiscDeviceRegistration<T> {}
>  
>  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,
>          })
>      }
> @@ -97,10 +104,18 @@ pub fn device(&self) -> &Device {
>          // before the underlying `struct miscdevice` is destroyed.
>          unsafe { Device::as_ref((*self.as_raw()).this_device) }
>      }
> +
> +    /// Access the additional data stored in this registration.
> +    pub fn data(&self) -> &T::RegistrationData {
> +        // SAFETY:
> +        // No mutable reference to the value contained by self.data can ever be created.
> +        // The value contained by self.data is valid for the entire lifetime of self.
> +        unsafe { &*self.data.get() }
> +    }
>  }
>  
>  #[pinned_drop]
> -impl<T> PinnedDrop for MiscDeviceRegistration<T> {
> +impl<T: MiscDevice> PinnedDrop for MiscDeviceRegistration<T> {
>      fn drop(self: Pin<&mut Self>) {
>          // SAFETY: We know that the device is registered by the type invariants.
>          unsafe { bindings::misc_deregister(self.inner.get()) };
> @@ -113,6 +128,11 @@ pub trait MiscDevice: Sized {
>      /// What kind of pointer should `Self` be wrapped in.
>      type Ptr: ForeignOwnable + Send + Sync;
>  
> +    /// The additional data carried by the `MiscDeviceRegistration` for this `MiscDevice`.
> +    /// If no additional data is required than () should be used.
> +    /// This data can be accessed in `open()` using `MiscDeviceRegistration::data()`.

Changed to intra-doc links.

> +    type RegistrationData: Sync;
> +
>      /// Called when the misc device is opened.
>      ///
>      /// The returned pointer will be stored as the private data for the file.
> @@ -218,6 +238,9 @@ impl<T: MiscDevice> VtableHelper<T> {
>      // SAFETY: This is a miscdevice, so `misc_open()` set the private data to a pointer to the
>      // associated `struct miscdevice` before calling into this method. Furthermore, `misc_open()`
>      // ensures that the miscdevice can't be unregistered and freed during this call to `fops_open`.
> +    // Since this the `MiscDeviceRegistration` struct uses `#[repr(C)]` and the miscdevice is the
> +    // first entry it is guaranteed that the address of the miscdevice is the same as the address
> +    // of the entire `MiscDeviceRegistration` struct.
>      let misc = unsafe { &*misc_ptr.cast::<MiscDeviceRegistration<T>>() };
>  
>      // SAFETY:
> diff --git a/samples/rust/rust_misc_device.rs b/samples/rust/rust_misc_device.rs
> index 40ad7266c2252e5c0b4e91e501ef9ada2eda3b16..779fcfd64119bdd5b4f8be740f7e8336c652b4d3 100644
> --- a/samples/rust/rust_misc_device.rs
> +++ b/samples/rust/rust_misc_device.rs
> @@ -136,7 +136,7 @@ fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
>          };
>  
>          try_pin_init!(Self {
> -            _miscdev <- MiscDeviceRegistration::register(options),
> +            _miscdev <- MiscDeviceRegistration::register(options, ()),
>          })
>      }
>  }
> @@ -156,6 +156,8 @@ struct RustMiscDevice {
>  impl MiscDevice for RustMiscDevice {
>      type Ptr = Pin<KBox<Self>>;
>  
> +    type RegistrationData = ();
> +
>      fn open(_file: &File, misc: &MiscDeviceRegistration<Self>) -> Result<Pin<KBox<Self>>> {
>          let dev = ARef::from(misc.device());
>  
>