rust-for-linux.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] uaccess: rust: use newtype for user pointers
@ 2025-05-27 13:53 Alice Ryhl
  2025-05-27 14:42 ` Greg Kroah-Hartman
                   ` (5 more replies)
  0 siblings, 6 replies; 17+ messages in thread
From: Alice Ryhl @ 2025-05-27 13:53 UTC (permalink / raw)
  To: Miguel Ojeda, Alexander Viro, Greg Kroah-Hartman, Arnd Bergmann
  Cc: Andrew Morton, Boqun Feng, Gary Guo, Björn Roy Baron,
	Benno Lossin, Andreas Hindborg, Trevor Gross, Danilo Krummrich,
	rust-for-linux, linux-kernel, 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
leakage.

The type is added to the prelude as it is a fairly fundamental type
similar to c_int. The wrapping_add() method is renamed to
wrapping_byte_add() for consistency with the method name found on raw
pointers.

Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
This is based on top of the strncpy_from_user for Rust patch.
---
Changes in v2:
- Change usize to raw pointer.
- Make field private.
- Rename wrapping_add to wrapping_byte_add.
- Add to prelude.
- Rebase on v4 of strncpy_from_user
- Link to v1: https://lore.kernel.org/r/20250506-userptr-newtype-v1-1-a0f6f8ce9fc5@google.com
---
 rust/kernel/prelude.rs           |  2 ++
 rust/kernel/uaccess.rs           | 68 +++++++++++++++++++++++++++++++++-------
 samples/rust/rust_misc_device.rs |  2 ++
 3 files changed, 60 insertions(+), 12 deletions(-)

diff --git a/rust/kernel/prelude.rs b/rust/kernel/prelude.rs
index baa774a351ceeb995a2a647f78a27b408d9f3834..081af5bc07b0bcefb1da16e5a81fc611b3178aea 100644
--- a/rust/kernel/prelude.rs
+++ b/rust/kernel/prelude.rs
@@ -41,3 +41,5 @@
 pub use super::init::InPlaceInit;
 
 pub use super::current;
+
+pub use super::uaccess::UserPtr;
diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs
index e6534b52a1920254d61f8349426d4cdb38286089..02e0561eb1c6f4d813a4ab13a124bfac2d2a5c75 100644
--- a/rust/kernel/uaccess.rs
+++ b/rust/kernel/uaccess.rs
@@ -14,8 +14,48 @@
 };
 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(*mut c_void);
+
+impl UserPtr {
+    /// Create a `UserPtr` from an integer representing the userspace address.
+    pub fn from_addr(addr: usize) -> Self {
+        Self(addr as *mut c_void)
+    }
+
+    /// Create a `UserPtr` from a pointer representing the userspace address.
+    pub fn from_ptr(addr: *mut c_void) -> Self {
+        Self(addr)
+    }
+
+    /// 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
+    }
+
+    /// 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
+    }
+
+    /// Increment this user pointer by `add` bytes.
+    ///
+    /// This 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_byte_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.
 ///
@@ -179,7 +219,7 @@ impl UserSliceReader {
     pub fn skip(&mut self, num_skip: usize) -> Result {
         // Update `self.length` first since that's the fallible part of this operation.
         self.length = self.length.checked_sub(num_skip).ok_or(EFAULT)?;
-        self.ptr = self.ptr.wrapping_add(num_skip);
+        self.ptr = self.ptr.wrapping_byte_add(num_skip);
         Ok(())
     }
 
@@ -226,11 +266,11 @@ 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);
         }
-        self.ptr = self.ptr.wrapping_add(len);
+        self.ptr = self.ptr.wrapping_byte_add(len);
         self.length -= len;
         Ok(())
     }
@@ -264,14 +304,14 @@ 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,
             )
         };
         if res != 0 {
             return Err(EFAULT);
         }
-        self.ptr = self.ptr.wrapping_add(len);
+        self.ptr = self.ptr.wrapping_byte_add(len);
         self.length -= len;
         // SAFETY: The read above has initialized all bytes in `out`, and since `T` implements
         // `FromBytes`, any bit-pattern is a valid value for this type.
@@ -384,11 +424,11 @@ 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);
         }
-        self.ptr = self.ptr.wrapping_add(len);
+        self.ptr = self.ptr.wrapping_byte_add(len);
         self.length -= len;
         Ok(())
     }
@@ -411,7 +451,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,
             )
@@ -419,7 +459,7 @@ pub fn write<T: AsBytes>(&mut self, value: &T) -> Result {
         if res != 0 {
             return Err(EFAULT);
         }
-        self.ptr = self.ptr.wrapping_add(len);
+        self.ptr = self.ptr.wrapping_byte_add(len);
         self.length -= len;
         Ok(())
     }
