[PATCH v2 0/2] strncpy_from_user for Rust

[PATCH v2 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 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 | 69 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 68 insertions(+), 1 deletion(-)
---
base-commit: 9c32cda43eb78f78c73aee4aa344b777714e259b
change-id: 20250424-strncpy-from-user-1f2d06b0cdde

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

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

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

diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs
index 80a9782b1c6e98ed6eae308ade8551afa7adc188..acb703f074a30e60d42a222dd26aed80d8bdb76a 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,35 @@ 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. 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]
+pub 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.901.g37484f566f-goog

[PATCH v2 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 | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs
index acb703f074a30e60d42a222dd26aed80d8bdb76a..7cec1b62bd8b816f523c8be12cb29905740789fc 100644
--- a/rust/kernel/uaccess.rs
+++ b/rust/kernel/uaccess.rs
@@ -293,6 +293,41 @@ 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 append it to `dst`.
+    ///
+    /// Fails with [`EFAULT`] if the read happens on a bad address.
+    pub fn strcpy_into_buf<'buf>(&mut 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() {
+            // We hit the `self.length` limit before `buf.len()`.
+            return Err(EFAULT);
+        } else {
+            // SAFETY: Due to the check at the beginning, the buffer is not empty.
+            unsafe { *buf.last_mut().unwrap_unchecked() = 0 };
+        }
+        self.skip(len)?;
+
+        // 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`].

-- 
2.49.0.901.g37484f566f-goog

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

[Danilo Krummrich]

On Tue, Apr 29, 2025 at 09:02:22AM +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.

If we can't think of a use-case to be built upon outside of
rust/kernel/uaccess.rs, I'd make it private. We can still make it public should
we find a use-case later on. If we have one already, it's fine of course.

With that,

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

> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
>  rust/kernel/uaccess.rs | 34 +++++++++++++++++++++++++++++++++-
>  1 file changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs
> index 80a9782b1c6e98ed6eae308ade8551afa7adc188..acb703f074a30e60d42a222dd26aed80d8bdb76a 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,35 @@ 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. 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]
> +pub 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.901.g37484f566f-goog
> 

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

[Alice Ryhl]

On Tue, Apr 29, 2025 at 12:11 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Tue, Apr 29, 2025 at 09:02:22AM +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.
>
> If we can't think of a use-case to be built upon outside of
> rust/kernel/uaccess.rs, I'd make it private. We can still make it public should
> we find a use-case later on. If we have one already, it's fine of course.
>
> With that,
>
>         Reviewed-by: Danilo Krummrich <dakr@kernel.org>

Ok, I can drop the pub.

Alice

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

[Danilo Krummrich]

On Tue, Apr 29, 2025 at 09:02:23AM +0000, 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.
> 
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
>  rust/kernel/uaccess.rs | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs
> index acb703f074a30e60d42a222dd26aed80d8bdb76a..7cec1b62bd8b816f523c8be12cb29905740789fc 100644
> --- a/rust/kernel/uaccess.rs
> +++ b/rust/kernel/uaccess.rs
> @@ -293,6 +293,41 @@ 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 append it to `dst`.
> +    ///
> +    /// Fails with [`EFAULT`] if the read happens on a bad address.
> +    pub fn strcpy_into_buf<'buf>(&mut 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() {
> +            // We hit the `self.length` limit before `buf.len()`.
> +            return Err(EFAULT);

So, this one we can only ever hit if `len == dst.len()`, which means that the
string (incl. the NULL terminator) is longer than dst. If at the same time
`len < buf.len()`, we know that dst has been shortened because
`buf.len() > self.length`, which means that the string spans across the
self.length boundary.

That seems a bit subtle to me. Maybe we should check for `dst.len() < buf.len()`
instead and add a comment explaining the logic a bit more in detail.

