[PATCH v3 0/2] strncpy_from_user for Rust

[PATCH v3 0/2] strncpy_from_user for Rust

[Alice Ryhl]

There is currently no easy way to read NUL-terminated strings from
userspace. Trying to use the ordinary read function on an array of the
maximum length doesn't work because it could fail with EFAULT when the C
string is shorter than the maximum length. In this case,
strncpy_from_user is better because it doesn't return EFAULT even if it
encounters a page fault on bytes that are after the NUL-terminator but
before the maximum length.

Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
Changes in v3:
- Remove pub from raw_strncpy_from_user.
- Mention that some data may have been copied on EFAULT.
- Add more comments to strcpy_into_buf about tricky cases.
- Rewrite documentation of strcpy_into_buf.
- Pick up Reviewed-by tags.
- Link to v2: https://lore.kernel.org/r/20250429-strncpy-from-user-v2-0-7e6facac0bf0@google.com

Changes in v2:
- Rename the raw wrapper around strncpy_from_user to raw_strncpy_from_user.
- Add a more convenient helper on top that adds the missing
  NUL-terminator when necessary.
- Link to v1: https://lore.kernel.org/r/20250424-strncpy-from-user-v1-1-f983fe21685a@google.com

---
Alice Ryhl (2):
      uaccess: rust: add strncpy_from_user
      uaccess: rust: add UserSliceReader::strcpy_into_buf

 rust/kernel/uaccess.rs | 86 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 85 insertions(+), 1 deletion(-)
---
base-commit: 9c32cda43eb78f78c73aee4aa344b777714e259b
change-id: 20250424-strncpy-from-user-1f2d06b0cdde

Best regards,
-- 
Alice Ryhl <aliceryhl@google.com>

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

[Alice Ryhl]

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> {
+    // CAST: Slice lengths are guaranteed to be `<= isize::MAX`.
+    let len = buf.len() as isize;
+
+    // SAFETY: `buf` is valid for writing `buf.len()` bytes.
+    let res = unsafe {
+        bindings::strncpy_from_user(buf.as_mut_ptr().cast::<c_char>(), ptr as *const c_char, len)
+    };
+
+    if res < 0 {
+        return Err(Error::from_errno(res as i32));
+    }
+
+    #[cfg(CONFIG_RUST_OVERFLOW_CHECKS)]
+    assert!(res <= len);
+
+    Ok(res as usize)
+}

-- 
2.49.0.967.g6a0df3ecc3-goog

[PATCH v3 2/2] uaccess: rust: add UserSliceReader::strcpy_into_buf

[Alice Ryhl]

This patch adds a more convenient method for reading C strings from
userspace. Logic is added to NUL-terminate the buffer when necessary so
that a &CStr can be returned.

Note that we treat attempts to read past `self.length` as a fault, so
this returns EFAULT if that limit is exceeded before `buf.len()` is
reached.

Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 rust/kernel/uaccess.rs | 53 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 52 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs
index a7b123915e9aa2329f376d67cad93e2fc17ae017..978205289d297a4001a51fa40ac29039bff73672 100644
--- a/rust/kernel/uaccess.rs
+++ b/rust/kernel/uaccess.rs
@@ -293,6 +293,58 @@ pub fn read_all<A: Allocator>(mut self, buf: &mut Vec<u8, A>, flags: Flags) -> R
         unsafe { buf.set_len(buf.len() + len) };
         Ok(())
     }
+
+    /// Read a NUL-terminated string from userspace and return it.
+    ///
+    /// The string is read into `buf` and a NUL-terminator is added if the end of `buf` is reached.
+    /// Since there must be space to add a NUL-terminator, the buffer must not be empty. The
+    /// returned `&CStr` points into `buf`.
+    ///
+    /// Fails with [`EFAULT`] if the read happens on a bad address (some data may have been
+    /// copied).
+    #[doc(alias = "strncpy_from_user")]
+    pub fn strcpy_into_buf<'buf>(self, buf: &'buf mut [u8]) -> Result<&'buf CStr> {
+        if buf.is_empty() {
+            return Err(EINVAL);
+        }
+
+        // SAFETY: The types are compatible and `strncpy_from_user` doesn't write uninitialized
+        // bytes to `buf`.
+        let mut dst = unsafe { &mut *(buf as *mut [u8] as *mut [MaybeUninit<u8>]) };
+
+        // We never read more than `self.length` bytes.
+        if dst.len() > self.length {
+            dst = &mut dst[..self.length];
+        }
+
+        let mut len = raw_strncpy_from_user(self.ptr, dst)?;
+        if len < dst.len() {
+            // Add one to include the NUL-terminator.
+            len += 1;
+        } else if len < buf.len() {
+            // This implies that len == dst.len() < buf.len().
+            //
+            // This means that we could not fill the entire buffer, but we had to stop reading
+            // because we hit the `self.length` limit of this `UserSliceReader`. Since we did not
+            // fill the buffer, we treat this case as if we tried to read past the `self.length`
+            // limit and received a page fault, which is consistent with other `UserSliceReader`
+            // methods that also return page faults when you exceed `self.length`.
+            return Err(EFAULT);
+        } else {
+            // This implies that len == buf.len().
+            //
+            // This means that we filled the buffer exactly. In this case, we add a NUL-terminator
+            // and return it. Unlike the `len < dst.len()` branch, don't modify `len` because it
+            // already represents the length including the NUL-terminator.
+            //
+            // SAFETY: Due to the check at the beginning, the buffer is not empty.
+            unsafe { *buf.last_mut().unwrap_unchecked() = 0 };
+        }
+
+        // SAFETY: `raw_strncpy_from_user` guarantees that this range of bytes represents a
+        // NUL-terminated string with the only NUL byte being at the end.
+        Ok(unsafe { CStr::from_bytes_with_nul_unchecked(&buf[..len]) })
+    }
 }
 
 /// A writer for [`UserSlice`].
