[PATCH] uaccess: rust: use newtype for user pointers

[PATCH] uaccess: rust: use newtype for user pointers

[Alice Ryhl]

In C code we use sparse with the __user annotation to detect cases where
a user pointer is mixed up with other things. To replicate that, we
introduce a new struct UserPtr that serves the same purpose using the
newtype pattern.

The UserPtr type is not marked with #[derive(Debug)], which means that
it's not possible to print values of this type. This avoids ASLR
breakage.

Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 rust/kernel/uaccess.rs           | 48 ++++++++++++++++++++++++++++++++++------
 samples/rust/rust_misc_device.rs |  4 +++-
 2 files changed, 44 insertions(+), 8 deletions(-)

diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs
index 978205289d297a4001a51fa40ac29039bff73672..2914a35de3dfbc7860ebf0d6e11cc65d409e9481 100644
--- a/rust/kernel/uaccess.rs
+++ b/rust/kernel/uaccess.rs
@@ -14,8 +14,38 @@
 };
 use core::mem::{size_of, MaybeUninit};
 
-/// The type used for userspace addresses.
-pub type UserPtr = usize;
+/// A pointer into userspace.
+///
+/// This is the Rust equivalent to C pointers tagged with `__user`.
+#[repr(transparent)]
+#[derive(Copy, Clone)]
+pub struct UserPtr(pub usize);
+
+impl UserPtr {
+    /// Cast this userspace pointer to a raw const void pointer.
+    ///
+    /// It is up to the caller to use the returned pointer correctly.
+    #[inline]
+    pub fn as_const_ptr(self) -> *const c_void {
+        self.0 as *const c_void
+    }
+
+    /// Cast this userspace pointer to a raw mutable void pointer.
+    ///
+    /// It is up to the caller to use the returned pointer correctly.
+    #[inline]
+    pub fn as_mut_ptr(self) -> *mut c_void {
+        self.0 as *mut c_void
+    }
+
+    /// Increment this user pointer by `add` bytes.
+    ///
+    /// This is addition is wrapping, so wrapping around the address space does not result in a
+    /// panic even if `CONFIG_RUST_OVERFLOW_CHECKS` is enabled.
+    pub fn wrapping_add(self, add: usize) -> UserPtr {
+        UserPtr(self.0.wrapping_add(add))
+    }
+}
 
 /// A pointer to an area in userspace memory, which can be either read-only or read-write.
 ///
@@ -226,7 +256,7 @@ pub fn read_raw(&mut self, out: &mut [MaybeUninit<u8>]) -> Result {
         }
         // SAFETY: `out_ptr` points into a mutable slice of length `len`, so we may write
         // that many bytes to it.
-        let res = unsafe { bindings::copy_from_user(out_ptr, self.ptr as *const c_void, len) };
+        let res = unsafe { bindings::copy_from_user(out_ptr, self.ptr.as_const_ptr(), len) };
         if res != 0 {
             return Err(EFAULT);
         }