> +        } else {
> +            // SAFETY: Due to the check at the beginning, the buffer is not empty.
> +            unsafe { *buf.last_mut().unwrap_unchecked() = 0 };
> +        }
> +        self.skip(len)?;
> +
> +        // 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`].
> 
> -- 
> 2.49.0.901.g37484f566f-goog
> 

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

[Greg Kroah-Hartman]

On Tue, Apr 29, 2025 at 09:02:22AM +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.
> 
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
>  rust/kernel/uaccess.rs | 34 +++++++++++++++++++++++++++++++++-
>  1 file changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs
> index 80a9782b1c6e98ed6eae308ade8551afa7adc188..acb703f074a30e60d42a222dd26aed80d8bdb76a 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,35 @@ 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. 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.

I don't know if it matters, but this can fill up the buffer a bit and
still fail, to quote from the strncpy_from_user() documentation:

	 If access to userspace fails, returns -EFAULT (some data may have been copied).

> +///
> +/// # 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]
> +pub 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));

Nit, this can just be returning EFAULT, but I guess it's safest just to
mirror what was passed back.

I would say to just leave it as "pub" for now, but that's not a big
deal.

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

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

[Greg Kroah-Hartman]

On Tue, Apr 29, 2025 at 09:02:23AM +0000, 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.
> 
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
>  rust/kernel/uaccess.rs | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs
> index acb703f074a30e60d42a222dd26aed80d8bdb76a..7cec1b62bd8b816f523c8be12cb29905740789fc 100644
> --- a/rust/kernel/uaccess.rs
> +++ b/rust/kernel/uaccess.rs
> @@ -293,6 +293,41 @@ 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 append it to `dst`.
> +    ///
> +    /// Fails with [`EFAULT`] if the read happens on a bad address.

Also returns this error:

> +    pub fn strcpy_into_buf<'buf>(&mut self, buf: &'buf mut [u8]) -> Result<&'buf CStr> {
> +        if buf.is_empty() {
> +            return Err(EINVAL);

if the buffer is of 0 length.  Don't know if you want to document that
or not.

> +        }
> +
> +        // 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() {
> +            // We hit the `self.length` limit before `buf.len()`.
> +            return Err(EFAULT);

How can this happen?  And if it does, why is that a memory fault?
Doesn't this just mean that we read smaller than our overall size of our
buffer?  Or am I misreading this completely?

Maybe a self-test would be good to exercise all of this :)

thanks,

greg k-h

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

[Danilo Krummrich]

On 2025-04-29 13:09, Greg Kroah-Hartman wrote:
> On Tue, Apr 29, 2025 at 09:02:23AM +0000, 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.
>> 
>> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
>> ---
>>  rust/kernel/uaccess.rs | 35 +++++++++++++++++++++++++++++++++++
>>  1 file changed, 35 insertions(+)
>> 
>> diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs
>> index 
>> acb703f074a30e60d42a222dd26aed80d8bdb76a..7cec1b62bd8b816f523c8be12cb29905740789fc 
>> 100644
>> --- a/rust/kernel/uaccess.rs
>> +++ b/rust/kernel/uaccess.rs
>> @@ -293,6 +293,41 @@ 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 append it to 
>> `dst`.
>> +    ///
>> +    /// Fails with [`EFAULT`] if the read happens on a bad address.
> 
> Also returns this error:
> 
>> +    pub fn strcpy_into_buf<'buf>(&mut self, buf: &'buf mut [u8]) -> 
>> Result<&'buf CStr> {
>> +        if buf.is_empty() {
>> +            return Err(EINVAL);
> 
> if the buffer is of 0 length.  Don't know if you want to document that
> or not.
> 
>> +        }
>> +
>> +        // 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() {
>> +            // We hit the `self.length` limit before `buf.len()`.
>> +            return Err(EFAULT);
> 
> How can this happen?

See my reply here (if I did not get it wrong):

https://lore.kernel.org/rust-for-linux/aBCrqJe4two4I45G@pollux/

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

[Greg Kroah-Hartman]

