Received-SPF: pass (google.com: domain of linux-kernel+bounces-59236-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240208-alice-mm-v2-3-d821250204a6@google.com> <20240208225748.12031-1-kernel@valentinobst.de>
In-Reply-To: <20240208225748.12031-1-kernel@valentinobst.de>
From: Alice Ryhl <aliceryhl@google.com>
Date: Fri, 9 Feb 2024 11:40:24 +0100
Message-ID: <CAH5fLgg3C79DHz0FJ1LOhvetv4THHNAnu6XPx0vycFCb_txkfg@mail.gmail.com>
Subject: Re: [PATCH v2 3/4] rust: uaccess: add typed accessors for userspace pointers
To: Valentin Obst <kernel@valentinobst.de>
Cc: a.hindborg@samsung.com, akpm@linux-foundation.org, alex.gaynor@gmail.com, 
	arnd@arndb.de, arve@android.com, benno.lossin@proton.me, 
	bjorn3_gh@protonmail.com, boqun.feng@gmail.com, brauner@kernel.org, 
	cmllamas@google.com, gary@garyguo.net, gregkh@linuxfoundation.org, 
	joel@joelfernandes.org, keescook@chromium.org, linux-kernel@vger.kernel.org, 
	linux-mm@kvack.org, maco@android.com, ojeda@kernel.org, 
	rust-for-linux@vger.kernel.org, surenb@google.com, tkjos@android.com, 
	viro@zeniv.linux.org.uk, wedsonaf@gmail.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Feb 8, 2024 at 11:57=E2=80=AFPM Valentin Obst <kernel@valentinobst.=
de> wrote:
>
> > +/// If a struct implements this trait, then it is okay to copy it byte=
-for-byte
> > +/// to userspace. This means that it should not have any padding, as p=
adding
> > +/// bytes are uninitialized. Reading uninitialized memory is not just =
undefined
> > +/// behavior, it may even lead to leaking sensitive information on the=
 stack to
> > +/// userspace.
>
> This feels a bit too restrictive to me. Isn't it okay to copy types with
> padding if it is ensured that the padding is always initialized?
>
> I recall that in C one occasionally does a `memset` for structs that are
> copied to user space. I imagine that one could have a Rust
> abstraction/macro that makes it easy to define custom types that can
> always guarantee that all padding bytes are initialized. Such types
> would then qualify for being copied to user space if all field do so as
> well.
>
> This could be a significant quality-of-life improvement for drivers
> as it can be tedious to define struct without padding.

I don't think we should go that route. For example:

let struct_1 =3D ..;
memset(&mut struct_1, 0);
let struct_2 =3D struct_1;

Even though struct_1 has its padding zeroed here, that is not the case
for struct_2. When Rust performs a typed copy/move, the padding is not
copied.

Anyway, there is a work-around. Define your struct with MaybeUninit:

// INVARIANT: All bytes always initialized.
struct MyWrapper(MaybeUninit<bindings::c_struct>);

impl Default for MyWrapper {
    fn default() -> Self {
        MyWrapper(MaybeUninit::zeroed())
    }
}

Unlike the bare struct, things wrapped in MaybeUninit always have
their padding preserved. Then, you can implement the trait for this
wrapper, since its padding is always initialized even if that is not
true for the wrapped struct.

Alice