@@ -383,7 +435,6 @@ pub fn write<T: AsBytes>(&mut self, value: &T) -> Result {
 /// 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> {
     // CAST: Slice lengths are guaranteed to be `<= isize::MAX`.
     let len = buf.len() as isize;

-- 
2.49.0.967.g6a0df3ecc3-goog

Re: [PATCH v3 0/2] strncpy_from_user for Rust

[Alice Ryhl]

On Mon, May 5, 2025 at 2:17 PM Alice Ryhl <aliceryhl@google.com> wrote:
>
> There is currently no easy way to read NUL-terminated strings from
> userspace. Trying to use the ordinary read function on an array of the
> maximum length doesn't work because it could fail with EFAULT when the C
> string is shorter than the maximum length. In this case,
> strncpy_from_user is better because it doesn't return EFAULT even if it
> encounters a page fault on bytes that are after the NUL-terminator but
> before the maximum length.
>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
> Changes in v3:
> - Remove pub from raw_strncpy_from_user.
> - Mention that some data may have been copied on EFAULT.
> - Add more comments to strcpy_into_buf about tricky cases.
> - Rewrite documentation of strcpy_into_buf.
> - Pick up Reviewed-by tags.
> - Link to v2: https://lore.kernel.org/r/20250429-strncpy-from-user-v2-0-7e6facac0bf0@google.com

Oh I forgot to mention that I added a documentation alias so that
searching for strncpy_from_user using the search box on
https://rust.docs.kernel.org/ will show this function in the search
results.

Alice

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

[Greg Kroah-Hartman]

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 :(

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...)

thanks,

greg k-h

Re: [PATCH v3 2/2] uaccess: rust: add UserSliceReader::strcpy_into_buf

[Danilo Krummrich]

On Mon, May 05, 2025 at 12:17:32PM +0000, Alice Ryhl wrote:
> +    /// Read a NUL-terminated string from userspace and return it.
> +    ///
> +    /// The string is read into `buf` and a NUL-terminator is added if the end of `buf` is reached.
> +    /// Since there must be space to add a NUL-terminator, the buffer must not be empty. The
> +    /// returned `&CStr` points into `buf`.
> +    ///
> +    /// Fails with [`EFAULT`] if the read happens on a bad address (some data may have been
> +    /// copied).
> +    #[doc(alias = "strncpy_from_user")]
> +    pub fn strcpy_into_buf<'buf>(self, buf: &'buf mut [u8]) -> Result<&'buf CStr> {
> +        if buf.is_empty() {
> +            return Err(EINVAL);
> +        }
> +
> +        // SAFETY: The types are compatible and `strncpy_from_user` doesn't write uninitialized
> +        // bytes to `buf`.
> +        let mut dst = unsafe { &mut *(buf as *mut [u8] as *mut [MaybeUninit<u8>]) };
> +
> +        // We never read more than `self.length` bytes.
> +        if dst.len() > self.length {
> +            dst = &mut dst[..self.length];
> +        }
> +
> +        let mut len = raw_strncpy_from_user(self.ptr, dst)?;
> +        if len < dst.len() {
> +            // Add one to include the NUL-terminator.
> +            len += 1;
> +        } else if len < buf.len() {
> +            // This implies that len == dst.len() < buf.len().
> +            //
> +            // This means that we could not fill the entire buffer, but we had to stop reading
> +            // because we hit the `self.length` limit of this `UserSliceReader`. Since we did not
> +            // fill the buffer, we treat this case as if we tried to read past the `self.length`
> +            // limit and received a page fault, which is consistent with other `UserSliceReader`
> +            // methods that also return page faults when you exceed `self.length`.
> +            return Err(EFAULT);
> +        } else {
> +            // This implies that len == buf.len().
> +            //
> +            // This means that we filled the buffer exactly. In this case, we add a NUL-terminator
> +            // and return it. Unlike the `len < dst.len()` branch, don't modify `len` because it
> +            // already represents the length including the NUL-terminator.
> +            //
> +            // SAFETY: Due to the check at the beginning, the buffer is not empty.
> +            unsafe { *buf.last_mut().unwrap_unchecked() = 0 };
> +        }
> +
> +        // SAFETY: `raw_strncpy_from_user` guarantees that this range of bytes represents a
> +        // NUL-terminated string with the only NUL byte being at the end.

