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-googOn 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
>
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
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
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
> >
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
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
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
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/
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
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
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!
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
>
© 2016 - 2026 Red Hat, Inc.