| 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
| ||
|
Message-ID: <Z6Tlsn2RIiE121Lg@cassiopeiae>
Date: Thu, 6 Feb 2025 17:39:14 +0100
From: Danilo Krummrich <dakr@...nel.org>
To: Tamir Duberstein <tamird@...il.com>
Cc: Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>,
Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
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>,
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 v15 1/3] rust: types: add `ForeignOwnable::PointedTo`
On Thu, Feb 06, 2025 at 11:24:43AM -0500, Tamir Duberstein 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.
>
> Reviewed-by: Andreas Hindborg <a.hindborg@...nel.org>
> Signed-off-by: Tamir Duberstein <tamird@...il.com>
> ---
> rust/kernel/alloc/kbox.rs | 38 ++++++++++++++++++++------------------
> rust/kernel/miscdevice.rs | 7 ++++++-
> rust/kernel/pci.rs | 5 ++++-
> rust/kernel/platform.rs | 5 ++++-
> rust/kernel/sync/arc.rs | 21 ++++++++++++---------
> rust/kernel/types.rs | 46 +++++++++++++++++++++++++++++++---------------
> 6 files changed, 77 insertions(+), 45 deletions(-)
>
> diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs
> index e14433b2ab9d..f1a081dd64c7 100644
> --- a/rust/kernel/miscdevice.rs
> +++ b/rust/kernel/miscdevice.rs
> @@ -225,13 +225,15 @@ impl<T: MiscDevice> VtableHelper<T> {
> Ok(ptr) => ptr,
> Err(err) => return err.to_errno(),
> };
> + let ptr = ptr.into_foreign();
> + let ptr = ptr.cast();
>
> // This overwrites the private data with the value specified by the user, changing the type of
> // this file's private data. All future accesses to the private data is performed by other
> // fops_* methods in this file, which all correctly cast the private data to the new type.
> //
> // SAFETY: The open call of a file can access the private data.
> - unsafe { (*raw_file).private_data = ptr.into_foreign() };
> + unsafe { (*raw_file).private_data = ptr };
Why not just ptr.into_foreign().cast()?
>
> 0
> }
> @@ -246,6 +248,7 @@ impl<T: MiscDevice> VtableHelper<T> {
> ) -> c_int {
> // SAFETY: The release call of a file owns the private data.
> let private = unsafe { (*file).private_data };
> + let private = private.cast();
> // SAFETY: The release call of a file owns the private data.
> let ptr = unsafe { <T::Ptr as ForeignOwnable>::from_foreign(private) };
>
> @@ -267,6 +270,7 @@ impl<T: MiscDevice> VtableHelper<T> {
> ) -> c_long {
> // SAFETY: The ioctl call of a file can access the private data.
> let private = unsafe { (*file).private_data };
> + let private = private.cast();
> // SAFETY: Ioctl calls can borrow the private data of the file.
> let device = unsafe { <T::Ptr as ForeignOwnable>::borrow(private) };
>
> @@ -316,6 +320,7 @@ impl<T: MiscDevice> VtableHelper<T> {
> ) {
> // SAFETY: The release call of a file owns the private data.
> let private = unsafe { (*file).private_data };
> + let private = private.cast();
> // SAFETY: Ioctl calls can borrow the private data of the file.
> let device = unsafe { <T::Ptr as ForeignOwnable>::borrow(private) };
> // SAFETY:
> diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> index 4c98b5b9aa1e..eb25fabbff9c 100644
> --- a/rust/kernel/pci.rs
> +++ b/rust/kernel/pci.rs
> @@ -72,10 +72,12 @@ extern "C" fn probe_callback(
>
> match T::probe(&mut pdev, info) {
> Ok(data) => {
> + let data = data.into_foreign();
> + let data = data.cast();
> // Let the `struct pci_dev` own a reference of the driver's private data.
> // SAFETY: By the type invariant `pdev.as_raw` returns a valid pointer to a
> // `struct pci_dev`.
> - unsafe { bindings::pci_set_drvdata(pdev.as_raw(), data.into_foreign() as _) };
> + unsafe { bindings::pci_set_drvdata(pdev.as_raw(), data) };
This change isn't necessary for this patch, is it? I think it makes sense to
replace `as _` with cast(), but this should be a separate patch then.
> }
> Err(err) => return Error::to_errno(err),
> }
> @@ -87,6 +89,7 @@ extern "C" fn remove_callback(pdev: *mut bindings::pci_dev) {
> // SAFETY: The PCI bus only ever calls the remove callback with a valid pointer to a
> // `struct pci_dev`.
> let ptr = unsafe { bindings::pci_get_drvdata(pdev) };
> + let ptr = ptr.cast();
>
> // SAFETY: `remove_callback` is only ever called after a successful call to
> // `probe_callback`, hence it's guaranteed that `ptr` points to a valid and initialized
> diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
> index 50e6b0421813..53764cb7f804 100644
> --- a/rust/kernel/platform.rs
> +++ b/rust/kernel/platform.rs
> @@ -63,10 +63,12 @@ extern "C" fn probe_callback(pdev: *mut bindings::platform_device) -> kernel::ff
> let info = <Self as driver::Adapter>::id_info(pdev.as_ref());
> match T::probe(&mut pdev, info) {
> Ok(data) => {
> + let data = data.into_foreign();
> + let data = data.cast();
> // Let the `struct platform_device` own a reference of the driver's private data.
> // SAFETY: By the type invariant `pdev.as_raw` returns a valid pointer to a
> // `struct platform_device`.
> - unsafe { bindings::platform_set_drvdata(pdev.as_raw(), data.into_foreign() as _) };
> + unsafe { bindings::platform_set_drvdata(pdev.as_raw(), data) };
Same here.
Powered by blists - more mailing lists