On Tue, Apr 29, 2025 at 01:38:26PM +0200, Danilo Krummrich wrote:
> On 2025-04-29 13:09, Greg Kroah-Hartman wrote:
> > On Tue, Apr 29, 2025 at 09:02:23AM +0000, 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.
> > > 
> > > Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> > > ---
> > >  rust/kernel/uaccess.rs | 35 +++++++++++++++++++++++++++++++++++
> > >  1 file changed, 35 insertions(+)
> > > 
> > > diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs
> > > index acb703f074a30e60d42a222dd26aed80d8bdb76a..7cec1b62bd8b816f523c8be12cb29905740789fc
> > > 100644
> > > --- a/rust/kernel/uaccess.rs
> > > +++ b/rust/kernel/uaccess.rs
> > > @@ -293,6 +293,41 @@ 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 append it
> > > to `dst`.
> > > +    ///
> > > +    /// Fails with [`EFAULT`] if the read happens on a bad address.
> > 
> > Also returns this error:
> > 
> > > +    pub fn strcpy_into_buf<'buf>(&mut self, buf: &'buf mut [u8]) ->
> > > Result<&'buf CStr> {
> > > +        if buf.is_empty() {
> > > +            return Err(EINVAL);
> > 
> > if the buffer is of 0 length.  Don't know if you want to document that
> > or not.
> > 
> > > +        }
> > > +
> > > +        // 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() {
> > > +            // We hit the `self.length` limit before `buf.len()`.
> > > +            return Err(EFAULT);
> > 
> > How can this happen?
> 
> See my reply here (if I did not get it wrong):
> 
> https://lore.kernel.org/rust-for-linux/aBCrqJe4two4I45G@pollux/

Ah, I should have read ahead :)

I agree, some comments here would be good.  We want everyone to be able
to easily read and understand this code, off-by-one errors are rough.

thanks,

greg k-h

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

[Alice Ryhl]

On Tue, Apr 29, 2025 at 01:04:44PM +0200, Greg Kroah-Hartman wrote:
> On Tue, Apr 29, 2025 at 09:02:22AM +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.
> > 
> > Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> > ---
> >  rust/kernel/uaccess.rs | 34 +++++++++++++++++++++++++++++++++-
> >  1 file changed, 33 insertions(+), 1 deletion(-)
> > 
> > diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs
> > index 80a9782b1c6e98ed6eae308ade8551afa7adc188..acb703f074a30e60d42a222dd26aed80d8bdb76a 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,35 @@ 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. 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.
> 
> I don't know if it matters, but this can fill up the buffer a bit and
> still fail, to quote from the strncpy_from_user() documentation:
> 
> 	 If access to userspace fails, returns -EFAULT (some data may have been copied).

It doesn't matter, but it may still be useful to mention.

> > +///
> > +/// # 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]
> > +pub 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));
> 
> Nit, this can just be returning EFAULT, but I guess it's safest just to
> mirror what was passed back.

I think it's easiest to just mirror what was passed back.

> I would say to just leave it as "pub" for now, but that's not a big
> deal.
> 
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Thanks!

Alice

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

[Alice Ryhl]

On Tue, Apr 29, 2025 at 01:48:19PM +0200, Greg Kroah-Hartman wrote:
> On Tue, Apr 29, 2025 at 01:38:26PM +0200, Danilo Krummrich wrote:
> > On 2025-04-29 13:09, Greg Kroah-Hartman wrote:
> > > On Tue, Apr 29, 2025 at 09:02:23AM +0000, Alice Ryhl wrote:
> > > > +        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() {
> > > > +            // We hit the `self.length` limit before `buf.len()`.
> > > > +            return Err(EFAULT);
> > > 
> > > How can this happen?
> > 
> > See my reply here (if I did not get it wrong):
> > 
> > https://lore.kernel.org/rust-for-linux/aBCrqJe4two4I45G@pollux/
> 
> Ah, I should have read ahead :)
> 
> I agree, some comments here would be good.  We want everyone to be able
> to easily read and understand this code, off-by-one errors are rough.