@@ -444,7 +484,11 @@ fn raw_strncpy_from_user(dst: &mut [MaybeUninit<u8>], src: UserPtr) -> Result<us
 
     // SAFETY: `dst` is valid for writing `dst.len()` bytes.
     let res = unsafe {
-        bindings::strncpy_from_user(dst.as_mut_ptr().cast::<c_char>(), src as *const c_char, len)
+        bindings::strncpy_from_user(
+            dst.as_mut_ptr().cast::<c_char>(),
+            src.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..e7ab77448f754906615b6f89d72b51fa268f6c41 100644
--- a/samples/rust/rust_misc_device.rs
+++ b/samples/rust/rust_misc_device.rs
@@ -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::from_addr(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:v4
prerequisite-patch-id: d35c479ddb84bacc541dbb226c7911e5a22cad7e
prerequisite-patch-id: e0c345945dabfa18e9ca11f7fe11f8178d071285

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


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH v2] uaccess: rust: use newtype for user pointers
  2025-05-27 13:53 [PATCH v2] uaccess: rust: use newtype for user pointers Alice Ryhl
@ 2025-05-27 14:42 ` Greg Kroah-Hartman
  2025-05-27 15:20 ` Boqun Feng
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 17+ messages in thread
From: Greg Kroah-Hartman @ 2025-05-27 14:42 UTC (permalink / raw)
  To: Alice Ryhl
  Cc: Miguel Ojeda, Alexander Viro, Arnd Bergmann, Andrew Morton,
	Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
	Andreas Hindborg, Trevor Gross, Danilo Krummrich, rust-for-linux,
	linux-kernel

On Tue, May 27, 2025 at 01:53:12PM +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
> leakage.
> 
> The type is added to the prelude as it is a fairly fundamental type
> similar to c_int. The wrapping_add() method is renamed to
> wrapping_byte_add() for consistency with the method name found on raw
> pointers.
> 
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>

Nice!

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2] uaccess: rust: use newtype for user pointers
  2025-05-27 13:53 [PATCH v2] uaccess: rust: use newtype for user pointers Alice Ryhl
  2025-05-27 14:42 ` Greg Kroah-Hartman
@ 2025-05-27 15:20 ` Boqun Feng
  2025-05-27 15:22   ` Alice Ryhl
  2025-05-27 19:03 ` Christian Schrefl
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 17+ messages in thread
From: Boqun Feng @ 2025-05-27 15:20 UTC (permalink / raw)
  To: Alice Ryhl
  Cc: Miguel Ojeda, Alexander Viro, Greg Kroah-Hartman, Arnd Bergmann,
	Andrew Morton, Gary Guo, Björn Roy Baron, Benno Lossin,
	Andreas Hindborg, Trevor Gross, Danilo Krummrich, rust-for-linux,
	linux-kernel

On Tue, May 27, 2025 at 01:53:12PM +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
> leakage.
> 
> The type is added to the prelude as it is a fairly fundamental type
> similar to c_int. The wrapping_add() method is renamed to
> wrapping_byte_add() for consistency with the method name found on raw
> pointers.
> 
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>

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

A question below:

> ---
> This is based on top of the strncpy_from_user for Rust patch.
> ---
> Changes in v2:
> - Change usize to raw pointer.
> - Make field private.
> - Rename wrapping_add to wrapping_byte_add.
> - Add to prelude.
> - Rebase on v4 of strncpy_from_user
> - Link to v1: https://lore.kernel.org/r/20250506-userptr-newtype-v1-1-a0f6f8ce9fc5@google.com
> ---
>  rust/kernel/prelude.rs           |  2 ++
>  rust/kernel/uaccess.rs           | 68 +++++++++++++++++++++++++++++++++-------
>  samples/rust/rust_misc_device.rs |  2 ++
>  3 files changed, 60 insertions(+), 12 deletions(-)
> 
> diff --git a/rust/kernel/prelude.rs b/rust/kernel/prelude.rs
> index baa774a351ceeb995a2a647f78a27b408d9f3834..081af5bc07b0bcefb1da16e5a81fc611b3178aea 100644
> --- a/rust/kernel/prelude.rs
> +++ b/rust/kernel/prelude.rs
> @@ -41,3 +41,5 @@
>  pub use super::init::InPlaceInit;
>  
>  pub use super::current;
> +
> +pub use super::uaccess::UserPtr;
> diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs
> index e6534b52a1920254d61f8349426d4cdb38286089..02e0561eb1c6f4d813a4ab13a124bfac2d2a5c75 100644
> --- a/rust/kernel/uaccess.rs
> +++ b/rust/kernel/uaccess.rs
> @@ -14,8 +14,48 @@
>  };
>  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(*mut c_void);
> +
> +impl UserPtr {
> +    /// Create a `UserPtr` from an integer representing the userspace address.
> +    pub fn from_addr(addr: usize) -> Self {
> +        Self(addr as *mut c_void)
> +    }
> +
> +    /// Create a `UserPtr` from a pointer representing the userspace address.
> +    pub fn from_ptr(addr: *mut c_void) -> Self {
> +        Self(addr)
> +    }
> +
> +    /// 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
> +    }
> +
> +    /// 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
> +    }
> +

why are these two inline but the rest not?

Regards,
Boqun

> +    /// Increment this user pointer by `add` bytes.
> +    ///
> +    /// This 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_byte_add(self, add: usize) -> UserPtr {
> +        UserPtr(self.0.wrapping_add(add))
> +    }
> +}
>  
[...]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2] uaccess: rust: use newtype for user pointers
  2025-05-27 15:20 ` Boqun Feng
@ 2025-05-27 15:22   ` Alice Ryhl
  0 siblings, 0 replies; 17+ messages in thread
From: Alice Ryhl @ 2025-05-27 15:22 UTC (permalink / raw)
  To: Boqun Feng
  Cc: Miguel Ojeda, Alexander Viro, Greg Kroah-Hartman, Arnd Bergmann,
	Andrew Morton, Gary Guo, Björn Roy Baron, Benno Lossin,
	Andreas Hindborg, Trevor Gross, Danilo Krummrich, rust-for-linux,
	linux-kernel

On Tue, May 27, 2025 at 5:20 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> On Tue, May 27, 2025 at 01:53:12PM +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
> > leakage.
> >
> > The type is added to the prelude as it is a fairly fundamental type
> > similar to c_int. The wrapping_add() method is renamed to
> > wrapping_byte_add() for consistency with the method name found on raw
> > pointers.
> >
> > Signed-off-by: Alice Ryhl <aliceryhl@google.com>
>
> Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
>
> A question below:
>
> > ---
> > This is based on top of the strncpy_from_user for Rust patch.
> > ---
> > Changes in v2:
> > - Change usize to raw pointer.
> > - Make field private.
> > - Rename wrapping_add to wrapping_byte_add.
> > - Add to prelude.
> > - Rebase on v4 of strncpy_from_user
> > - Link to v1: https://lore.kernel.org/r/20250506-userptr-newtype-v1-1-a0f6f8ce9fc5@google.com
> > ---
> >  rust/kernel/prelude.rs           |  2 ++
> >  rust/kernel/uaccess.rs           | 68 +++++++++++++++++++++++++++++++++-------
> >  samples/rust/rust_misc_device.rs |  2 ++
> >  3 files changed, 60 insertions(+), 12 deletions(-)
> >
> > diff --git a/rust/kernel/prelude.rs b/rust/kernel/prelude.rs
> > index baa774a351ceeb995a2a647f78a27b408d9f3834..081af5bc07b0bcefb1da16e5a81fc611b3178aea 100644
> > --- a/rust/kernel/prelude.rs
> > +++ b/rust/kernel/prelude.rs
> > @@ -41,3 +41,5 @@
> >  pub use super::init::InPlaceInit;
> >
> >  pub use super::current;
> > +
> > +pub use super::uaccess::UserPtr;
> > diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs
> > index e6534b52a1920254d61f8349426d4cdb38286089..02e0561eb1c6f4d813a4ab13a124bfac2d2a5c75 100644
> > --- a/rust/kernel/uaccess.rs
> > +++ b/rust/kernel/uaccess.rs
> > @@ -14,8 +14,48 @@
> >  };
> >  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(*mut c_void);
> > +
> > +impl UserPtr {
> > +    /// Create a `UserPtr` from an integer representing the userspace address.
> > +    pub fn from_addr(addr: usize) -> Self {
> > +        Self(addr as *mut c_void)
> > +    }
> > +
> > +    /// Create a `UserPtr` from a pointer representing the userspace address.
> > +    pub fn from_ptr(addr: *mut c_void) -> Self {
> > +        Self(addr)
> > +    }
> > +
> > +    /// 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
> > +    }
> > +
> > +    /// 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
> > +    }
> > +
>
> why are these two inline but the rest not?

