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

Alice Ryhl posted 2 patches 9 months, 2 weeks ago
There is a newer version of this series
[PATCH v2 1/2] uaccess: rust: add strncpy_from_user
Posted by Alice Ryhl 9 months, 2 weeks ago
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
Re: [PATCH v2 1/2] uaccess: rust: add strncpy_from_user
Posted by Boqun Feng 9 months, 2 weeks ago
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 1/2] uaccess: rust: add strncpy_from_user
Posted by John Hubbard 9 months, 2 weeks ago
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
Posted by Boqun Feng 9 months, 2 weeks ago
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 1/2] uaccess: rust: add strncpy_from_user
Posted by Greg Kroah-Hartman 9 months, 2 weeks ago
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 1/2] uaccess: rust: add strncpy_from_user
Posted by Alice Ryhl 9 months, 2 weeks ago
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 1/2] uaccess: rust: add strncpy_from_user
Posted by Danilo Krummrich 9 months, 2 weeks ago
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
Posted by Alice Ryhl 9 months, 2 weeks ago
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