linux-kernel - Re: [PATCH v19 1/3] rust: types: add `ForeignOwnable::PointedTo`

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250430193112.4faaff3d.gary@garyguo.net>
Date: Wed, 30 Apr 2025 19:31:12 +0100
From: Gary Guo <gary@...yguo.net>
To: Tamir Duberstein <tamird@...il.com>
Cc: Danilo Krummrich <dakr@...nel.org>, Miguel Ojeda <ojeda@...nel.org>,
 Alex Gaynor <alex.gaynor@...il.com>, Boqun Feng <boqun.feng@...il.com>,
 Björn Roy Baron <bjorn3_gh@...tonmail.com>, Benno Lossin
 <benno.lossin@...ton.me>, Andreas Hindborg <a.hindborg@...nel.org>, Alice
 Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>, Matthew
 Wilcox <willy@...radead.org>, Bjorn Helgaas <bhelgaas@...gle.com>, Greg
 Kroah-Hartman <gregkh@...uxfoundation.org>, "Rafael J. Wysocki"
 <rafael@...nel.org>, FUJITA Tomonori <fujita.tomonori@...il.com>, "Rob
 Herring (Arm)" <robh@...nel.org>, Maíra Canal
 <mcanal@...lia.com>, Asahi Lina <lina@...hilina.net>,
 rust-for-linux@...r.kernel.org, linux-fsdevel@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org
Subject: Re: [PATCH v19 1/3] rust: types: add `ForeignOwnable::PointedTo`

On Wed, 23 Apr 2025 09:54:37 -0400
Tamir Duberstein <tamird@...il.com> wrote:

> Allow implementors to specify the foreign pointer type; this exposes
> information about the pointed-to type such as its alignment.
> 
> This requires the trait to be `unsafe` since it is now possible for
> implementors to break soundness by returning a misaligned pointer.
> 
> Encoding the pointer type in the trait (and avoiding pointer casts)
> allows the compiler to check that implementors return the correct
> pointer type. This is preferable to directly encoding the alignment in
> the trait using a constant as the compiler would be unable to check it.
> 
> Acked-by: Danilo Krummrich <dakr@...nel.org>
> Signed-off-by: Tamir Duberstein <tamird@...il.com>
> ---
>  rust/kernel/alloc/kbox.rs | 38 ++++++++++++++++++++------------------
>  rust/kernel/miscdevice.rs | 10 +++++-----
>  rust/kernel/pci.rs        |  2 +-
>  rust/kernel/platform.rs   |  2 +-
>  rust/kernel/sync/arc.rs   | 21 ++++++++++++---------
>  rust/kernel/types.rs      | 46 +++++++++++++++++++++++++++++++---------------
>  6 files changed, 70 insertions(+), 49 deletions(-)
> 
> diff --git a/rust/kernel/alloc/kbox.rs b/rust/kernel/alloc/kbox.rs
> index b77d32f3a58b..6aa88b01e84d 100644
> --- a/rust/kernel/alloc/kbox.rs
> +++ b/rust/kernel/alloc/kbox.rs
> @@ -360,68 +360,70 @@ fn try_init<E>(init: impl Init<T, E>, flags: Flags) -> Result<Self, E>
>      }
>  }
>  
> -impl<T: 'static, A> ForeignOwnable for Box<T, A>
> +// SAFETY: The `into_foreign` function returns a pointer that is well-aligned.
> +unsafe impl<T: 'static, A> ForeignOwnable for Box<T, A>
>  where
>      A: Allocator,
>  {
> +    type PointedTo = T;

I don't think this is the correct solution for this. The returned
pointer is supposed to opaque, and exposing this type may encourage
this is to be wrongly used.

IIUC, the only reason for this to be exposed is for XArray to be able
to check alignment. However `align_of::<PointedTo>()` is not the
minimum guaranteed alignment.

For example, if the type is allocated via kernel allocator then it can
always guarantee to be at least usize-aligned. ZST can be arbitrarily
aligned so ForeignOwnable` implementation could return a
validly-aligned pointer for XArray. Actually, I think all current
ForeignOwnable implementation can be modified to always give 4-byte
aligned pointers.

Having a const associated item indicating the minimum guaranteed
alignment for *that specific container* is the correct way IMO, not to
reason about the pointee type!

Best,
Gary

>      type Borrowed<'a> = &'a T;
>      type BorrowedMut<'a> = &'a mut T;
>  
> -    fn into_foreign(self) -> *mut crate::ffi::c_void {
> -        Box::into_raw(self).cast()
> +    fn into_foreign(self) -> *mut Self::PointedTo {
> +        Box::into_raw(self)
>      }
>  
> -    unsafe fn from_foreign(ptr: *mut crate::ffi::c_void) -> Self {
> +    unsafe fn from_foreign(ptr: *mut Self::PointedTo) -> Self {
>          // SAFETY: The safety requirements of this function ensure that `ptr` comes from a previous
>          // call to `Self::into_foreign`.
> -        unsafe { Box::from_raw(ptr.cast()) }
> +        unsafe { Box::from_raw(ptr) }
>      }
>  
> -    unsafe fn borrow<'a>(ptr: *mut crate::ffi::c_void) -> &'a T {
> +    unsafe fn borrow<'a>(ptr: *mut Self::PointedTo) -> &'a T {
>          // SAFETY: The safety requirements of this method ensure that the object remains alive and
>          // immutable for the duration of 'a.
> -        unsafe { &*ptr.cast() }
> +        unsafe { &*ptr }
>      }
>  
> -    unsafe fn borrow_mut<'a>(ptr: *mut crate::ffi::c_void) -> &'a mut T {
> -        let ptr = ptr.cast();
> +    unsafe fn borrow_mut<'a>(ptr: *mut Self::PointedTo) -> &'a mut T {
>          // SAFETY: The safety requirements of this method ensure that the pointer is valid and that
>          // nothing else will access the value for the duration of 'a.
>          unsafe { &mut *ptr }
>      }
>  }