Oh, I just forgot to add it.

Alice

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2] uaccess: rust: use newtype for user pointers
  2025-05-27 13:53 [PATCH v2] uaccess: rust: use newtype for user pointers Alice Ryhl
  2025-05-27 14:42 ` Greg Kroah-Hartman
  2025-05-27 15:20 ` Boqun Feng
@ 2025-05-27 19:03 ` Christian Schrefl
  2025-05-27 19:09   ` Alice Ryhl
  2025-05-27 22:12 ` Al Viro
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 17+ messages in thread
From: Christian Schrefl @ 2025-05-27 19:03 UTC (permalink / raw)
  To: Alice Ryhl, Miguel Ojeda, Alexander Viro, Greg Kroah-Hartman,
	Arnd Bergmann
  Cc: Andrew Morton, Boqun Feng, Gary Guo, Björn Roy Baron,
	Benno Lossin, Andreas Hindborg, Trevor Gross, Danilo Krummrich,
	rust-for-linux, linux-kernel

Hi Alice,

On 27.05.25 3:53 PM, 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
> leakage.
> 
> The type is added to the prelude as it is a fairly fundamental type
> similar to c_int. The wrapping_add() method is renamed to
> wrapping_byte_add() for consistency with the method name found on raw
> pointers.
> 
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
> This is based on top of the strncpy_from_user for Rust patch.
> ---
> Changes in v2:
> - Change usize to raw pointer.
> - Make field private.
> - Rename wrapping_add to wrapping_byte_add.
> - Add to prelude.
> - Rebase on v4 of strncpy_from_user
> - Link to v1: https://lore.kernel.org/r/20250506-userptr-newtype-v1-1-a0f6f8ce9fc5@google.com
> ---
>  rust/kernel/prelude.rs           |  2 ++
>  rust/kernel/uaccess.rs           | 68 +++++++++++++++++++++++++++++++++-------
>  samples/rust/rust_misc_device.rs |  2 ++
>  3 files changed, 60 insertions(+), 12 deletions(-)
> 
> diff --git a/rust/kernel/prelude.rs b/rust/kernel/prelude.rs
> index baa774a351ceeb995a2a647f78a27b408d9f3834..081af5bc07b0bcefb1da16e5a81fc611b3178aea 100644
> --- a/rust/kernel/prelude.rs
> +++ b/rust/kernel/prelude.rs
> @@ -41,3 +41,5 @@
>  pub use super::init::InPlaceInit;
>  
>  pub use super::current;
> +
> +pub use super::uaccess::UserPtr;
> diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs
> index e6534b52a1920254d61f8349426d4cdb38286089..02e0561eb1c6f4d813a4ab13a124bfac2d2a5c75 100644
> --- a/rust/kernel/uaccess.rs
> +++ b/rust/kernel/uaccess.rs
> @@ -14,8 +14,48 @@
>  };
>  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(*mut c_void);
> +
> +impl UserPtr {
> +    /// Create a `UserPtr` from an integer representing the userspace address.
> +    pub fn from_addr(addr: usize) -> Self {
> +        Self(addr as *mut c_void)
> +    }
> +
> +    /// Create a `UserPtr` from a pointer representing the userspace address.
> +    pub fn from_ptr(addr: *mut c_void) -> Self {
> +        Self(addr)
> +    }
> +
> +    /// 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
> +    }
> +
> +    /// 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
> +    }
> +
> +    /// Increment this user pointer by `add` bytes.
> +    ///
> +    /// This 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_byte_add(self, add: usize) -> UserPtr {
> +        UserPtr(self.0.wrapping_add(add))

Why use `ptr::wrapping_add` instead of `ptr::wrapping_byte_add`?

Should not really matter in this case, because `c_void` is 
`repr(u8)`, but I find it a bit weird to use `wrapping_add` here.

With that fixed:

Reviewed-by: Christian Schrefl <chrisi.schrefl@gmail.com>

> +    }
> +}
>  
>  /// A pointer to an area in userspace memory, which can be either read-only or read-write.
>  ///
> @@ -179,7 +219,7 @@ impl UserSliceReader {
>      pub fn skip(&mut self, num_skip: usize) -> Result {
>          // Update `self.length` first since that's the fallible part of this operation.
>          self.length = self.length.checked_sub(num_skip).ok_or(EFAULT)?;
> -        self.ptr = self.ptr.wrapping_add(num_skip);
> +        self.ptr = self.ptr.wrapping_byte_add(num_skip);
>          Ok(())
>      }
>  
> @@ -226,11 +266,11 @@ 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);
>          }
> -        self.ptr = self.ptr.wrapping_add(len);
> +        self.ptr = self.ptr.wrapping_byte_add(len);
>          self.length -= len;
>          Ok(())
>      }
> @@ -264,14 +304,14 @@ 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,
>              )
>          };
>          if res != 0 {
>              return Err(EFAULT);
>          }
> -        self.ptr = self.ptr.wrapping_add(len);
> +        self.ptr = self.ptr.wrapping_byte_add(len);
>          self.length -= len;
>          // SAFETY: The read above has initialized all bytes in `out`, and since `T` implements
>          // `FromBytes`, any bit-pattern is a valid value for this type.
> @@ -384,11 +424,11 @@ 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);
>          }
> -        self.ptr = self.ptr.wrapping_add(len);
> +        self.ptr = self.ptr.wrapping_byte_add(len);
>          self.length -= len;
>          Ok(())
>      }
> @@ -411,7 +451,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,
>              )
> @@ -419,7 +459,7 @@ pub fn write<T: AsBytes>(&mut self, value: &T) -> Result {
>          if res != 0 {
>              return Err(EFAULT);
>          }
> -        self.ptr = self.ptr.wrapping_add(len);
> +        self.ptr = self.ptr.wrapping_byte_add(len);
>          self.length -= len;
>          Ok(())
>      }
> @@ -444,7 +484,11 @@ fn raw_strncpy_from_user(dst: &mut [MaybeUninit<u8>], src: UserPtr) -> Result<us
>  
>      // SAFETY: `dst` is valid for writing `dst.len()` bytes.
>      let res = unsafe {
> -        bindings::strncpy_from_user(dst.as_mut_ptr().cast::<c_char>(), src as *const c_char, len)
> +        bindings::strncpy_from_user(
> +            dst.as_mut_ptr().cast::<c_char>(),
> +            src.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..e7ab77448f754906615b6f89d72b51fa268f6c41 100644
> --- a/samples/rust/rust_misc_device.rs
> +++ b/samples/rust/rust_misc_device.rs
> @@ -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::from_addr(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:v4
> prerequisite-patch-id: d35c479ddb84bacc541dbb226c7911e5a22cad7e
> prerequisite-patch-id: e0c345945dabfa18e9ca11f7fe11f8178d071285
> 
> Best regards,


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2] uaccess: rust: use newtype for user pointers
  2025-05-27 19:03 ` Christian Schrefl
