linux-kernel - Re: [PATCH 5/6] rust: page: Add physical address conversion functions

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: <87r03ucnh3.fsf@kernel.org>
Date: Wed, 19 Feb 2025 10:06:48 +0100
From: Andreas Hindborg <a.hindborg@...nel.org>
To: "Asahi Lina" <lina@...hilina.net>
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>,  "Alice Ryhl" <aliceryhl@...gle.com>,
  "Trevor Gross" <tmgross@...ch.edu>,  "Jann Horn" <jannh@...gle.com>,
  "Matthew Wilcox" <willy@...radead.org>,  "Paolo Bonzini"
 <pbonzini@...hat.com>,  "Danilo Krummrich" <dakr@...nel.org>,  "Wedson
 Almeida Filho" <wedsonaf@...il.com>,  "Valentin Obst"
 <kernel@...entinobst.de>,  "Andrew Morton" <akpm@...ux-foundation.org>,
  <linux-mm@...ck.org>,  <airlied@...hat.com>,  "Abdiel Janulgue"
 <abdiel.janulgue@...il.com>,  <rust-for-linux@...r.kernel.org>,
  <linux-kernel@...r.kernel.org>,  <asahi@...ts.linux.dev>
Subject: Re: [PATCH 5/6] rust: page: Add physical address conversion functions

"Asahi Lina" <lina@...hilina.net> writes:

[...]

>  /// A bitwise shift for the page size.
> @@ -249,6 +251,69 @@ pub unsafe fn copy_from_user_slice_raw(
>              reader.read_raw(unsafe { core::slice::from_raw_parts_mut(dst.cast(), len) })
>          })
>      }
> +
> +    /// Returns the physical address of this page.
> +    pub fn phys(&self) -> PhysicalAddr {
> +        // SAFETY: `page` is valid due to the type invariants on `Page`.
> +        unsafe { bindings::page_to_phys(self.as_ptr()) }
> +    }
> +
> +    /// Converts a Rust-owned Page into its physical address.
> +    ///
> +    /// The caller is responsible for calling [`Page::from_phys()`] to avoid leaking memory.
> +    pub fn into_phys(this: Owned<Self>) -> PhysicalAddr {
> +        ManuallyDrop::new(this).phys()
> +    }
> +
> +    /// Converts a physical address to a Rust-owned Page.
> +    ///
> +    /// # Safety
> +    /// The caller must ensure that the physical address was previously returned by a call to
> +    /// [`Page::into_phys()`], and that the physical address is no longer used after this call,
> +    /// nor is [`Page::from_phys()`] called again on it.

Do we really need the `PhysicalAddr` to come from a call to
`Page::into_phys`? Could we relax this and say that we don't care how
you came about the `PhysicalAddr` as long as you can guarantee that
ownership is correct? That would make interop with C easer in some cases.

> +    pub unsafe fn from_phys(phys: PhysicalAddr) -> Owned<Self> {
> +        // SAFETY: By the safety requirements, the physical address must be valid and
> +        // have come from `into_phys()`, so phys_to_page() cannot fail and
> +        // must return the original struct page pointer.
> +        unsafe { Owned::from_raw(NonNull::new_unchecked(bindings::phys_to_page(phys)).cast()) }
> +    }
> +
> +    /// Borrows a Page from a physical address, without taking over ownership.
> +    ///
> +    /// If the physical address does not have a `struct page` entry or is not
> +    /// part of a System RAM region, returns None.
> +    ///
> +    /// # Safety
> +    /// The caller must ensure that the physical address, if it is backed by a `struct page`,
> +    /// remains available for the duration of the borrowed lifetime.
> +    pub unsafe fn borrow_phys(phys: &PhysicalAddr) -> Option<&Self> {
> +        // SAFETY: This is always safe, as it is just arithmetic
> +        let pfn = unsafe { bindings::phys_to_pfn(*phys) };
> +        // SAFETY: This function is safe to call with any pfn
> +        if !unsafe { bindings::pfn_valid(pfn) && bindings::page_is_ram(pfn) != 0 } {
> +            None
> +        } else {
> +            // SAFETY: We have just checked that the pfn is valid above, so it must
> +            // have a corresponding struct page. By the safety requirements, we can
> +            // return a borrowed reference to it.
> +            Some(unsafe { &*(bindings::pfn_to_page(pfn) as *mut Self as *const Self) })

I think you can maybe go
`bindings::pfn_to_page(pfn).cast::<Self>().cast_const()` here.

> +        }
> +    }
> +
> +    /// Borrows a Page from a physical address, without taking over ownership
> +    /// nor checking for validity.
> +    ///
> +    /// # Safety
> +    /// The caller must ensure that the physical address is backed by a `struct page` and
> +    /// corresponds to System RAM. This is true when the address was returned by
> +    /// [`Page::into_phys()`].
> +    pub unsafe fn borrow_phys_unchecked(phys: &PhysicalAddr) -> &Self {
> +        // SAFETY: This is always safe, as it is just arithmetic
> +        let pfn = unsafe { bindings::phys_to_pfn(*phys) };
> +        // SAFETY: The caller guarantees that the pfn is valid. By the safety
> +        // requirements, we can return a borrowed reference to it.
> +        unsafe { &*(bindings::pfn_to_page(pfn) as *mut Self as *const Self) }

Same applies here.


Best regards,
Andreas Hindborg