I will add this comment:

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

Alice

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

[Greg Kroah-Hartman]

On Tue, Apr 29, 2025 at 03:15:01PM +0000, Alice Ryhl wrote:
> On Tue, Apr 29, 2025 at 01:48:19PM +0200, Greg Kroah-Hartman wrote:
> > On Tue, Apr 29, 2025 at 01:38:26PM +0200, Danilo Krummrich wrote:
> > > On 2025-04-29 13:09, Greg Kroah-Hartman wrote:
> > > > On Tue, Apr 29, 2025 at 09:02:23AM +0000, Alice Ryhl wrote:
> > > > > +        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() {
> > > > > +            // We hit the `self.length` limit before `buf.len()`.
> > > > > +            return Err(EFAULT);
> > > > 
> > > > How can this happen?
> > > 
> > > See my reply here (if I did not get it wrong):
> > > 
> > > https://lore.kernel.org/rust-for-linux/aBCrqJe4two4I45G@pollux/
> > 
> > Ah, I should have read ahead :)
> > 
> > I agree, some comments here would be good.  We want everyone to be able
> > to easily read and understand this code, off-by-one errors are rough.
> 
> I will add this comment:
> 
> 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);

Looks great, thanks!

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

[Boqun Feng]

On Tue, Apr 29, 2025 at 09:02:22AM +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.
> 
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
>  rust/kernel/uaccess.rs | 34 +++++++++++++++++++++++++++++++++-
>  1 file changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs
> index 80a9782b1c6e98ed6eae308ade8551afa7adc188..acb703f074a30e60d42a222dd26aed80d8bdb76a 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,35 @@ 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. 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]
> +pub 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));
> +    }
> +

Nit: this can be a

	let copy_len = kernel::error::to_result(res)?;

Other than that,

Reviewed-by: Boqun Feng <boqun.feng@gmail.com>

Regards,
Boqun

> +    #[cfg(CONFIG_RUST_OVERFLOW_CHECKS)]
> +    assert!(res <= len);
> +
> +    Ok(res as usize)
> +}
> 
> -- 
> 2.49.0.901.g37484f566f-goog
> 

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

[Boqun Feng]

On Tue, Apr 29, 2025 at 09:02:23AM +0000, 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.
> 
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
>  rust/kernel/uaccess.rs | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs
> index acb703f074a30e60d42a222dd26aed80d8bdb76a..7cec1b62bd8b816f523c8be12cb29905740789fc 100644
> --- a/rust/kernel/uaccess.rs
> +++ b/rust/kernel/uaccess.rs
> @@ -293,6 +293,41 @@ 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 append it to `dst`.

s/`dst`/`buf`

?

> +    ///
> +    /// Fails with [`EFAULT`] if the read happens on a bad address.
> +    pub fn strcpy_into_buf<'buf>(&mut 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>]) };

maybe:

	let mut dst = unsafe { &mut *(ptr::from_mut(buf).cast() };

? To align with:

	https://lore.kernel.org/rust-for-linux/20250418-ptr-as-ptr-v10-0-3d63d27907aa@gmail.com/	

> +
> +        // 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() {
> +            // We hit the `self.length` limit before `buf.len()`.
> +            return Err(EFAULT);
> +        } else {
> +            // SAFETY: Due to the check at the beginning, the buffer is not empty.
> +            unsafe { *buf.last_mut().unwrap_unchecked() = 0 };
> +        }
> +        self.skip(len)?;
> +

So if the UserSlice content is "abcdefg" (not tailing NUL), and the buf
size is 4, after a strcpy_into_buf(), the return would be a CStr "abc"
(with a tailing NUL), and the UserSlice would move 4 bytes and become
"edg" (not tailing NUL), is this a desired behavior?

Alternatively, we can make `dst` always 1 byte less then `buf`, so that
in the above case, UserSlice will only move 3 bytes and become "defg",
and the return CStr is still "abc" (with a tailing NUL).