@@ -264,7 +294,7 @@ pub fn read<T: FromBytes>(&mut self) -> Result<T> {
         let res = unsafe {
             bindings::_copy_from_user(
                 out.as_mut_ptr().cast::<c_void>(),
-                self.ptr as *const c_void,
+                self.ptr.as_const_ptr(),
                 len,
             )
         };
@@ -381,7 +411,7 @@ pub fn write_slice(&mut self, data: &[u8]) -> Result {
         }
         // SAFETY: `data_ptr` points into an immutable slice of length `len`, so we may read
         // that many bytes from it.
-        let res = unsafe { bindings::copy_to_user(self.ptr as *mut c_void, data_ptr, len) };
+        let res = unsafe { bindings::copy_to_user(self.ptr.as_mut_ptr(), data_ptr, len) };
         if res != 0 {
             return Err(EFAULT);
         }
@@ -408,7 +438,7 @@ pub fn write<T: AsBytes>(&mut self, value: &T) -> Result {
         // is a compile-time constant.
         let res = unsafe {
             bindings::_copy_to_user(
-                self.ptr as *mut c_void,
+                self.ptr.as_mut_ptr(),
                 (value as *const T).cast::<c_void>(),
                 len,
             )
@@ -441,7 +471,11 @@ fn raw_strncpy_from_user(ptr: UserPtr, buf: &mut [MaybeUninit<u8>]) -> Result<us
 
     // 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)
+        bindings::strncpy_from_user(
+            buf.as_mut_ptr().cast::<c_char>(),
+            ptr.as_const_ptr().cast::<c_char>(),
+            len,
+        )
     };
 
     if res < 0 {
diff --git a/samples/rust/rust_misc_device.rs b/samples/rust/rust_misc_device.rs
index c881fd6dbd08cf4308fe1bd37d11d28374c1f034..6519c61636311b9ffb90d55c03c0a36520933fde 100644
--- a/samples/rust/rust_misc_device.rs
+++ b/samples/rust/rust_misc_device.rs
@@ -107,7 +107,7 @@
     prelude::*,
     sync::Mutex,
     types::ARef,
-    uaccess::{UserSlice, UserSliceReader, UserSliceWriter},
+    uaccess::{UserPtr, UserSlice, UserSliceReader, UserSliceWriter},
 };
 
 const RUST_MISC_DEV_HELLO: u32 = _IO('|' as u32, 0x80);
@@ -176,6 +176,8 @@ fn open(_file: &File, misc: &MiscDeviceRegistration<Self>) -> Result<Pin<KBox<Se
     fn ioctl(me: Pin<&RustMiscDevice>, _file: &File, cmd: u32, arg: usize) -> Result<isize> {
         dev_info!(me.dev, "IOCTLing Rust Misc Device Sample\n");
 
+        // Treat the ioctl argument as a user pointer.
+        let arg = UserPtr(arg);
         let size = _IOC_SIZE(cmd);
 
         match cmd {

---
base-commit: f34da179a4517854b2ffbe4bce8c3405bd9be04e
change-id: 20250506-userptr-newtype-2f060985c33b
prerequisite-change-id: 20250424-strncpy-from-user-1f2d06b0cdde:v3
prerequisite-patch-id: 3b99605d033602b9440a12c7ca38acd5ad071a13
prerequisite-patch-id: fae3d2f99d1b0f00a79105921dcbff30d5229b91

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

Re: [PATCH] uaccess: rust: use newtype for user pointers

[Miguel Ojeda]

On Tue, May 6, 2025 at 3:26 PM Alice Ryhl <aliceryhl@google.com> wrote:
>
> The UserPtr type is not marked with #[derive(Debug)], which means that
> it's not possible to print values of this type. This avoids ASLR
> breakage.

By breakage you mean leaking the information by mistake?

Since it is `pub`, should we make it even harder to make a mistake
here by making it private? You are already providing and using the
`as_` methods anyway, so we would only need a `new` or conversion
method or `Into` similar (not sure which one would be best -- perhaps
a single one with a descriptive name is a good idea to grep for it
easily).

> +    /// Increment this user pointer by `add` bytes.
> +    ///
> +    /// This is addition is wrapping, so wrapping around the address space does not result in a

s/is//

> +    /// panic even if `CONFIG_RUST_OVERFLOW_CHECKS` is enabled.
> +    pub fn wrapping_add(self, add: usize) -> UserPtr {
> +        UserPtr(self.0.wrapping_add(add))
> +    }
> +}

I guess you are using `wrapping_add` since we have a `usize` internal
type, but I wonder if we should use the pointer-related naming, i.e.
`wrapping_byte_add`.

Also, perhaps it is best to use another name for the parameter -- I
would pick `count` like the standard library.

In addition, should we get this directly into the `prelude`? `__user`
is also global and fairly short. It may not be heavily used all the
time like other things, but it is fairly fundamental, like the `c_*`
ones.

Thanks!

Cheers,
Miguel

Re: [PATCH] uaccess: rust: use newtype for user pointers

[Greg Kroah-Hartman]

On Tue, May 06, 2025 at 01:25:58PM +0000, Alice Ryhl wrote:
> In C code we use sparse with the __user annotation to detect cases where
> a user pointer is mixed up with other things. To replicate that, we
> introduce a new struct UserPtr that serves the same purpose using the
> newtype pattern.
> 
> The UserPtr type is not marked with #[derive(Debug)], which means that
> it's not possible to print values of this type. This avoids ASLR
> breakage.
> 
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
>  rust/kernel/uaccess.rs           | 48 ++++++++++++++++++++++++++++++++++------
>  samples/rust/rust_misc_device.rs |  4 +++-
>  2 files changed, 44 insertions(+), 8 deletions(-)
> 
> diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs
> index 978205289d297a4001a51fa40ac29039bff73672..2914a35de3dfbc7860ebf0d6e11cc65d409e9481 100644
> --- a/rust/kernel/uaccess.rs
> +++ b/rust/kernel/uaccess.rs
> @@ -14,8 +14,38 @@
>  };
>  use core::mem::{size_of, MaybeUninit};
>  
> -/// The type used for userspace addresses.
> -pub type UserPtr = usize;
> +/// A pointer into userspace.
> +///
> +/// This is the Rust equivalent to C pointers tagged with `__user`.
> +#[repr(transparent)]
> +#[derive(Copy, Clone)]
> +pub struct UserPtr(pub usize);

Again, why can't this be:
	pub struct UserPtr(pub *const u8);

This is a pointer to a stream of u8 bytes, so let's treat it like that
as that is what we are copying from here.

thanks,

greg k-h

Re: [PATCH] uaccess: rust: use newtype for user pointers

[Alice Ryhl]

On Tue, May 06, 2025 at 03:59:20PM +0200, Miguel Ojeda wrote:
> On Tue, May 6, 2025 at 3:26 PM Alice Ryhl <aliceryhl@google.com> wrote:
> >
> > The UserPtr type is not marked with #[derive(Debug)], which means that
> > it's not possible to print values of this type. This avoids ASLR
> > breakage.
> 
> By breakage you mean leaking the information by mistake?

Yeah, I'll reword to "ASLR leakage".

> Since it is `pub`, should we make it even harder to make a mistake
> here by making it private? You are already providing and using the
> `as_` methods anyway, so we would only need a `new` or conversion
> method or `Into` similar (not sure which one would be best -- perhaps
> a single one with a descriptive name is a good idea to grep for it
> easily).

If we change it to store a raw pointer, then that might be a good idea.

> > +    /// Increment this user pointer by `add` bytes.
> > +    ///
> > +    /// This is addition is wrapping, so wrapping around the address space does not result in a
> 
> s/is//
> 
> > +    /// panic even if `CONFIG_RUST_OVERFLOW_CHECKS` is enabled.
> > +    pub fn wrapping_add(self, add: usize) -> UserPtr {
> > +        UserPtr(self.0.wrapping_add(add))
> > +    }
> > +}
> 
> I guess you are using `wrapping_add` since we have a `usize` internal
> type, but I wonder if we should use the pointer-related naming, i.e.
> `wrapping_byte_add`.

That makes sense.

> Also, perhaps it is best to use another name for the parameter -- I
> would pick `count` like the standard library.

Sure.

> In addition, should we get this directly into the `prelude`? `__user`
> is also global and fairly short. It may not be heavily used all the
> time like other things, but it is fairly fundamental, like the `c_*`
> ones.

Good idea.

Alice