Message-ID: <CAG48ez3fjXG1Zi=V8yte9ZgSkDVeJiQV6xau7FHocTiTMw0d=w@mail.gmail.com>
Date: Tue, 19 Nov 2024 18:07:43 +0100
From: Jann Horn <jannh@...gle.com>
To: Abdiel Janulgue <abdiel.janulgue@...il.com>
Cc: rust-for-linux@...r.kernel.org, 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>, 
	Danilo Krummrich <dakr@...nel.org>, Wedson Almeida Filho <wedsonaf@...il.com>, 
	Valentin Obst <kernel@...entinobst.de>, open list <linux-kernel@...r.kernel.org>, 
	Andrew Morton <akpm@...ux-foundation.org>, 
	"open list:MEMORY MANAGEMENT" <linux-mm@...ck.org>, airlied@...hat.com
Subject: Re: [PATCH v3 2/2] rust: page: Extend support to existing struct page mappings

Commenting as someone who understands kernel MM decently but only
knows a tiny bit about Rust:

On Tue, Nov 19, 2024 at 12:24 PM Abdiel Janulgue
<abdiel.janulgue@...il.com> wrote:
> Extend Page to support pages that are not allocated by the constructor,
> for example, those returned by vmalloc_to_page() or virt_to_page().
> Since we don't own those pages we shouldn't Drop them either. Hence we
> take advantage of the switch to Opaque so we can cast to a Page pointer
> from a struct page pointer and be able to retrieve the reference on an
> existing struct page mapping. In this case no destructor will be called
> since we are not instantiating a new Page instance.
>
> The new page_slice_to_page wrapper ensures that it explicity accepts
> only page-sized chunks.
>
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@...il.com>
[...]
> diff --git a/rust/kernel/page.rs b/rust/kernel/page.rs
> index fdf7ee203597..d0a896f53afb 100644
> --- a/rust/kernel/page.rs
> +++ b/rust/kernel/page.rs
> @@ -3,7 +3,7 @@
>  //! Kernel page allocation and management.
>
>  use crate::{
> -    alloc::{AllocError, Flags},
> +    alloc::{AllocError, Allocator, Flags, VVec, KVec, KVVec, Vec, flags::*},
>      bindings,
>      error::code::*,
>      error::Result,
> @@ -87,6 +87,49 @@ pub fn alloc_page(flags: Flags) -> Result<ARef<Self>, AllocError> {
>          Ok(unsafe { ARef::from_raw(NonNull::new_unchecked(ptr)) })
>      }
>
> +    /// Create a page object from a buffer which is associated with an existing C `struct page`.
> +    ///
> +    /// This function ensures it takes a page-sized buffer as represented by `PageSlice`.
> +    ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// use kernel::page::*;
> +    ///
> +    /// let somedata: [u8; PAGE_SIZE * 2] = [0; PAGE_SIZE * 2];
> +    /// let buf: &[u8] = &somedata;
> +    /// let pages: VVec<PageSlice> = buf.try_into()?;
> +    /// let page = Page::page_slice_to_page(&pages[0])?;
> +    /// # Ok::<(), Error>(())
> +    /// ```
> +    pub fn page_slice_to_page<'a>(page: &PageSlice) -> Result<&'a Self>

Sorry, can you explain to me what the semantics of this are? Does this
create a Page reference that is not lifetime-bound to the PageSlice?

> +    {
> +        let ptr: *const core::ffi::c_void = page.0.as_ptr() as _;
> +        if ptr.is_null() {
> +            return Err(EINVAL)
> +        }
> +        // SAFETY: We've checked that `ptr` is non-null, hence it's safe to call this method.
> +        let page = if unsafe { bindings::is_vmalloc_addr(ptr) } {
> +            // SAFETY: We've checked that `ptr` is non-null and within the vmalloc range, hence
> +            // it's safe to call this method.
> +            unsafe { bindings::vmalloc_to_page(ptr) }
> +        // SAFETY: We've checked that `ptr` is non-null, hence it's safe to call this method.
> +        } else if unsafe { bindings::virt_addr_valid(ptr) } {
> +            // SAFETY: We've checked that `ptr` is non-null and a valid virtual address, hence
> +            // it's safe to call this method.
> +            unsafe { bindings::virt_to_page(ptr) }
> +        } else {
> +            ptr::null_mut()
> +        };
> +        if page.is_null() {
> +            return Err(EINVAL);
> +        }
> +        // CAST: `Self` is a `repr(transparent)` wrapper around `bindings::page`.
> +        // SAFETY: We just successfully retrieved an existing `bindings::page`, therefore
> +        // dereferencing the page pointer is valid.
> +        Ok(unsafe { &*page.cast() })
> +    }
> +
>      /// Returns a raw pointer to the page.
>      pub fn as_ptr(&self) -> *mut bindings::page {
>          self.page.get()
> @@ -270,3 +313,55 @@ unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
>          unsafe { bindings::put_page(obj.cast().as_ptr()) }
>      }
>  }
> +
> +/// A page-aligned, page-sized object.
> +///
> +/// This is used for convenience to convert a large buffer into an array of page-sized chunks
> +/// allocated with the kernel's allocators which can then be used in the
> +/// `Page::page_slice_to_page` wrapper.
> +///
> +// FIXME: This should be `PAGE_SIZE`, but the compiler rejects everything except a literal
> +// integer argument for the `repr(align)` attribute.
> +#[repr(align(4096))]
> +pub struct PageSlice([u8; PAGE_SIZE]);
> +
> +fn to_vec_with_allocator<A: Allocator>(val: &[u8]) -> Result<Vec<PageSlice, A>, AllocError> {
> +    let mut k = Vec::<PageSlice, A>::new();
> +    let pages = page_align(val.len()) >> PAGE_SHIFT;
> +    match k.reserve(pages, GFP_KERNEL) {

Do I understand correctly that this can be used to create a kmalloc
allocation whose pages can then basically be passed to
page_slice_to_page()?

FYI, the page refcount does not protect against UAF of slab
allocations through new slab allocations of the same size. In other
words: The slab allocator can internally recycle memory without going
through the page allocator, and the slab allocator itself does not
care about page refcounts.

If the Page returned from calling page_slice_to_page() on the slab
memory pages returned from to_vec_with_allocator() is purely usable as
a borrow and there is no way to later grab a refcounted reference to
it or pass it into a C function that assumes it can grab a reference
to the page, I guess that works. But if I understand correctly what's
going on here, and you can grab references to slab pages returned from
page_slice_to_page(to_vec_with_allocator()), that'd be bad.

> +        Ok(()) => {
> +            // SAFETY: from above, the length should be equal to the vector's capacity
> +            unsafe { k.set_len(pages); }
> +            // SAFETY: src buffer sized val.len() does not overlap with dst buffer since
> +            // the dst buffer's size is val.len() padded up to a multiple of PAGE_SIZE.
> +            unsafe { ptr::copy_nonoverlapping(val.as_ptr(), k.as_mut_ptr() as *mut u8,
> +                                              val.len()) };
> +            Ok(k)
> +        },
> +        Err(_) => Err(AllocError),
> +    }
> +}

Powered by blists - more mailing lists