@ 2025-05-27 19:09   ` Alice Ryhl
  0 siblings, 0 replies; 17+ messages in thread
From: Alice Ryhl @ 2025-05-27 19:09 UTC (permalink / raw)
  To: Christian Schrefl
  Cc: Miguel Ojeda, Alexander Viro, Greg Kroah-Hartman, Arnd Bergmann,
	Andrew Morton, Boqun Feng, Gary Guo, Björn Roy Baron,
	Benno Lossin, Andreas Hindborg, Trevor Gross, Danilo Krummrich,
	rust-for-linux, linux-kernel

On Tue, May 27, 2025 at 9:03 PM Christian Schrefl
<chrisi.schrefl@gmail.com> wrote:
>
> Hi Alice,
>
> On 27.05.25 3:53 PM, 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
> > leakage.
> >
> > The type is added to the prelude as it is a fairly fundamental type
> > similar to c_int. The wrapping_add() method is renamed to
> > wrapping_byte_add() for consistency with the method name found on raw
> > pointers.
> >
> > Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> > ---
> > This is based on top of the strncpy_from_user for Rust patch.
> > ---
> > Changes in v2:
> > - Change usize to raw pointer.
> > - Make field private.
> > - Rename wrapping_add to wrapping_byte_add.
> > - Add to prelude.
> > - Rebase on v4 of strncpy_from_user
> > - Link to v1: https://lore.kernel.org/r/20250506-userptr-newtype-v1-1-a0f6f8ce9fc5@google.com
> > ---
> >  rust/kernel/prelude.rs           |  2 ++
> >  rust/kernel/uaccess.rs           | 68 +++++++++++++++++++++++++++++++++-------
> >  samples/rust/rust_misc_device.rs |  2 ++
> >  3 files changed, 60 insertions(+), 12 deletions(-)
> >
> > diff --git a/rust/kernel/prelude.rs b/rust/kernel/prelude.rs
> > index baa774a351ceeb995a2a647f78a27b408d9f3834..081af5bc07b0bcefb1da16e5a81fc611b3178aea 100644
> > --- a/rust/kernel/prelude.rs
> > +++ b/rust/kernel/prelude.rs
> > @@ -41,3 +41,5 @@
> >  pub use super::init::InPlaceInit;
> >
> >  pub use super::current;
> > +
> > +pub use super::uaccess::UserPtr;
> > diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs
> > index e6534b52a1920254d61f8349426d4cdb38286089..02e0561eb1c6f4d813a4ab13a124bfac2d2a5c75 100644
> > --- a/rust/kernel/uaccess.rs
> > +++ b/rust/kernel/uaccess.rs
> > @@ -14,8 +14,48 @@
> >  };
> >  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(*mut c_void);
> > +
> > +impl UserPtr {
> > +    /// Create a `UserPtr` from an integer representing the userspace address.
> > +    pub fn from_addr(addr: usize) -> Self {
> > +        Self(addr as *mut c_void)
> > +    }
> > +
> > +    /// Create a `UserPtr` from a pointer representing the userspace address.
> > +    pub fn from_ptr(addr: *mut c_void) -> Self {
> > +        Self(addr)
> > +    }
> > +
> > +    /// 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
> > +    }
> > +
> > +    /// 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
> > +    }
> > +
> > +    /// Increment this user pointer by `add` bytes.
> > +    ///
> > +    /// This 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_byte_add(self, add: usize) -> UserPtr {
> > +        UserPtr(self.0.wrapping_add(add))
>
> Why use `ptr::wrapping_add` instead of `ptr::wrapping_byte_add`?
>
> Should not really matter in this case, because `c_void` is
> `repr(u8)`, but I find it a bit weird to use `wrapping_add` here.
>
> With that fixed:
>
> Reviewed-by: Christian Schrefl <chrisi.schrefl@gmail.com>

Ah yeah, this should probably just be wrapping_byte_add() too.
Alice

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2] uaccess: rust: use newtype for user pointers
  2025-05-27 13:53 [PATCH v2] uaccess: rust: use newtype for user pointers Alice Ryhl
                   ` (2 preceding siblings ...)
  2025-05-27 19:03 ` Christian Schrefl
@ 2025-05-27 22:12 ` Al Viro
  2025-05-27 23:13   ` Boqun Feng
  2025-05-28 10:47   ` Alice Ryhl
  2025-05-28 10:52 ` Danilo Krummrich
  2025-05-28 16:55 ` Benno Lossin
  5 siblings, 2 replies; 17+ messages in thread
