linux-kernel - Re: [PATCH 2/7] rust: cred: add Rust abstraction for `struct cred`

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]

Date:   Fri, 1 Dec 2023 11:27:33 +0100
From:   Christian Brauner <brauner@...nel.org>
To:     Alice Ryhl <aliceryhl@...gle.com>
Cc:     benno.lossin@...ton.me, a.hindborg@...sung.com,
        alex.gaynor@...il.com, arve@...roid.com, bjorn3_gh@...tonmail.com,
        boqun.feng@...il.com, cmllamas@...gle.com,
        dan.j.williams@...el.com, dxu@...uu.xyz, gary@...yguo.net,
        gregkh@...uxfoundation.org, joel@...lfernandes.org,
        keescook@...omium.org, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org, maco@...roid.com, ojeda@...nel.org,
        peterz@...radead.org, rust-for-linux@...r.kernel.org,
        surenb@...gle.com, tglx@...utronix.de, tkjos@...roid.com,
        viro@...iv.linux.org.uk, wedsonaf@...il.com, willy@...radead.org
Subject: Re: [PATCH 2/7] rust: cred: add Rust abstraction for `struct cred`

On Fri, Dec 01, 2023 at 09:06:35AM +0000, Alice Ryhl wrote:
> Benno Lossin <benno.lossin@...ton.me> writes:
> > On 11/29/23 13:51, Alice Ryhl wrote:
> >> +    /// Returns the credentials of the task that originally opened the file.
> >> +    pub fn cred(&self) -> &Credential {
> >> +        // This `read_volatile` is intended to correspond to a READ_ONCE call.
> >> +        //
> >> +        // SAFETY: The file is valid because the shared reference guarantees a nonzero refcount.
> >> +        //
> >> +        // TODO: Replace with `read_once` when available on the Rust side.
> >> +        let ptr = unsafe { core::ptr::addr_of!((*self.0.get()).f_cred).read_volatile() };
> >> +
> >> +        // SAFETY: The signature of this function ensures that the caller will only access the
> >> +        // returned credential while the file is still valid, and the credential must stay valid
> >> +        // while the file is valid.
> > 
> > About the last part of this safety comment, is this a guarantee from the
> > C side? If yes, then I would phrase it that way:
> > 
> >     ... while the file is still valid, and the C side ensures that the
> >     credentials stay valid while the file is valid.
> 
> Yes, that's my intention with this code.
> 
> But I guess this is a good question for Christian Brauner to confirm:
> 
> If I read the credential from the `f_cred` field, is it guaranteed that
> the pointer remains valid for at least as long as the file?
> 
> Or should I do some dance along the lines of "lock file, increment
> refcount on credential, unlock file"?

The lifetime of the f_cred reference is at least as long as the lifetime
of the file:

// file not yet visible anywhere
some_file = alloc_file*()
-> init_file()
   {
           file->f_cred = get_cred(cred /* usually current_cred() */)
   }


// install into fd_table -> irreversible, thing visible, possibly shared
fd_install(1234, some_file)

// last fput
fput()
// atomic_dec_and_test() dance:
-> file_free() // either "delayed" through task work, workqueue, or
	       // sometimes freed right away if file hasn't been opened,
	       // i.e., if fd_install() wasn't called
   -> put_cred(file->f_cred)

In order to access anything you must hold a reference to the file or
files->file_lock. IOW, no poking around in f->f_cred or any field for
that matter just under rcu_read_lock() for example. Because files are
SLAB_TYPESAFE_BY_RCU. You might be poking in someone else's creds then.