The current behavior makes me feel like we can lose some information,
for example, if the user-kernel protocol is that "a userslice that
contains 4 64-byte strings which don't have a tailing NUL", we cannot do
4 strcpy_into_buf() to get them, right? But of course, the scenario is
completely made up, just food for thoughts.

Regards,
Boqun

> +        // 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`].
> 
> -- 
> 2.49.0.901.g37484f566f-goog
> 

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

[Boqun Feng]

On Tue, Apr 29, 2025 at 11:02:07AM -0700, Boqun Feng wrote:
> On Tue, Apr 29, 2025 at 09:02:23AM +0000, 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.
> > 
> > Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> > ---
> >  rust/kernel/uaccess.rs | 35 +++++++++++++++++++++++++++++++++++
> >  1 file changed, 35 insertions(+)
> > 
> > diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs
> > index acb703f074a30e60d42a222dd26aed80d8bdb76a..7cec1b62bd8b816f523c8be12cb29905740789fc 100644
> > --- a/rust/kernel/uaccess.rs
> > +++ b/rust/kernel/uaccess.rs
> > @@ -293,6 +293,41 @@ 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 append it to `dst`.
> 
> s/`dst`/`buf`
> 
> ?
> 
> > +    ///
> > +    /// Fails with [`EFAULT`] if the read happens on a bad address.
> > +    pub fn strcpy_into_buf<'buf>(&mut 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>]) };
> 
> maybe:
> 
> 	let mut dst = unsafe { &mut *(ptr::from_mut(buf).cast() };
> 
> ? To align with:
> 
> 	https://lore.kernel.org/rust-for-linux/20250418-ptr-as-ptr-v10-0-3d63d27907aa@gmail.com/	
> 
> > +
> > +        // 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() {
> > +            // We hit the `self.length` limit before `buf.len()`.
> > +            return Err(EFAULT);
> > +        } else {
> > +            // SAFETY: Due to the check at the beginning, the buffer is not empty.
> > +            unsafe { *buf.last_mut().unwrap_unchecked() = 0 };
> > +        }
> > +        self.skip(len)?;
> > +
> 
> So if the UserSlice content is "abcdefg" (not tailing NUL), and the buf
> size is 4, after a strcpy_into_buf(), the return would be a CStr "abc"
> (with a tailing NUL), and the UserSlice would move 4 bytes and become
> "edg" (not tailing NUL), is this a desired behavior?
> 
> Alternatively, we can make `dst` always 1 byte less then `buf`, so that

Hmm.. this part is not correct, what we should do is:

        // 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;
	    self.skip(len)?;
        } else if len < buf.len() {
            // We hit the `self.length` limit before `buf.len()`.
            return Err(EFAULT);
        } else {
            // SAFETY: Due to the check at the beginning, the buffer is not empty.
            unsafe { *buf.last_mut().unwrap_unchecked() = 0 };

	    // if any copy really happened, and we don't find a NUL char
	    // until the end of the buf/dst, we will add a NUL char as
	    // above, but in this case, we need to not skip the last
	    // char in `self` (because it's overwritten in the returning
	    // string by a NUL char).
	    if dst.len() != 0 { 
	        self.skip(len - 1)?;
	    }
        }

Of course, the code can be re-organized, but this is the idea.

Regards,
Boqun

> in the above case, UserSlice will only move 3 bytes and become "defg",
> and the return CStr is still "abc" (with a tailing NUL).
> 
> The current behavior makes me feel like we can lose some information,
> for example, if the user-kernel protocol is that "a userslice that
> contains 4 64-byte strings which don't have a tailing NUL", we cannot do
> 4 strcpy_into_buf() to get them, right? But of course, the scenario is
> completely made up, just food for thoughts.
> 
> Regards,
> Boqun
> 
> > +        // 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`].
> > 
> > -- 
> > 2.49.0.901.g37484f566f-goog
> > 

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