From: Al Viro @ 2025-05-27 22:12 UTC (permalink / raw)
  To: Alice Ryhl
  Cc: Miguel Ojeda, Greg Kroah-Hartman, Arnd Bergmann, Andrew Morton,
	Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
	Andreas Hindborg, Trevor Gross, Danilo Krummrich, rust-for-linux,
	linux-kernel

On Tue, May 27, 2025 at 01:53:12PM +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
> leakage.
> 
> The type is added to the prelude as it is a fairly fundamental type
> similar to c_int. The wrapping_add() method is renamed to
> wrapping_byte_add() for consistency with the method name found on raw
> pointers.

That's considerably weaker than __user, though - with
	struct foo {struct bar x; struct baz y[2]; };
	struct foo __user *p;
	void f(struct bar __user *);
sparse does figure out that f(&p->y[1]) is a type error - &p->y[1] is
struct baz __user * and f() expects struct bar __user *.

It's not just mixing userland pointers with other things - it's not mixing
userland pointers to different types, etc.

In practice I've seen quite a few brainos caught by that...

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2] uaccess: rust: use newtype for user pointers
  2025-05-27 22:12 ` Al Viro
@ 2025-05-27 23:13   ` Boqun Feng
  2025-05-28 10:48     ` Alice Ryhl
  2025-05-28 15:38     ` Benno Lossin
  2025-05-28 10:47   ` Alice Ryhl
  1 sibling, 2 replies; 17+ messages in thread
From: Boqun Feng @ 2025-05-27 23:13 UTC (permalink / raw)
  To: Al Viro
  Cc: Alice Ryhl, Miguel Ojeda, Greg Kroah-Hartman, Arnd Bergmann,
	Andrew Morton, Gary Guo, Björn Roy Baron, Benno Lossin,
	Andreas Hindborg, Trevor Gross, Danilo Krummrich, rust-for-linux,
	linux-kernel

On Tue, May 27, 2025 at 11:12:11PM +0100, Al Viro wrote:
> On Tue, May 27, 2025 at 01:53:12PM +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
> > leakage.
> > 
> > The type is added to the prelude as it is a fairly fundamental type
> > similar to c_int. The wrapping_add() method is renamed to
> > wrapping_byte_add() for consistency with the method name found on raw
> > pointers.
> 
> That's considerably weaker than __user, though - with
> 	struct foo {struct bar x; struct baz y[2]; };

Translate to Rust this is:

    struct Foo {
        x: Bar,
	y: Baz[2],
    }

> 	struct foo __user *p;

UserPtr should probably be generic over pointee, so:

    pub struct UserPtr<T>(*mut c_void, PhantomData<*mut T>);

and

    let p: UserPtr<Foo> = ...;

> 	void f(struct bar __user *);

and this is:

    pub fn f(bar: UserPtr<Bar>)

and the checking should work, a (maybe unrelated) tricky part though..

> sparse does figure out that f(&p->y[1]) is a type error - &p->y[1] is

In Rust, you will need to play a little unsafe game to get &p->y[1]:

    let foo_ptr: *mut Foo = p.as_mut_ptr();
    let y_ptr: *mut Baz = unsafe { addr_of_mut!((*foo_ptr).y[1]) };
    let y: UserPtr<Baz> = unsafe { UserPtr::from_ptr(y_ptr) };

passing y to f() will get a type mismatch, so the detection/checking
works. To avoid the unsafe game we need field projection [1].

> struct baz __user * and f() expects struct bar __user *.
> 
> It's not just mixing userland pointers with other things - it's not mixing
> userland pointers to different types, etc.
> 

In short, with UserPtr generic over pointee, we can have the similar
detection as sparse.

[1]: https://github.com/rust-lang/rfcs/pull/3735

Regards,
Boqun

> In practice I've seen quite a few brainos caught by that...

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2] uaccess: rust: use newtype for user pointers
  2025-05-27 22:12 ` Al Viro
  2025-05-27 23:13   ` Boqun Feng
@ 2025-05-28 10:47   ` Alice Ryhl
  2025-05-28 17:45     ` Al Viro
  1 sibling, 1 reply; 17+ messages in thread
From: Alice Ryhl @ 2025-05-28 10:47 UTC (permalink / raw)
  To: Al Viro
  Cc: Miguel Ojeda, Greg Kroah-Hartman, Arnd Bergmann, Andrew Morton,
	Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
	Andreas Hindborg, Trevor Gross, Danilo Krummrich, rust-for-linux,
	linux-kernel

On Tue, May 27, 2025 at 11:12:11PM +0100, Al Viro wrote:
> On Tue, May 27, 2025 at 01:53:12PM +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
> > leakage.
> > 
> > The type is added to the prelude as it is a fairly fundamental type
> > similar to c_int. The wrapping_add() method is renamed to
> > wrapping_byte_add() for consistency with the method name found on raw
> > pointers.
> 
> That's considerably weaker than __user, though - with
> 	struct foo {struct bar x; struct baz y[2]; };
> 	struct foo __user *p;
> 	void f(struct bar __user *);
> sparse does figure out that f(&p->y[1]) is a type error - &p->y[1] is
> struct baz __user * and f() expects struct bar __user *.
> 
> It's not just mixing userland pointers with other things - it's not mixing
> userland pointers to different types, etc.
> 
> In practice I've seen quite a few brainos caught by that...

We don't currently have any way to perform that kind of pointer-math on
user pointers, nor do we have any users of it. I imagine that this type
checking is only useful if you can actually perform pointer math in the
first place?

Right now, actual reading/writing to userspace is done via the
UserSliceReader and UserSliceWriter types rather than directly on
UserPtr, and UserPtr is just the constructor for those other types.
There is already some means of type checking on UserSliceReader and
UserSliceWriter, so I'm inclined to say that this is good enough.

Of course, if we ever add a way to do pointer math on user pointers,
then they should have a pointee type and hence also this check.

Alice

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2] uaccess: rust: use newtype for user pointers
  2025-05-27 23:13   ` Boqun Feng