This isn't true if we hit the else case above, no?

With that fixed,

	Reviewed-by: Danilo Krummrich <dakr@kernel.org>

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

[Boqun Feng]

On Mon, May 05, 2025 at 04:30:05PM +0200, Greg Kroah-Hartman wrote:
[...]
> > +
> > +/// 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> {
> 
[...]
> 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

I think the type `UserPtr` is supposed to track this information, i.e.
Rust can only get a `UserPtr` from an input from userspace, and of
course Rust code cannot treat `UserPtr` as a normal pointer. For
example, the following would fall because of this:

    pub fn foo(ptr: *mut u8, ...) {
        raw_strncpy_from_user(ptr, ...);
    }

Regards,
Boqun

> 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...)
> 
> thanks,
> 
> greg k-h

Re: [PATCH v3 2/2] uaccess: rust: add UserSliceReader::strcpy_into_buf

[Alice Ryhl]

On Mon, May 05, 2025 at 06:22:31PM +0200, Danilo Krummrich wrote:
> On Mon, May 05, 2025 at 12:17:32PM +0000, Alice Ryhl wrote:
> > +    /// Read a NUL-terminated string from userspace and return it.
> > +    ///
> > +    /// The string is read into `buf` and a NUL-terminator is added if the end of `buf` is reached.
> > +    /// Since there must be space to add a NUL-terminator, the buffer must not be empty. The
> > +    /// returned `&CStr` points into `buf`.
> > +    ///
> > +    /// Fails with [`EFAULT`] if the read happens on a bad address (some data may have been
> > +    /// copied).
> > +    #[doc(alias = "strncpy_from_user")]
> > +    pub fn strcpy_into_buf<'buf>(self, buf: &'buf mut [u8]) -> Result<&'buf CStr> {
> > +        if buf.is_empty() {
> > +            return Err(EINVAL);
> > +        }
> > +
> > +        // SAFETY: The types are compatible and `strncpy_from_user` doesn't write uninitialized
> > +        // bytes to `buf`.
> > +        let mut dst = unsafe { &mut *(buf as *mut [u8] as *mut [MaybeUninit<u8>]) };
> > +
> > +        // We never read more than `self.length` bytes.
> > +        if dst.len() > self.length {
> > +            dst = &mut dst[..self.length];
> > +        }
> > +
> > +        let mut len = raw_strncpy_from_user(self.ptr, dst)?;
> > +        if len < dst.len() {
> > +            // Add one to include the NUL-terminator.
> > +            len += 1;
> > +        } else if len < buf.len() {
> > +            // This implies that len == dst.len() < buf.len().
> > +            //
> > +            // This means that we could not fill the entire buffer, but we had to stop reading
> > +            // because we hit the `self.length` limit of this `UserSliceReader`. Since we did not
> > +            // fill the buffer, we treat this case as if we tried to read past the `self.length`
> > +            // limit and received a page fault, which is consistent with other `UserSliceReader`
> > +            // methods that also return page faults when you exceed `self.length`.
> > +            return Err(EFAULT);
> > +        } else {
> > +            // This implies that len == buf.len().
> > +            //
> > +            // This means that we filled the buffer exactly. In this case, we add a NUL-terminator
> > +            // and return it. Unlike the `len < dst.len()` branch, don't modify `len` because it
> > +            // already represents the length including the NUL-terminator.
> > +            //
> > +            // SAFETY: Due to the check at the beginning, the buffer is not empty.
> > +            unsafe { *buf.last_mut().unwrap_unchecked() = 0 };
> > +        }
> > +
> > +        // SAFETY: `raw_strncpy_from_user` guarantees that this range of bytes represents a
> > +        // NUL-terminated string with the only NUL byte being at the end.
> 
> This isn't true if we hit the else case above, no?
> 
> With that fixed,
> 
> 	Reviewed-by: Danilo Krummrich <dakr@kernel.org>

I guess the wording is a bit off. I will update to this:

// SAFETY: There are two cases:
// * If we hit the `len < dst.len()` case, then `raw_strncpy_from_user`
//   guarantees that this slice contains exactly one NUL byte at the end
//   of the string.
// * Otherwise, `raw_strncpy_from_user` guarantees that the string
//   contained no NUL bytes, and we have since added a NUL byte at the
//   end.

Alice

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

[Alice Ryhl]

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.)

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.

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.

Alice

[1]: https://doc.rust-lang.org/rust-by-example/generics/new_types.html

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

[Greg Kroah-Hartman]

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

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

[Alice Ryhl]

On Tue, May 6, 2025 at 2:52 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.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@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.

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