[PATCH v4 0/2] strncpy_from_user for Rust

[PATCH v4 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 v4:
- Swap order of arguments to raw_strncpy_from_user, and rename buf to
  dst.
- Update safety comment on CStr::from_bytes_with_nul_unchecked.
- Add `` in "This implies that len == dst.len() < buf.len()."
- Pick up Reviewed-by tags.
- Link to v3: https://lore.kernel.org/r/20250505-strncpy-from-user-v3-0-85c677fd4f91@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.
- Add documentation alias.
- 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 | 89 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 88 insertions(+), 1 deletion(-)
---
base-commit: 9c32cda43eb78f78c73aee4aa344b777714e259b
change-id: 20250424-strncpy-from-user-1f2d06b0cdde

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

[PATCH v4 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..9b1e4016fca2c25a44a8417c7e35e0fcf08aa959 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 `dst` and returns the length.
+///
+/// This reads from userspace until a NUL byte is encountered, or until `dst.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(dst.len())` is returned.
+///
+/// # Guarantees
+///
+/// When this function returns `Ok(len)`, it is guaranteed that the first `len` bytes of `dst` are
+/// initialized and non-zero. Furthermore, if `len < dst.len()`, then `dst[len]` is a NUL byte.
+/// Unsafe code may rely on these guarantees.
+#[inline]
+#[expect(dead_code)]
+fn raw_strncpy_from_user(dst: &mut [MaybeUninit<u8>], src: UserPtr) -> Result<usize> {
+    // CAST: Slice lengths are guaranteed to be `<= isize::MAX`.
+    let len = dst.len() as isize;
+
+    // SAFETY: `dst` is valid for writing `dst.len()` bytes.
+    let res = unsafe {
+        bindings::strncpy_from_user(dst.as_mut_ptr().cast::<c_char>(), src 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.1151.ga128411c76-goog

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

[Benno Lossin]

On Tue May 27, 2025 at 2:34 PM CEST, 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>

Reviewed-by: Benno Lossin <lossin@kernel.org>

One question below.

> ---
>  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..9b1e4016fca2c25a44a8417c7e35e0fcf08aa959 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 `dst` and returns the length.
> +///
> +/// This reads from userspace until a NUL byte is encountered, or until `dst.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(dst.len())` is returned.
> +///
> +/// # Guarantees
> +///
> +/// When this function returns `Ok(len)`, it is guaranteed that the first `len` bytes of `dst` are
> +/// initialized and non-zero. Furthermore, if `len < dst.len()`, then `dst[len]` is a NUL byte.
> +/// Unsafe code may rely on these guarantees.
> +#[inline]
> +#[expect(dead_code)]
> +fn raw_strncpy_from_user(dst: &mut [MaybeUninit<u8>], src: UserPtr) -> Result<usize> {

We could also return `&[u8]` here instead of the size. Would that
improve the users of this API?

---
Cheers,
Benno

> +    // CAST: Slice lengths are guaranteed to be `<= isize::MAX`.
> +    let len = dst.len() as isize;
> +
> +    // SAFETY: `dst` is valid for writing `dst.len()` bytes.
> +    let res = unsafe {
> +        bindings::strncpy_from_user(dst.as_mut_ptr().cast::<c_char>(), src 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)
> +}

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

[Greg Kroah-Hartman]

On Fri, May 30, 2025 at 01:32:44PM +0200, Benno Lossin wrote:
> On Tue May 27, 2025 at 2:34 PM CEST, 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>
> 
> Reviewed-by: Benno Lossin <lossin@kernel.org>
> 
> One question below.
> 
> > ---
> >  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..9b1e4016fca2c25a44a8417c7e35e0fcf08aa959 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 `dst` and returns the length.
> > +///
> > +/// This reads from userspace until a NUL byte is encountered, or until `dst.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(dst.len())` is returned.
> > +///
> > +/// # Guarantees
> > +///
> > +/// When this function returns `Ok(len)`, it is guaranteed that the first `len` bytes of `dst` are
> > +/// initialized and non-zero. Furthermore, if `len < dst.len()`, then `dst[len]` is a NUL byte.
> > +/// Unsafe code may rely on these guarantees.
> > +#[inline]
> > +#[expect(dead_code)]
> > +fn raw_strncpy_from_user(dst: &mut [MaybeUninit<u8>], src: UserPtr) -> Result<usize> {
> 
> We could also return `&[u8]` here instead of the size. Would that
> improve the users of this API?

That would differ from the C function, strncpy_from_user() and force us
reviewers to try to remember what is supposed to be happening here.  So
I wouldn't recommend that if at all possible please.

thanks,

greg k-h

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

[Alice Ryhl]

On Fri, May 30, 2025 at 01:32:44PM +0200, Benno Lossin wrote:
> On Tue May 27, 2025 at 2:34 PM CEST, 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>
> 
> Reviewed-by: Benno Lossin <lossin@kernel.org>

Thanks!

> > +/// Reads a nul-terminated string into `dst` and returns the length.
> > +///
> > +/// This reads from userspace until a NUL byte is encountered, or until `dst.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(dst.len())` is returned.
> > +///
> > +/// # Guarantees
> > +///
> > +/// When this function returns `Ok(len)`, it is guaranteed that the first `len` bytes of `dst` are
> > +/// initialized and non-zero. Furthermore, if `len < dst.len()`, then `dst[len]` is a NUL byte.
> > +/// Unsafe code may rely on these guarantees.
> > +#[inline]
> > +#[expect(dead_code)]
> > +fn raw_strncpy_from_user(dst: &mut [MaybeUninit<u8>], src: UserPtr) -> Result<usize> {
> 
> We could also return `&[u8]` here instead of the size. Would that
> improve the users of this API?

Beyond what Greg says, convenience of use is not a goal *at all* of this
function. It's purpose is to faithfully wrap the C function and match
its semantics exactly. Ease of use is taken care of by patch 2 of this
series.

Alice

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

[Benno Lossin]

This patch's title should be adjusted, as it's adding
`raw_strncpy_from_user` and not `strncpy_from_user`.

On Tue May 27, 2025 at 2:34 PM CEST, Alice Ryhl wrote:
> +/// Reads a nul-terminated string into `dst` and returns the length.
> +///
> +/// This reads from userspace until a NUL byte is encountered, or until `dst.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(dst.len())` is returned.
> +///
> +/// # Guarantees
> +///
> +/// When this function returns `Ok(len)`, it is guaranteed that the first `len` bytes of `dst` are
> +/// initialized and non-zero. Furthermore, if `len < dst.len()`, then `dst[len]` is a NUL byte.
> +/// Unsafe code may rely on these guarantees.

I would remove the last sentence, it already is implied.

> +#[inline]
> +#[expect(dead_code)]
> +fn raw_strncpy_from_user(dst: &mut [MaybeUninit<u8>], src: UserPtr) -> Result<usize> {
> +    // CAST: Slice lengths are guaranteed to be `<= isize::MAX`.
> +    let len = dst.len() as isize;
> +
> +    // SAFETY: `dst` is valid for writing `dst.len()` bytes.
> +    let res = unsafe {
> +        bindings::strncpy_from_user(dst.as_mut_ptr().cast::<c_char>(), src 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)

We probably should add a `GUARANTEES` comment here, no?

---
Cheers,
Benno

> +}

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

[Alice Ryhl]

On Fri, May 30, 2025 at 8:13 PM Benno Lossin <lossin@kernel.org> wrote:
>
> This patch's title should be adjusted, as it's adding
> `raw_strncpy_from_user` and not `strncpy_from_user`.
>
> On Tue May 27, 2025 at 2:34 PM CEST, Alice Ryhl wrote:
> > +/// Reads a nul-terminated string into `dst` and returns the length.
> > +///
> > +/// This reads from userspace until a NUL byte is encountered, or until `dst.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(dst.len())` is returned.
> > +///
> > +/// # Guarantees
> > +///
> > +/// When this function returns `Ok(len)`, it is guaranteed that the first `len` bytes of `dst` are
> > +/// initialized and non-zero. Furthermore, if `len < dst.len()`, then `dst[len]` is a NUL byte.
> > +/// Unsafe code may rely on these guarantees.
>
> I would remove the last sentence, it already is implied.

I do not mind that.

> > +#[inline]
> > +#[expect(dead_code)]
> > +fn raw_strncpy_from_user(dst: &mut [MaybeUninit<u8>], src: UserPtr) -> Result<usize> {
> > +    // CAST: Slice lengths are guaranteed to be `<= isize::MAX`.
> > +    let len = dst.len() as isize;
> > +
> > +    // SAFETY: `dst` is valid for writing `dst.len()` bytes.
> > +    let res = unsafe {
> > +        bindings::strncpy_from_user(dst.as_mut_ptr().cast::<c_char>(), src 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)
>
> We probably should add a `GUARANTEES` comment here, no?

I can see the idea behind such comments, but I do not believe we've
used them so far.

Alice

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

[Benno Lossin]

On Sat May 31, 2025 at 3:27 PM CEST, Alice Ryhl wrote:
> On Fri, May 30, 2025 at 8:13 PM Benno Lossin <lossin@kernel.org> wrote:
>> On Tue May 27, 2025 at 2:34 PM CEST, Alice Ryhl wrote:
>> > +#[inline]
>> > +#[expect(dead_code)]
>> > +fn raw_strncpy_from_user(dst: &mut [MaybeUninit<u8>], src: UserPtr) -> Result<usize> {
>> > +    // CAST: Slice lengths are guaranteed to be `<= isize::MAX`.
>> > +    let len = dst.len() as isize;
>> > +
>> > +    // SAFETY: `dst` is valid for writing `dst.len()` bytes.
>> > +    let res = unsafe {
>> > +        bindings::strncpy_from_user(dst.as_mut_ptr().cast::<c_char>(), src 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)
>>
>> We probably should add a `GUARANTEES` comment here, no?
>
> I can see the idea behind such comments, but I do not believe we've
> used them so far.

Yes, but we also haven't really used `# Guarantees` all that often.

---
Cheers,
Benno

[PATCH v4 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.

Reviewed-by: Danilo Krummrich <dakr@kernel.org>
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 rust/kernel/uaccess.rs | 56 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 55 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs
index 9b1e4016fca2c25a44a8417c7e35e0fcf08aa959..e6534b52a1920254d61f8349426d4cdb38286089 100644
--- a/rust/kernel/uaccess.rs
+++ b/rust/kernel/uaccess.rs
@@ -293,6 +293,61 @@ 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(dst, self.ptr)?;
+        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: 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.
+        Ok(unsafe { CStr::from_bytes_with_nul_unchecked(&buf[..len]) })
+    }
 }
 
 /// A writer for [`UserSlice`].
@@ -383,7 +438,6 @@ pub fn write<T: AsBytes>(&mut self, value: &T) -> Result {
 /// initialized and non-zero. Furthermore, if `len < dst.len()`, then `dst[len]` is a NUL byte.
 /// Unsafe code may rely on these guarantees.
 #[inline]
-#[expect(dead_code)]
 fn raw_strncpy_from_user(dst: &mut [MaybeUninit<u8>], src: UserPtr) -> Result<usize> {
     // CAST: Slice lengths are guaranteed to be `<= isize::MAX`.
     let len = dst.len() as isize;

-- 
2.49.0.1151.ga128411c76-goog

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

[Benno Lossin]

On Tue May 27, 2025 at 2:34 PM CEST, Alice Ryhl wrote:
> 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.
>
> Reviewed-by: Danilo Krummrich <dakr@kernel.org>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
>  rust/kernel/uaccess.rs | 56 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 55 insertions(+), 1 deletion(-)
>
> diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs
> index 9b1e4016fca2c25a44a8417c7e35e0fcf08aa959..e6534b52a1920254d61f8349426d4cdb38286089 100644
> --- a/rust/kernel/uaccess.rs
> +++ b/rust/kernel/uaccess.rs
> @@ -293,6 +293,61 @@ 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(dst, self.ptr)?;
> +        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 };

In this case you're overwriting the last character read. Should we give
`raw_strncpy_from_user` access to one less byte and then write NUL into
that?

---
Cheers,
Benno

> +        }
> +
> +        // 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.
> +        Ok(unsafe { CStr::from_bytes_with_nul_unchecked(&buf[..len]) })
> +    }
>  }
>  
>  /// A writer for [`UserSlice`].
> @@ -383,7 +438,6 @@ pub fn write<T: AsBytes>(&mut self, value: &T) -> Result {
>  /// initialized and non-zero. Furthermore, if `len < dst.len()`, then `dst[len]` is a NUL byte.
>  /// Unsafe code may rely on these guarantees.
>  #[inline]
> -#[expect(dead_code)]
>  fn raw_strncpy_from_user(dst: &mut [MaybeUninit<u8>], src: UserPtr) -> Result<usize> {
>      // CAST: Slice lengths are guaranteed to be `<= isize::MAX`.
>      let len = dst.len() as isize;

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

[Alice Ryhl]

On Fri, May 30, 2025 at 8:16 PM Benno Lossin <lossin@kernel.org> wrote:
>
> On Tue May 27, 2025 at 2:34 PM CEST, Alice Ryhl wrote:
> > 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.
> >
> > Reviewed-by: Danilo Krummrich <dakr@kernel.org>
> > Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> > ---
> >  rust/kernel/uaccess.rs | 56 +++++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 55 insertions(+), 1 deletion(-)
> >
> > diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs
> > index 9b1e4016fca2c25a44a8417c7e35e0fcf08aa959..e6534b52a1920254d61f8349426d4cdb38286089 100644
> > --- a/rust/kernel/uaccess.rs
> > +++ b/rust/kernel/uaccess.rs
> > @@ -293,6 +293,61 @@ 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(dst, self.ptr)?;
> > +        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 };
>
> In this case you're overwriting the last character read. Should we give
> `raw_strncpy_from_user` access to one less byte and then write NUL into
> that?

Why? I'm not interested in changing the implementation just because.
It needs to be significantly simpler, and I do not think it is.

Alice

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

[Benno Lossin]

On Sat May 31, 2025 at 3:25 PM CEST, Alice Ryhl wrote:
> On Fri, May 30, 2025 at 8:16 PM Benno Lossin <lossin@kernel.org> wrote:
>> On Tue May 27, 2025 at 2:34 PM CEST, Alice Ryhl wrote:
>> > 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.
>> >
>> > Reviewed-by: Danilo Krummrich <dakr@kernel.org>
>> > Signed-off-by: Alice Ryhl <aliceryhl@google.com>
>> > ---
>> >  rust/kernel/uaccess.rs | 56 +++++++++++++++++++++++++++++++++++++++++++++++++-
>> >  1 file changed, 55 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs
>> > index 9b1e4016fca2c25a44a8417c7e35e0fcf08aa959..e6534b52a1920254d61f8349426d4cdb38286089 100644
>> > --- a/rust/kernel/uaccess.rs
>> > +++ b/rust/kernel/uaccess.rs
>> > @@ -293,6 +293,61 @@ 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(dst, self.ptr)?;
>> > +        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 };
>>
>> In this case you're overwriting the last character read. Should we give
>> `raw_strncpy_from_user` access to one less byte and then write NUL into
>> that?
>
> Why? I'm not interested in changing the implementation just because.
> It needs to be significantly simpler, and I do not think it is.

Sure, but then I think we should document this behavior.

---
Cheers,
Benno

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

[Alice Ryhl]

On Sat, May 31, 2025 at 5:25 PM Benno Lossin <lossin@kernel.org> wrote:
>
> On Sat May 31, 2025 at 3:25 PM CEST, Alice Ryhl wrote:
> > On Fri, May 30, 2025 at 8:16 PM Benno Lossin <lossin@kernel.org> wrote:
> >> On Tue May 27, 2025 at 2:34 PM CEST, Alice Ryhl wrote:
> >> > 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.
> >> >
> >> > Reviewed-by: Danilo Krummrich <dakr@kernel.org>
> >> > Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> >> > ---
> >> >  rust/kernel/uaccess.rs | 56 +++++++++++++++++++++++++++++++++++++++++++++++++-
> >> >  1 file changed, 55 insertions(+), 1 deletion(-)
> >> >
> >> > diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs
> >> > index 9b1e4016fca2c25a44a8417c7e35e0fcf08aa959..e6534b52a1920254d61f8349426d4cdb38286089 100644
> >> > --- a/rust/kernel/uaccess.rs
> >> > +++ b/rust/kernel/uaccess.rs
> >> > @@ -293,6 +293,61 @@ 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(dst, self.ptr)?;
> >> > +        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 };
> >>
> >> In this case you're overwriting the last character read. Should we give
> >> `raw_strncpy_from_user` access to one less byte and then write NUL into
> >> that?
> >
> > Why? I'm not interested in changing the implementation just because.
> > It needs to be significantly simpler, and I do not think it is.
>
> Sure, but then I think we should document this behavior.

Document what? I understood your suggestion as a change to the
implementation of strcpy_into_buf that would not change its behavior.
Did I misunderstand?

Alice

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

[Benno Lossin]

On Sat May 31, 2025 at 7:38 PM CEST, Alice Ryhl wrote:
> On Sat, May 31, 2025 at 5:25 PM Benno Lossin <lossin@kernel.org> wrote:
>> On Sat May 31, 2025 at 3:25 PM CEST, Alice Ryhl wrote:
>> > On Fri, May 30, 2025 at 8:16 PM Benno Lossin <lossin@kernel.org> wrote:
>> >> On Tue May 27, 2025 at 2:34 PM CEST, Alice Ryhl wrote:
>> >> > 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.
>> >> >
>> >> > Reviewed-by: Danilo Krummrich <dakr@kernel.org>
>> >> > Signed-off-by: Alice Ryhl <aliceryhl@google.com>
>> >> > ---
>> >> >  rust/kernel/uaccess.rs | 56 +++++++++++++++++++++++++++++++++++++++++++++++++-
>> >> >  1 file changed, 55 insertions(+), 1 deletion(-)
>> >> >
>> >> > diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs
>> >> > index 9b1e4016fca2c25a44a8417c7e35e0fcf08aa959..e6534b52a1920254d61f8349426d4cdb38286089 100644
>> >> > --- a/rust/kernel/uaccess.rs
>> >> > +++ b/rust/kernel/uaccess.rs
>> >> > @@ -293,6 +293,61 @@ 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(dst, self.ptr)?;
>> >> > +        if len < dst.len() {
>> >> > +            // Add one to include the NUL-terminator.
>> >> > +            //
>> >> > +            // 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 };
>> >>
>> >> In this case you're overwriting the last character read. Should we give
>> >> `raw_strncpy_from_user` access to one less byte and then write NUL into
>> >> that?
>> >
>> > Why? I'm not interested in changing the implementation just because.
>> > It needs to be significantly simpler, and I do not think it is.
>>
>> Sure, but then I think we should document this behavior.
>
> Document what? I understood your suggestion as a change to the
> implementation of strcpy_into_buf that would not change its behavior.
> Did I misunderstand?

Maybe I misunderstood the code, but if you do this:

    let slice = UserSlice::new(ptr, 1024);
    let mut buf = [0; 42];
    let s = slice.strcpy_into_buf(&mut buf)?;

Then it will read 42 characters from userspace and (if there was no nul
byte) overwrite the last character with `\0`. If we now do

    let mut buf2 = [0; 42];
    let s2 = slice.strcpy_into_buf(&mut buf2)?;

Then that will continue the read at index 42, but effectively one
character will get skipped.

(Now it's not possible to call `strcpy_into_buf` multiple times, but I
see no real reason why it isn't a `&mut self` method. Also a user could
call `clone_reader` and then manually `skip` 42 bytes. Although they
might only skip 41 bytes, since that's the length of the CStr. But that
runs into the problem that if there was a `\0` at index 41, then
repeated uses of the pattern above will yield empty strings.)

---
Cheers,
Benno

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

[Alice Ryhl]

On Sat, May 31, 2025 at 10:38 PM Benno Lossin <lossin@kernel.org> wrote:
>
> On Sat May 31, 2025 at 7:38 PM CEST, Alice Ryhl wrote:
> > On Sat, May 31, 2025 at 5:25 PM Benno Lossin <lossin@kernel.org> wrote:
> >> On Sat May 31, 2025 at 3:25 PM CEST, Alice Ryhl wrote:
> >> > On Fri, May 30, 2025 at 8:16 PM Benno Lossin <lossin@kernel.org> wrote:
> >> >> On Tue May 27, 2025 at 2:34 PM CEST, Alice Ryhl wrote:
> >> >> > 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.
> >> >> >
> >> >> > Reviewed-by: Danilo Krummrich <dakr@kernel.org>
> >> >> > Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> >> >> > ---
> >> >> >  rust/kernel/uaccess.rs | 56 +++++++++++++++++++++++++++++++++++++++++++++++++-
> >> >> >  1 file changed, 55 insertions(+), 1 deletion(-)
> >> >> >
> >> >> > diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs
> >> >> > index 9b1e4016fca2c25a44a8417c7e35e0fcf08aa959..e6534b52a1920254d61f8349426d4cdb38286089 100644
> >> >> > --- a/rust/kernel/uaccess.rs
> >> >> > +++ b/rust/kernel/uaccess.rs
> >> >> > @@ -293,6 +293,61 @@ 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(dst, self.ptr)?;
> >> >> > +        if len < dst.len() {
> >> >> > +            // Add one to include the NUL-terminator.
> >> >> > +            //
> >> >> > +            // 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 };
> >> >>
> >> >> In this case you're overwriting the last character read. Should we give
> >> >> `raw_strncpy_from_user` access to one less byte and then write NUL into
> >> >> that?
> >> >
> >> > Why? I'm not interested in changing the implementation just because.
> >> > It needs to be significantly simpler, and I do not think it is.
> >>
> >> Sure, but then I think we should document this behavior.
> >
> > Document what? I understood your suggestion as a change to the
> > implementation of strcpy_into_buf that would not change its behavior.
> > Did I misunderstand?
>
> Maybe I misunderstood the code, but if you do this:
>
>     let slice = UserSlice::new(ptr, 1024);
>     let mut buf = [0; 42];
>     let s = slice.strcpy_into_buf(&mut buf)?;
>
> Then it will read 42 characters from userspace and (if there was no nul
> byte) overwrite the last character with `\0`. If we now do
>
>     let mut buf2 = [0; 42];
>     let s2 = slice.strcpy_into_buf(&mut buf2)?;
>
> Then that will continue the read at index 42, but effectively one
> character will get skipped.
>
> (Now it's not possible to call `strcpy_into_buf` multiple times, but I
> see no real reason why it isn't a `&mut self` method. Also a user could
> call `clone_reader` and then manually `skip` 42 bytes. Although they
> might only skip 41 bytes, since that's the length of the CStr. But that
> runs into the problem that if there was a `\0` at index 41, then
> repeated uses of the pattern above will yield empty strings.)

I removed the ability to call it multiple times to avoid dealing with
this kind of question. You may submit a follow-up patch to change it
if you have a use-case.

Alice

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

[Benno Lossin]

On Sat May 31, 2025 at 11:09 PM CEST, Alice Ryhl wrote:
> On Sat, May 31, 2025 at 10:38 PM Benno Lossin <lossin@kernel.org> wrote:
>> Maybe I misunderstood the code, but if you do this:
>>
>>     let slice = UserSlice::new(ptr, 1024);
>>     let mut buf = [0; 42];
>>     let s = slice.strcpy_into_buf(&mut buf)?;
>>
>> Then it will read 42 characters from userspace and (if there was no nul
>> byte) overwrite the last character with `\0`. If we now do
>>
>>     let mut buf2 = [0; 42];
>>     let s2 = slice.strcpy_into_buf(&mut buf2)?;
>>
>> Then that will continue the read at index 42, but effectively one
>> character will get skipped.
>>
>> (Now it's not possible to call `strcpy_into_buf` multiple times, but I
>> see no real reason why it isn't a `&mut self` method. Also a user could
>> call `clone_reader` and then manually `skip` 42 bytes. Although they
>> might only skip 41 bytes, since that's the length of the CStr. But that
>> runs into the problem that if there was a `\0` at index 41, then
>> repeated uses of the pattern above will yield empty strings.)
>
> I removed the ability to call it multiple times to avoid dealing with
> this kind of question. You may submit a follow-up patch to change it
> if you have a use-case.

I don't have a use-case, but we should document this behavior somewhere
especially since the ability to only call this function once guarantees
the correctness.

---
Cheers,
Benno

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

[Alice Ryhl]

On Sun, Jun 01, 2025 at 06:09:26PM +0200, Benno Lossin wrote:
> On Sat May 31, 2025 at 11:09 PM CEST, Alice Ryhl wrote:
> > On Sat, May 31, 2025 at 10:38 PM Benno Lossin <lossin@kernel.org> wrote:
> >> Maybe I misunderstood the code, but if you do this:
> >>
> >>     let slice = UserSlice::new(ptr, 1024);
> >>     let mut buf = [0; 42];
> >>     let s = slice.strcpy_into_buf(&mut buf)?;
> >>
> >> Then it will read 42 characters from userspace and (if there was no nul
> >> byte) overwrite the last character with `\0`. If we now do
> >>
> >>     let mut buf2 = [0; 42];
> >>     let s2 = slice.strcpy_into_buf(&mut buf2)?;
> >>
> >> Then that will continue the read at index 42, but effectively one
> >> character will get skipped.
> >>
> >> (Now it's not possible to call `strcpy_into_buf` multiple times, but I
> >> see no real reason why it isn't a `&mut self` method. Also a user could
> >> call `clone_reader` and then manually `skip` 42 bytes. Although they
> >> might only skip 41 bytes, since that's the length of the CStr. But that
> >> runs into the problem that if there was a `\0` at index 41, then
> >> repeated uses of the pattern above will yield empty strings.)
> >
> > I removed the ability to call it multiple times to avoid dealing with
> > this kind of question. You may submit a follow-up patch to change it
> > if you have a use-case.
> 
> I don't have a use-case, but we should document this behavior somewhere
> especially since the ability to only call this function once guarantees
> the correctness.

I'll add a comment, though I would note that what we pass to
strncpy_from_user isn't really relevant here, even if the method was
&mut self. In that case, the thing that matters is how much we change
self.length by.

Alice