@ 2025-05-28 10:48     ` Alice Ryhl
  2025-05-28 15:38     ` Benno Lossin
  1 sibling, 0 replies; 17+ messages in thread
From: Alice Ryhl @ 2025-05-28 10:48 UTC (permalink / raw)
  To: Boqun Feng
  Cc: Al Viro, Miguel Ojeda, Greg Kroah-Hartman, Arnd Bergmann,
	Andrew Morton, Gary Guo, Björn Roy Baron, Benno Lossin,
	Andreas Hindborg, Trevor Gross, Danilo Krummrich, rust-for-linux,
	linux-kernel

On Tue, May 27, 2025 at 04:13:03PM -0700, Boqun Feng wrote:
> On Tue, May 27, 2025 at 11:12:11PM +0100, Al Viro wrote:
> > On Tue, May 27, 2025 at 01:53:12PM +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
> > > leakage.
> > > 
> > > The type is added to the prelude as it is a fairly fundamental type
> > > similar to c_int. The wrapping_add() method is renamed to
> > > wrapping_byte_add() for consistency with the method name found on raw
> > > pointers.
> > 
> > That's considerably weaker than __user, though - with
> > 	struct foo {struct bar x; struct baz y[2]; };
> 
> Translate to Rust this is:
> 
>     struct Foo {
>         x: Bar,
> 	y: Baz[2],
>     }
> 
> > 	struct foo __user *p;
> 
> UserPtr should probably be generic over pointee, so:
> 
>     pub struct UserPtr<T>(*mut c_void, PhantomData<*mut T>);
> 
> and
> 
>     let p: UserPtr<Foo> = ...;
> 
> > 	void f(struct bar __user *);
> 
> and this is:
> 
>     pub fn f(bar: UserPtr<Bar>)
> 
> and the checking should work, a (maybe unrelated) tricky part though..
> 
> > sparse does figure out that f(&p->y[1]) is a type error - &p->y[1] is
> 
> In Rust, you will need to play a little unsafe game to get &p->y[1]:
> 
>     let foo_ptr: *mut Foo = p.as_mut_ptr();
>     let y_ptr: *mut Baz = unsafe { addr_of_mut!((*foo_ptr).y[1]) };
>     let y: UserPtr<Baz> = unsafe { UserPtr::from_ptr(y_ptr) };
> 
> passing y to f() will get a type mismatch, so the detection/checking
> works. To avoid the unsafe game we need field projection [1].

That looks pretty unergonomic. We can do better than that.

Alice

> > struct baz __user * and f() expects struct bar __user *.
> > 
> > It's not just mixing userland pointers with other things - it's not mixing
> > userland pointers to different types, etc.
> > 
> 
> In short, with UserPtr generic over pointee, we can have the similar
> detection as sparse.
> 
> [1]: https://github.com/rust-lang/rfcs/pull/3735
> 
> Regards,
> Boqun
> 
> > In practice I've seen quite a few brainos caught by that...

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2] uaccess: rust: use newtype for user pointers
  2025-05-27 13:53 [PATCH v2] uaccess: rust: use newtype for user pointers Alice Ryhl
                   ` (3 preceding siblings ...)
  2025-05-27 22:12 ` Al Viro
@ 2025-05-28 10:52 ` Danilo Krummrich
  2025-05-28 16:55 ` Benno Lossin
  5 siblings, 0 replies; 17+ messages in thread
From: Danilo Krummrich @ 2025-05-28 10:52 UTC (permalink / raw)
  To: Alice Ryhl
  Cc: Miguel Ojeda, Alexander Viro, Greg Kroah-Hartman, Arnd Bergmann,
	Andrew Morton, Boqun Feng, Gary Guo, Björn Roy Baron,
	Benno Lossin, Andreas Hindborg, Trevor Gross, rust-for-linux,
	linux-kernel

On 5/27/25 3:53 PM, 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
> leakage.
> 
> The type is added to the prelude as it is a fairly fundamental type
> similar to c_int. The wrapping_add() method is renamed to
> wrapping_byte_add() for consistency with the method name found on raw
> pointers.
> 
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2] uaccess: rust: use newtype for user pointers
  2025-05-27 23:13   ` Boqun Feng
  2025-05-28 10:48     ` Alice Ryhl
@ 2025-05-28 15:38     ` Benno Lossin
  2025-05-28 16:02       ` Boqun Feng
  1 sibling, 1 reply; 17+ messages in thread
From: Benno Lossin @ 2025-05-28 15:38 UTC (permalink / raw)
  To: Boqun Feng, Al Viro
  Cc: Alice Ryhl, Miguel Ojeda, Greg Kroah-Hartman, Arnd Bergmann,
	Andrew Morton, Gary Guo, Björn Roy Baron, Benno Lossin,
	Andreas Hindborg, Trevor Gross, Danilo Krummrich, rust-for-linux,
	linux-kernel

On Wed May 28, 2025 at 1:13 AM CEST, Boqun Feng wrote:
> On Tue, May 27, 2025 at 11:12:11PM +0100, Al Viro wrote:
>> On Tue, May 27, 2025 at 01:53:12PM +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
>> > leakage.
>> > 
>> > The type is added to the prelude as it is a fairly fundamental type
>> > similar to c_int. The wrapping_add() method is renamed to
>> > wrapping_byte_add() for consistency with the method name found on raw
>> > pointers.
>> 
>> That's considerably weaker than __user, though - with
>> 	struct foo {struct bar x; struct baz y[2]; };
>
> Translate to Rust this is:
>
>     struct Foo {
>         x: Bar,
> 	y: Baz[2],
>     }
>
>> 	struct foo __user *p;
>
> UserPtr should probably be generic over pointee, so:
>
>     pub struct UserPtr<T>(*mut c_void, PhantomData<*mut T>);
>
> and
>
>     let p: UserPtr<Foo> = ...;
>
>> 	void f(struct bar __user *);
>
> and this is:
>
>     pub fn f(bar: UserPtr<Bar>)
>
> and the checking should work, a (maybe unrelated) tricky part though..
>
>> sparse does figure out that f(&p->y[1]) is a type error - &p->y[1] is
>
> In Rust, you will need to play a little unsafe game to get &p->y[1]:
>
>     let foo_ptr: *mut Foo = p.as_mut_ptr();
>     let y_ptr: *mut Baz = unsafe { addr_of_mut!((*foo_ptr).y[1]) };
>     let y: UserPtr<Baz> = unsafe { UserPtr::from_ptr(y_ptr) };

