Message-ID: <CAH5fLgjVmyAy8GRBzmRG8+i3ci16M4zk7qQghn47krpPmpLQnw@mail.gmail.com>
Date: Tue, 6 May 2025 15:30:30 +0200
From: Alice Ryhl <aliceryhl@...gle.com>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: Miguel Ojeda <ojeda@...nel.org>, Andrew Morton <akpm@...ux-foundation.org>, 
	Alexander Viro <viro@...iv.linux.org.uk>, 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>, Danilo Krummrich <dakr@...nel.org>, rust-for-linux@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 1/2] uaccess: rust: add strncpy_from_user

On Tue, May 6, 2025 at 2:52 PM Greg Kroah-Hartman
<gregkh@...uxfoundation.org> wrote:
>
> On Tue, May 06, 2025 at 09:18:41AM +0000, Alice Ryhl wrote:
> > On Mon, May 05, 2025 at 04:30:05PM +0200, Greg Kroah-Hartman wrote:
> > > On Mon, May 05, 2025 at 12:17:31PM +0000, Alice Ryhl wrote:
> > > > This patch adds a direct wrapper around the C function of the same name.
> > > > It's not really intended for direct use by Rust code since
> > > > strncpy_from_user has a somewhat unfortunate API where it only
> > > > nul-terminates the buffer if there's space for the nul-terminator. This
> > > > means that a direct Rust wrapper around it could not return a &CStr
> > > > since the buffer may not be a cstring. However, we still add the method
> > > > to build more convenient APIs on top of it, which will happen in
> > > > subsequent patches.
> > > >
> > > > Reviewed-by: Danilo Krummrich <dakr@...nel.org>
> > > > Reviewed-by: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> > > > Reviewed-by: Boqun Feng <boqun.feng@...il.com>
> > > > Signed-off-by: Alice Ryhl <aliceryhl@...gle.com>
> > > > ---
> > > >  rust/kernel/uaccess.rs | 35 ++++++++++++++++++++++++++++++++++-
> > > >  1 file changed, 34 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs
> > > > index 80a9782b1c6e98ed6eae308ade8551afa7adc188..a7b123915e9aa2329f376d67cad93e2fc17ae017 100644
> > > > --- a/rust/kernel/uaccess.rs
> > > > +++ b/rust/kernel/uaccess.rs
> > > > @@ -8,7 +8,7 @@
> > > >      alloc::{Allocator, Flags},
> > > >      bindings,
> > > >      error::Result,
> > > > -    ffi::c_void,
> > > > +    ffi::{c_char, c_void},
> > > >      prelude::*,
> > > >      transmute::{AsBytes, FromBytes},
> > > >  };
> > > > @@ -369,3 +369,36 @@ pub fn write<T: AsBytes>(&mut self, value: &T) -> Result {
> > > >          Ok(())
> > > >      }
> > > >  }
> > > > +
> > > > +/// Reads a nul-terminated string into `buf` and returns the length.
> > > > +///
> > > > +/// This reads from userspace until a NUL byte is encountered, or until `buf.len()` bytes have been
> > > > +/// read. Fails with [`EFAULT`] if a read happens on a bad address (some data may have been
> > > > +/// copied). When the end of the buffer is encountered, no NUL byte is added, so the string is
> > > > +/// *not* guaranteed to be NUL-terminated when `Ok(buf.len())` is returned.
> > > > +///
> > > > +/// # Guarantees
> > > > +///
> > > > +/// When this function returns `Ok(len)`, it is guaranteed that the first `len` of `buf` bytes are
> > > > +/// initialized and non-zero. Furthermore, if `len < buf.len()`, then `buf[len]` is a NUL byte.
> > > > +/// Unsafe code may rely on these guarantees.
> > > > +#[inline]
> > > > +#[expect(dead_code)]
> > > > +fn raw_strncpy_from_user(ptr: UserPtr, buf: &mut [MaybeUninit<u8>]) -> Result<usize> {
> > >
> > > Nit, the parameters here are backwards from the C version of
> > > strncpy_from_user(), which is going to cause us no end of grief when
> > > reviewing code between the two languages :(
> >
> > I'll swap them.
> >
> > fn raw_strncpy_from_user(dst: &mut [MaybeUninit<u8>], src: UserPtr) -> Result<usize> {
> >
> > > Also, it's not your fault, but we don't have any type of __user tag for
> > > data coming from userspace yet to track this type of thing?  The
> > > compiler (well sparse) can catch this type of thing in C, any hints on
> > > what we could do in Rust for the same type of guarantee (i.e. don't
> > > touch user data before it's been copied, and then we need to treat it as
> > > "unverified" but that's a different patch series...)
> >
> > The UserPtr typedef is intended to do that, but since it's only a
> > typedef to usize, the compiler won't detect it if you mix up a user
> > pointer with a length. (It will detect mix-ups with pointers since we
> > use an integer type for UserPtr.)
>
> Sorry, I missed the "UserPtr" for some reason.  But having an integer
> type for UserPtr feels like it's going to cause problems in the
> long-run.
>
> > What we can do is replace the typedef with
> >
> > #[repr(transparent)]
> > struct UserPtr(pub usize);
> >
> > That way, it becomes it's own separate type (this is called the newtype
> > pattern [1]) so that it can't be mixed up with anything else.
>
> Why not use a real pointer like:
>         struct UserPtr(pub *const u8)
>
> > The #[repr(transparent)] annotation makes the compiler treat it like a
> > bare long for ABI-purposes. I'm not sure if any function ABIs actually
> > treat a long differently from a struct that just contains a long, but if
> > such ABIs exist, then the annotation ensures that the long ABI is used
> > rather than the struct-containing-long ABI.
>
> In the kernel, "unsigned long" is guaranteed to hold a pointer.  Which
> is why many of the old allocation functions return that type.

Let's continue this discussion on the patch I just sent to make it a
real struct:
https://lore.kernel.org/r/20250506-userptr-newtype-v1-1-a0f6f8ce9fc5@google.com

I don't fully recall the reason, but it was changed from a raw pointer
to usize in version 6 of the patch set that added it.

Alice

Powered by blists - more mailing lists