Re: [PATCH v3 1/2] uaccess: rust: add strncpy_from_user

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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@kernel.org>
> > > Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
> > > Signed-off-by: Alice Ryhl <aliceryhl@google.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.

thanks,

greg k-h