Message-ID: <ef3b773f-f797-46be-b6f0-eba2e0e34005@gmail.com>
Date: Mon, 20 Jan 2025 01:27:58 +0100
From: Christian Schrefl <chrisi.schrefl@...il.com>
To: 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>, Alice Ryhl <aliceryhl@...gle.com>,
 Trevor Gross <tmgross@...ch.edu>, Arnd Bergmann <arnd@...db.de>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Lee Jones <lee@...nel.org>
Cc: rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] rust: miscdevice: Add additional data to
 MiscDeviceRegistration

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@...gle.com>
> Signed-off-by: Christian Schrefl <chrisi.schrefl@...il.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());
>  
> 

Powered by blists - more mailing lists