Shouldn't this use `wrapping_add` since the pointer shouldn't be
dereferenced?

If we don't use `wrapping_add`, then the field projection operation for
this type must be `unsafe`.

---
Cheers,
Benno

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2] uaccess: rust: use newtype for user pointers
  2025-05-28 15:38     ` Benno Lossin
@ 2025-05-28 16:02       ` Boqun Feng
  0 siblings, 0 replies; 17+ messages in thread
From: Boqun Feng @ 2025-05-28 16:02 UTC (permalink / raw)
  To: Benno Lossin
  Cc: Al Viro, Alice Ryhl, Miguel Ojeda, Greg Kroah-Hartman,
	Arnd Bergmann, Andrew Morton, Gary Guo, Björn Roy Baron,
	Benno Lossin, Andreas Hindborg, Trevor Gross, Danilo Krummrich,
	rust-for-linux, linux-kernel

On Wed, May 28, 2025 at 05:38:08PM +0200, Benno Lossin wrote:
> On Wed May 28, 2025 at 1:13 AM CEST, Boqun Feng wrote:
> > On Tue, May 27, 2025 at 11:12:11PM +0100, Al Viro wrote:
> >> On Tue, May 27, 2025 at 01:53:12PM +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
> >> > leakage.
> >> > 
> >> > The type is added to the prelude as it is a fairly fundamental type
> >> > similar to c_int. The wrapping_add() method is renamed to
> >> > wrapping_byte_add() for consistency with the method name found on raw
> >> > pointers.
> >> 
> >> That's considerably weaker than __user, though - with
> >> 	struct foo {struct bar x; struct baz y[2]; };
> >
> > Translate to Rust this is:
> >
> >     struct Foo {
> >         x: Bar,
> > 	y: Baz[2],
> >     }
> >
> >> 	struct foo __user *p;
> >
> > UserPtr should probably be generic over pointee, so:
> >
> >     pub struct UserPtr<T>(*mut c_void, PhantomData<*mut T>);
> >
> > and
> >
> >     let p: UserPtr<Foo> = ...;
> >
> >> 	void f(struct bar __user *);
> >
> > and this is:
> >
> >     pub fn f(bar: UserPtr<Bar>)
> >
> > and the checking should work, a (maybe unrelated) tricky part though..
> >
> >> sparse does figure out that f(&p->y[1]) is a type error - &p->y[1] is
> >
> > In Rust, you will need to play a little unsafe game to get &p->y[1]:
> >
> >     let foo_ptr: *mut Foo = p.as_mut_ptr();
> >     let y_ptr: *mut Baz = unsafe { addr_of_mut!((*foo_ptr).y[1]) };
> >     let y: UserPtr<Baz> = unsafe { UserPtr::from_ptr(y_ptr) };
> 
> Shouldn't this use `wrapping_add` since the pointer shouldn't be
> dereferenced?
> 

Good point, so:

     let foo_ptr: *mut Foo = p.as_mut_ptr();
     let y_ptr: *mut Baz = foo_ptr.wrapping_byte_add(offset_of!(Foo, y[1]));
     let y: UserPtr<Baz> = unsafe { UserPtr::from_ptr(y_ptr) };

(using wrapping_byte_add() + offset_of!())

Regards,
Boqun

> If we don't use `wrapping_add`, then the field projection operation for
> this type must be `unsafe`.
> 
> ---
> Cheers,
> Benno

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2] uaccess: rust: use newtype for user pointers
  2025-05-27 13:53 [PATCH v2] uaccess: rust: use newtype for user pointers Alice Ryhl
                   ` (4 preceding siblings ...)
  2025-05-28 10:52 ` Danilo Krummrich
@ 2025-05-28 16:55 ` Benno Lossin
  5 siblings, 0 replies; 17+ messages in thread
From: Benno Lossin @ 2025-05-28 16:55 UTC (permalink / raw)
  To: Alice Ryhl, Miguel Ojeda, Alexander Viro, Greg Kroah-Hartman,
	Arnd Bergmann
  Cc: Andrew Morton, Boqun Feng, Gary Guo, Björn Roy Baron,
	Benno Lossin, Andreas Hindborg, Trevor Gross, Danilo Krummrich,
	rust-for-linux, linux-kernel

On Tue May 27, 2025 at 3:53 PM CEST, 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

Should we still implement it, not printing the address of course? So
something like `UserPtr(/* .. */)` or whatever people print instead of
showing the address?

> it's not possible to print values of this type. This avoids ASLR
> leakage.
>
> The type is added to the prelude as it is a fairly fundamental type
> similar to c_int. The wrapping_add() method is renamed to
> wrapping_byte_add() for consistency with the method name found on raw
> pointers.
>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>

I left one question below, but it already looks good, so:

Reviewed-by: Benno Lossin <lossin@kernel.org>

> @@ -444,7 +484,11 @@ fn raw_strncpy_from_user(dst: &mut [MaybeUninit<u8>], src: UserPtr) -> Result<us
>  
>      // SAFETY: `dst` is valid for writing `dst.len()` bytes.
>      let res = unsafe {
> -        bindings::strncpy_from_user(dst.as_mut_ptr().cast::<c_char>(), src as *const c_char, len)
> +        bindings::strncpy_from_user(
> +            dst.as_mut_ptr().cast::<c_char>(),
> +            src.as_const_ptr().cast::<c_char>(),

Should we also have `as_{const,mut}_char_ptr` functions? After a quick
search, I found that `char __user *` is a common type for user pointers.

---
Cheers,
Benno

> +            len,
> +        )
>      };

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2] uaccess: rust: use newtype for user pointers
  2025-05-28 10:47   ` Alice Ryhl
