linux-kernel - Re: [PATCH v2 2/2] rust: miscdevice: access the `struct miscdevice` from fops->open()

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: <2024120925-express-unmasked-76b4@gregkh>
Date: Mon, 9 Dec 2024 09:48:43 +0100
From: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To: Alice Ryhl <aliceryhl@...gle.com>
Cc: Arnd Bergmann <arnd@...db.de>, Alexander Viro <viro@...iv.linux.org.uk>,
	Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>,
	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>, Lee Jones <lee@...nel.org>,
	rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] rust: miscdevice: access the `struct miscdevice`
 from fops->open()

On Mon, Dec 09, 2024 at 07:27:47AM +0000, Alice Ryhl wrote:
> Providing access to the underlying `struct miscdevice` is useful for
> various reasons. For example, this allows you access the miscdevice's
> internal `struct device` for use with the `dev_*` printing macros.
> 
> Note that since the underlying `struct miscdevice` could get freed at
> any point after the fops->open() call, only the open call is given
> access to it. To print from other calls, they should take a refcount on
> the device to keep it alive.

The lifespan of the miscdevice is at least from open until close, so
it's safe for at least then (i.e. read/write/ioctl/etc.)

> Signed-off-by: Alice Ryhl <aliceryhl@...gle.com>
> ---
>  rust/kernel/miscdevice.rs | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs
> index 0cb79676c139..c5af1d5ec4be 100644
> --- a/rust/kernel/miscdevice.rs
> +++ b/rust/kernel/miscdevice.rs
> @@ -104,7 +104,7 @@ pub trait MiscDevice {
>      /// Called when the misc device is opened.
>      ///
>      /// The returned pointer will be stored as the private data for the file.
> -    fn open(_file: &File) -> Result<Self::Ptr>;
> +    fn open(_file: &File, _misc: &MiscDeviceRegistration<Self>) -> Result<Self::Ptr>;
>  
>      /// Called when the misc device is released.
>      fn release(device: Self::Ptr, _file: &File) {
> @@ -190,14 +190,27 @@ impl<T: MiscDevice> VtableHelper<T> {
>          return ret;
>      }
>  
> +    // SAFETY: The opwn call of a file can access the private data.

s/opwn/open/ :)

> +    let misc_ptr = unsafe { (*file).private_data };

Blank line here?

> +    // 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`.

Aren't we wrapping comment lines at 80 columns still?  I can't remember
anymore...

> +    let misc = unsafe { &*misc_ptr.cast::<MiscDeviceRegistration<T>>() };
> +
>      // SAFETY:
> -    // * The file is valid for the duration of this call.
> +    // * The file is valid for the duration of the `T::open` call.

It's valid for the lifespan between open/release.

>      // * There is no active fdget_pos region on the file on this thread.
> -    let ptr = match T::open(unsafe { File::from_raw_file(file) }) {
> +    let file = unsafe { File::from_raw_file(file) };
> +
> +    let ptr = match T::open(file, misc) {
>          Ok(ptr) => ptr,
>          Err(err) => return err.to_errno(),
>      };
>  
> +    // This overwrites the private data from above. It makes sense to not hold on to the misc
> +    // pointer since the `struct miscdevice` can get unregistered as soon as we return from this
> +    // call, so the misc pointer might be dangling on future file operations.
> +    //

Wait, what are we overwriting this here with?  Now private data points
to the misc device when before it was the file structure.  No other code
needed to be changed because of that?  Can't we enforce this pointer
type somewhere so that any casts in any read/write/ioctl also "knows" it
has the right type?  This feels "dangerous" to me.

>      // SAFETY: The open call of a file owns the private data.
>      unsafe { (*file).private_data = ptr.into_foreign().cast_mut() };

Is this SAFETY comment still correct?

thanks,

greg k-h