linux-kernel - Re: [PATCH v3 2/4] rust: debugfs: Bind file creation for long-lived Display

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: <CAGSQo02bU9HLG=KoYAukUWDX=Ky+kx_wCJszpS-x4gjWXWrYjQ@mail.gmail.com>
Date: Fri, 2 May 2025 11:07:47 -0700
From: Matthew Maurer <mmaurer@...gle.com>
To: Danilo Krummrich <dakr@...nel.org>
Cc: Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, 
	Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>, 
	Björn Roy Baron <bjorn3_gh@...tonmail.com>, 
	Benno Lossin <benno.lossin@...ton.me>, Andreas Hindborg <a.hindborg@...nel.org>, 
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>, 
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>, "Rafael J. Wysocki" <rafael@...nel.org>, 
	Sami Tolvanen <samitolvanen@...gle.com>, Timur Tabi <ttabi@...dia.com>, linux-kernel@...r.kernel.org, 
	rust-for-linux@...r.kernel.org
Subject: Re: [PATCH v3 2/4] rust: debugfs: Bind file creation for long-lived Display

On Thu, May 1, 2025 at 11:52 PM Danilo Krummrich <dakr@...nel.org> wrote:
>
> On Thu, May 01, 2025 at 10:47:42PM +0000, Matthew Maurer wrote:
> > +/// Handle to a DebugFS file.
> > +#[repr(transparent)]
> > +pub struct File(ManuallyDrop<Dir>);
>
> Same as with SubDir, please keep your original approach with keep().
>
> Exposing this as a separate type is much better, but I still find it a bit weird
> that it uses Dir internally, which still provides methods that are not
> applicable.
>
> I think it would be good to have the following types instead:
>
>         // Generic wrapper around the dentry pointer.
>         struct Entry;
>
>         // Based on Entry; provides Dir specific methods.
>         struct Dir;
>
>         // Based on Dir; implements Keep.
>         struct SubDir;
>
>         // Based on Entry; implements Keep.
>         struct File;
>
>         // Common trait that implements keep().
>         trait Keep;
>
> > +impl File {
> > +    /// Remove the file from DebugFS.
> > +    ///
> > +    /// # Examples
> > +    /// ```
> > +    /// # use kernel::c_str;
> > +    /// # use kernel::debugfs::Dir;
> > +    /// let dir = Dir::new(c_str!("foo"));
> > +    /// {
> > +    ///     let file = dir.display_file(c_str!("bar"), &0);
> > +    ///     // "foo/bar" is created.
> > +    /// }
> > +    /// // "foo/bar" still exists.
> > +    /// {
> > +    ///     let file = dir.display_file(c_str!("baz"), &0);
> > +    ///     // "foo/baz" is created.
> > +    ///     file.remove();
> > +    ///     // "foo/baz" is gone.
> > +    /// }
> > +    pub fn remove(self) {
> > +        drop(ManuallyDrop::into_inner(self.0))
> > +    }
> > +}
>
> Same as with my comment on Dir::subdir(), it really gets confusing if we invert
> the normal drop() logic. Removing the file when it is dropped and keeping it
> when calling keep() is much more intuitive..
>
> > +
> > +#[cfg(CONFIG_DEBUG_FS)]
> > +mod helpers {
> > +    use crate::seq_file::SeqFile;
> > +    use crate::seq_print;
> > +    use core::fmt::Display;
> > +
> > +    /// Implements `open` for `file_operations` via `single_open` to fill out a `seq_file`.
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// * `inode`'s private pointer must point to a value of type `T` which will outlive the `inode`
> > +    ///   and will not be mutated during this call.
> > +    /// * `file` must point to a live, not-yet-initialized file object.
> > +    pub(crate) unsafe extern "C" fn display_open<T: Display>(
> > +        inode: *mut bindings::inode,
> > +        file: *mut bindings::file,
> > +    ) -> i32 {
> > +        // SAFETY:
> > +        // * `file` is acceptable by caller precondition.
> > +        // * `print_act` will be called on a `seq_file` with private data set to the third argument,
> > +        //   so we meet its safety requirements.
> > +        // * The `data` pointer passed in the third argument is a valid `T` pointer that outlives
> > +        //   this call by caller preconditions.
> > +        unsafe { bindings::single_open(file, Some(display_act::<T>), (*inode).i_private) }
>
> Please split up unsafe operations.
>
> > +    }
> > +
> > +    /// Prints private data stashed in a seq_file to that seq file.
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// `seq` must point to a live `seq_file` whose private data is a live pointer to a `T` which is
> > +    /// not being mutated.
> > +    pub(crate) unsafe extern "C" fn display_act<T: Display>(
> > +        seq: *mut bindings::seq_file,
> > +        _: *mut core::ffi::c_void,
> > +    ) -> i32 {
> > +        // SAFETY: By caller precondition, this pointer is live, points to a value of type `T`, and
> > +        // is not being mutated.
> > +        let data = unsafe { &*((*seq).private as *mut T) };
>
> This creates an intermediate reference to private, which is UB. Please use
> addr_of! instead.

I'm making this change, but so that I can be correct in the future,
can you explain why taking an intermediate reference to private is UB?
My understanding is that my provided vtable are supposed to be the
only methods looking at this field, and they don't mutate it.
Additionally, the `private_data` field on file is accessed this way in
`miscdevice` at the moment - what makes it safe there, and UB here?