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: <CAH5fLgigt1SL0qyRwvFe77YqpzEXzKOOrCpNfpb1qLT1gW7S+g@mail.gmail.com>
Date: Mon, 9 Dec 2024 11:50:57 +0100
From: Alice Ryhl <aliceryhl@...gle.com>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
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 9, 2024 at 9:48 AM Greg Kroah-Hartman
<gregkh@...uxfoundation.org> wrote:
>
> 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.)

How is that enforced? What happens if I call misc_deregister while
there are open fds?

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

Not sure what the rules are, but I don't think Rust comments are being
wrapped at 80.

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

Ultimately, when interfacing with C code using void pointers, Rust is
going to need a pointer cast somewhere to assert what the type is.
With the current design, that place is the fops_* functions. We need
to get the pointer casts right there, but anywhere else the types are
enforced.

> >      // 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?

Well, it could probably be worded better at least. The point is that
nobody else is going to touch this field and we can do what we want
with it.

Alice