@ 2025-05-28 17:45     ` Al Viro
  2025-05-28 20:01       ` Al Viro
  2025-05-28 20:35       ` Benno Lossin
  0 siblings, 2 replies; 17+ messages in thread
From: Al Viro @ 2025-05-28 17:45 UTC (permalink / raw)
  To: Alice Ryhl
  Cc: Miguel Ojeda, Greg Kroah-Hartman, Arnd Bergmann, Andrew Morton,
	Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
	Andreas Hindborg, Trevor Gross, Danilo Krummrich, rust-for-linux,
	linux-kernel

On Wed, May 28, 2025 at 10:47:12AM +0000, Alice Ryhl wrote:

> We don't currently have any way to perform that kind of pointer-math on
> user pointers, nor do we have any users of it. I imagine that this type
> checking is only useful if you can actually perform pointer math in the
> first place?

What you want is something like
	x->field::UserPtr(beta) iff
		x::UserPtr(alpha) and
		_.field::beta where _::alpha

Generated code would be "add offset and cast to pointer to type of...",
but doing that manually would really invite headache.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2] uaccess: rust: use newtype for user pointers
  2025-05-28 17:45     ` Al Viro
@ 2025-05-28 20:01       ` Al Viro
  2025-05-28 20:35       ` Benno Lossin
  1 sibling, 0 replies; 17+ messages in thread
From: Al Viro @ 2025-05-28 20:01 UTC (permalink / raw)
  To: Alice Ryhl
  Cc: Miguel Ojeda, Greg Kroah-Hartman, Arnd Bergmann, Andrew Morton,
	Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
	Andreas Hindborg, Trevor Gross, Danilo Krummrich, rust-for-linux,
	linux-kernel

On Wed, May 28, 2025 at 06:45:46PM +0100, Al Viro wrote:
> On Wed, May 28, 2025 at 10:47:12AM +0000, Alice Ryhl wrote:
> 
> > We don't currently have any way to perform that kind of pointer-math on
> > user pointers, nor do we have any users of it. I imagine that this type
> > checking is only useful if you can actually perform pointer math in the
> > first place?
> 
> What you want is something like
> 	x->field::UserPtr(beta) iff
> 		x::UserPtr(alpha) and
> 		_.field::beta where _::alpha
> 
> Generated code would be "add offset and cast to pointer to type of...",
> but doing that manually would really invite headache.

... except that -> comes with wrong connotations, of course.  Hell knows
what a decent syntax would look like.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2] uaccess: rust: use newtype for user pointers
  2025-05-28 17:45     ` Al Viro
  2025-05-28 20:01       ` Al Viro
@ 2025-05-28 20:35       ` Benno Lossin
  1 sibling, 0 replies; 17+ messages in thread
From: Benno Lossin @ 2025-05-28 20:35 UTC (permalink / raw)
  To: Al Viro, Alice Ryhl
  Cc: Miguel Ojeda, Greg Kroah-Hartman, Arnd Bergmann, Andrew Morton,
	Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
	Andreas Hindborg, Trevor Gross, Danilo Krummrich, rust-for-linux,
	linux-kernel, Al Viro

On Wed May 28, 2025 at 7:45 PM CEST, Al Viro wrote:
> On Wed, May 28, 2025 at 10:47:12AM +0000, Alice Ryhl wrote:
>
>> We don't currently have any way to perform that kind of pointer-math on
>> user pointers, nor do we have any users of it. I imagine that this type
>> checking is only useful if you can actually perform pointer math in the
>> first place?
>
> What you want is something like
> 	x->field::UserPtr(beta) iff
> 		x::UserPtr(alpha) and
> 		_.field::beta where _::alpha

Not 100% sure I understand your code correctly, do you mean that:
`x->field` is of type `UserPtr(beta)` given that `x` is of type
`UserPtr(alpha)` and `field` is a field of `alpha` of type `beta`?

If that is correct, then I have a proposal called field projection [1]
for the rust language itself to support this kind of operation for any
custom type.

I also have an macro-based implementation [2] and I could cook up some
examples with `UserPtr` [^3].

[1]: https://github.com/rust-lang/rfcs/pull/3735
[2]: https://github.com/Rust-for-Linux/field-projection/tree/new
[^3]: Here is a quick sketch of how the above would look like in Rust
      using [2]:

      #[derive(HasFields)]
      struct Alpha {
          field: Beta,
      }

      impl Alpha {
          fn write_beta<'a>(mut self: UserPtr<'a, Self>, value: Beta) {
              start_proj!(mut self);
              let mut field: UserPtr<'a, Beta> = p!(@mut self->field);
              field.write(value);
          }
      }

      A couple notes for anyone not too familiar with Rust:

      * `'a` is called a lifetime, it says for how long a value is valid
      * the `< ... >` are generic types
      * the `ident: Type` is a type annotation used in parameters (not
        optional) and in local variable bindings (optional)
      * the `self` parameter is special, it allows one to use the
        `val.fun` syntax (it's similar to `this` in java and `self` in
        python, but without the object orientation stuff associated with
        it)

---
Cheers,
Benno

> Generated code would be "add offset and cast to pointer to type of...",
> but doing that manually would really invite headache.

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2025-05-28 20:35 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-05-27 13:53 [PATCH v2] uaccess: rust: use newtype for user pointers Alice Ryhl
2025-05-27 14:42 ` Greg Kroah-Hartman
2025-05-27 15:20 ` Boqun Feng
2025-05-27 15:22   ` Alice Ryhl
2025-05-27 19:03 ` Christian Schrefl
2025-05-27 19:09   ` Alice Ryhl
2025-05-27 22:12 ` Al Viro
2025-05-27 23:13   ` Boqun Feng
2025-05-28 10:48     ` Alice Ryhl
2025-05-28 15:38     ` Benno Lossin
2025-05-28 16:02       ` Boqun Feng
2025-05-28 10:47   ` Alice Ryhl
2025-05-28 17:45     ` Al Viro
2025-05-28 20:01       ` Al Viro
2025-05-28 20:35       ` Benno Lossin
2025-05-28 10:52 ` Danilo Krummrich
2025-05-28 16:55 ` Benno Lossin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).