[Alice Ryhl]

On Tue, Apr 29, 2025 at 8:02 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> On Tue, Apr 29, 2025 at 09:02:23AM +0000, 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.
> >
> > Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> > ---
> >  rust/kernel/uaccess.rs | 35 +++++++++++++++++++++++++++++++++++
> >  1 file changed, 35 insertions(+)
> >
> > diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs
> > index acb703f074a30e60d42a222dd26aed80d8bdb76a..7cec1b62bd8b816f523c8be12cb29905740789fc 100644
> > --- a/rust/kernel/uaccess.rs
> > +++ b/rust/kernel/uaccess.rs
> > @@ -293,6 +293,41 @@ 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 append it to `dst`.
>
> s/`dst`/`buf`
>
> ?

Hm, append is also wrong. Thanks.

> > +
> > +        // 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() {
> > +            // We hit the `self.length` limit before `buf.len()`.
> > +            return Err(EFAULT);
> > +        } else {
> > +            // SAFETY: Due to the check at the beginning, the buffer is not empty.
> > +            unsafe { *buf.last_mut().unwrap_unchecked() = 0 };
> > +        }
> > +        self.skip(len)?;
> > +
>
> So if the UserSlice content is "abcdefg" (not tailing NUL), and the buf
> size is 4, after a strcpy_into_buf(), the return would be a CStr "abc"
> (with a tailing NUL), and the UserSlice would move 4 bytes and become
> "edg" (not tailing NUL), is this a desired behavior?
>
> Alternatively, we can make `dst` always 1 byte less then `buf`, so that
> in the above case, UserSlice will only move 3 bytes and become "defg",
> and the return CStr is still "abc" (with a tailing NUL).

Maybe we just have this method consume the UserSliceReader and avoid
thinking about what happens if you use it afterwards.

> The current behavior makes me feel like we can lose some information,
> for example, if the user-kernel protocol is that "a userslice that
> contains 4 64-byte strings which don't have a tailing NUL", we cannot do
> 4 strcpy_into_buf() to get them, right? But of course, the scenario is
> completely made up, just food for thoughts.

But then you should probably just read the [u8;64] type four times?

Alice

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

[Boqun Feng]

On Tue, Apr 29, 2025 at 09:29:07PM +0200, Alice Ryhl wrote:
> On Tue, Apr 29, 2025 at 8:02 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> >
> > On Tue, Apr 29, 2025 at 09:02:23AM +0000, 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.
> > >
> > > Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> > > ---
> > >  rust/kernel/uaccess.rs | 35 +++++++++++++++++++++++++++++++++++
> > >  1 file changed, 35 insertions(+)
> > >
> > > diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs
> > > index acb703f074a30e60d42a222dd26aed80d8bdb76a..7cec1b62bd8b816f523c8be12cb29905740789fc 100644
> > > --- a/rust/kernel/uaccess.rs
> > > +++ b/rust/kernel/uaccess.rs
> > > @@ -293,6 +293,41 @@ 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 append it to `dst`.
> >
> > s/`dst`/`buf`
> >
> > ?
> 
> Hm, append is also wrong. Thanks.
> 
> > > +
> > > +        // 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() {
> > > +            // We hit the `self.length` limit before `buf.len()`.
> > > +            return Err(EFAULT);
> > > +        } else {
> > > +            // SAFETY: Due to the check at the beginning, the buffer is not empty.
> > > +            unsafe { *buf.last_mut().unwrap_unchecked() = 0 };
> > > +        }
> > > +        self.skip(len)?;
> > > +
> >
> > So if the UserSlice content is "abcdefg" (not tailing NUL), and the buf
> > size is 4, after a strcpy_into_buf(), the return would be a CStr "abc"
> > (with a tailing NUL), and the UserSlice would move 4 bytes and become
> > "edg" (not tailing NUL), is this a desired behavior?
> >
> > Alternatively, we can make `dst` always 1 byte less then `buf`, so that
> > in the above case, UserSlice will only move 3 bytes and become "defg",
> > and the return CStr is still "abc" (with a tailing NUL).
> 
> Maybe we just have this method consume the UserSliceReader and avoid
> thinking about what happens if you use it afterwards.
> 
> > The current behavior makes me feel like we can lose some information,
> > for example, if the user-kernel protocol is that "a userslice that
> > contains 4 64-byte strings which don't have a tailing NUL", we cannot do
> > 4 strcpy_into_buf() to get them, right? But of course, the scenario is
> > completely made up, just food for thoughts.
> 
> But then you should probably just read the [u8;64] type four times?
> 

