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-googOn 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;
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
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
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
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
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
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
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
© 2016 - 2025 Red Hat, Inc.