Ah, that makes sense. Seems I was trying to over-task this method ;-)

Regards,
Boqun

> Alice

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

[John Hubbard]

On 4/29/25 10:30 AM, Boqun Feng wrote:
> On Tue, Apr 29, 2025 at 09:02:22AM +0000, Alice Ryhl wrote:
...
>> +#[inline]
>> +pub 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));
>> +    }
>> +
> 
> Nit: this can be a
> 
> 	let copy_len = kernel::error::to_result(res)?;
> 

Doesn't that discard the length, though, by returning Ok(()) ?


thanks,
-- 
John Hubbard

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

[Boqun Feng]

On Tue, Apr 29, 2025 at 01:28:09PM -0700, John Hubbard wrote:
> On 4/29/25 10:30 AM, Boqun Feng wrote:
> > On Tue, Apr 29, 2025 at 09:02:22AM +0000, Alice Ryhl wrote:
> ...
> >> +#[inline]
> >> +pub 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));
> >> +    }
> >> +
> > 
> > Nit: this can be a
> > 
> > 	let copy_len = kernel::error::to_result(res)?;
> > 
> 
> Doesn't that discard the length, though, by returning Ok(()) ?
> 

Oh, you're right, so probably we need a to_result_i32():

    pub fn to_result_i32() -> Result<i32>

, so we don't have to open-code "if res < 0" every time.

Regards,
Boqun

> 
> thanks,
> -- 
> John Hubbard
> 

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

[Alice Ryhl]

On Tue, Apr 29, 2025 at 01:09:18PM +0200, Greg Kroah-Hartman wrote:
> On Tue, Apr 29, 2025 at 09:02:23AM +0000, 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.
> > 
> > Signed-off-by: Alice Ryhl <aliceryhl@google.com>

> > +        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() {
> > +            // We hit the `self.length` limit before `buf.len()`.
> > +            return Err(EFAULT);
> 
> How can this happen?  And if it does, why is that a memory fault?
> Doesn't this just mean that we read smaller than our overall size of our
> buffer?  Or am I misreading this completely?
> 
> Maybe a self-test would be good to exercise all of this :)

How can I test userspace access? Is there a way to create a kernel
buffer that strncpy_from_user will let you read from for use in a kunit
test?

Alice

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

[Greg Kroah-Hartman]

On Wed, Apr 30, 2025 at 10:58:10AM +0000, Alice Ryhl wrote:
> On Tue, Apr 29, 2025 at 01:09:18PM +0200, Greg Kroah-Hartman wrote:
> > On Tue, Apr 29, 2025 at 09:02:23AM +0000, 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.
> > > 
> > > Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> 
> > > +        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() {
> > > +            // We hit the `self.length` limit before `buf.len()`.
> > > +            return Err(EFAULT);
> > 
> > How can this happen?  And if it does, why is that a memory fault?
> > Doesn't this just mean that we read smaller than our overall size of our
> > buffer?  Or am I misreading this completely?
> > 
> > Maybe a self-test would be good to exercise all of this :)
> 
> How can I test userspace access? Is there a way to create a kernel
> buffer that strncpy_from_user will let you read from for use in a kunit
> test?

I think you'll need to just wire up a misc device and test it from
userspace, sorry.

thanks,

greg k-h