From 8a257cc78094c4f1d7fbaaa8c1c7ba11aa62bafe Mon Sep 17 00:00:00 2001 From: Philipp Oppermann Date: Fri, 6 Mar 2020 13:21:30 +0100 Subject: [PATCH 1/5] Revert "Add new UnsafePhysFrame type and use it in Mapper::map_to" This reverts commit 0c8756feb347044654c8da27de2888f7d1f7b879. --- src/structures/paging/frame_alloc.rs | 42 ++----------------- .../paging/mapper/mapped_page_table.rs | 25 +++++------ src/structures/paging/mapper/mod.rs | 17 ++++---- .../paging/mapper/offset_page_table.rs | 12 +++--- .../paging/mapper/recursive_page_table.rs | 22 +++++----- 5 files changed, 41 insertions(+), 77 deletions(-) diff --git a/src/structures/paging/frame_alloc.rs b/src/structures/paging/frame_alloc.rs index 26efca0d1..6e80f15fe 100644 --- a/src/structures/paging/frame_alloc.rs +++ b/src/structures/paging/frame_alloc.rs @@ -1,7 +1,6 @@ //! Traits for abstracting away frame allocation and deallocation. -use crate::structures::paging::{PageSize, PhysFrame, Size4KiB}; -use core::ops::{Deref, DerefMut}; +use crate::structures::paging::{PageSize, PhysFrame}; /// A trait for types that can allocate a frame of memory. /// @@ -9,46 +8,11 @@ use core::ops::{Deref, DerefMut}; /// the `allocate_frame` method returns only unique unused frames. pub unsafe trait FrameAllocator { /// Allocate a frame of the appropriate size and return it if possible. - fn allocate_frame(&mut self) -> Option>; + fn allocate_frame(&mut self) -> Option>; } /// A trait for types that can deallocate a frame of memory. pub trait FrameDeallocator { /// Deallocate the given frame of memory. - fn deallocate_frame(&mut self, frame: UnusedPhysFrame); -} - -/// Represents a physical frame that is not used for any mapping. -#[derive(Debug)] -pub struct UnusedPhysFrame(PhysFrame); - -impl UnusedPhysFrame { - /// Creates a new UnusedPhysFrame from the given frame. - /// - /// ## Safety - /// - /// This method is unsafe because the caller must guarantee - /// that the given frame is unused. - pub unsafe fn new(frame: PhysFrame) -> Self { - Self(frame) - } - - /// Returns the physical frame as `PhysFrame` type. - pub fn frame(self) -> PhysFrame { - self.0 - } -} - -impl Deref for UnusedPhysFrame { - type Target = PhysFrame; - - fn deref(&self) -> &Self::Target { - &self.0 - } -} - -impl DerefMut for UnusedPhysFrame { - fn deref_mut(&mut self) -> &mut Self::Target { - &mut self.0 - } + fn deallocate_frame(&mut self, frame: PhysFrame); } diff --git a/src/structures/paging/mapper/mapped_page_table.rs b/src/structures/paging/mapper/mapped_page_table.rs index b5a4711e0..4e706fa83 100644 --- a/src/structures/paging/mapper/mapped_page_table.rs +++ b/src/structures/paging/mapper/mapped_page_table.rs @@ -41,7 +41,7 @@ impl<'a, P: PhysToVirt> MappedPageTable<'a, P> { fn map_to_1gib( &mut self, page: Page, - frame: UnusedPhysFrame, + frame: PhysFrame, flags: PageTableFlags, allocator: &mut A, ) -> Result, MapToError> @@ -66,7 +66,7 @@ impl<'a, P: PhysToVirt> MappedPageTable<'a, P> { fn map_to_2mib( &mut self, page: Page, - frame: UnusedPhysFrame, + frame: PhysFrame, flags: PageTableFlags, allocator: &mut A, ) -> Result, MapToError> @@ -94,7 +94,7 @@ impl<'a, P: PhysToVirt> MappedPageTable<'a, P> { fn map_to_4kib( &mut self, page: Page, - frame: UnusedPhysFrame, + frame: PhysFrame, flags: PageTableFlags, allocator: &mut A, ) -> Result, MapToError> @@ -115,17 +115,17 @@ impl<'a, P: PhysToVirt> MappedPageTable<'a, P> { if !p1[page.p1_index()].is_unused() { return Err(MapToError::PageAlreadyMapped(frame)); } - p1[page.p1_index()].set_frame(frame.frame(), flags); + p1[page.p1_index()].set_frame(frame, flags); Ok(MapperFlush::new(page)) } } impl<'a, P: PhysToVirt> Mapper for MappedPageTable<'a, P> { - fn map_to( + unsafe fn map_to( &mut self, page: Page, - frame: UnusedPhysFrame, + frame: PhysFrame, flags: PageTableFlags, allocator: &mut A, ) -> Result, MapToError> @@ -195,10 +195,10 @@ impl<'a, P: PhysToVirt> Mapper for MappedPageTable<'a, P> { } impl<'a, P: PhysToVirt> Mapper for MappedPageTable<'a, P> { - fn map_to( + unsafe fn map_to( &mut self, page: Page, - frame: UnusedPhysFrame, + frame: PhysFrame, flags: PageTableFlags, allocator: &mut A, ) -> Result, MapToError> @@ -276,10 +276,10 @@ impl<'a, P: PhysToVirt> Mapper for MappedPageTable<'a, P> { } impl<'a, P: PhysToVirt> Mapper for MappedPageTable<'a, P> { - fn map_to( + unsafe fn map_to( &mut self, page: Page, - frame: UnusedPhysFrame, + frame: PhysFrame, flags: PageTableFlags, allocator: &mut A, ) -> Result, MapToError> @@ -465,10 +465,7 @@ impl PageTableWalker

{ if entry.is_unused() { if let Some(frame) = allocator.allocate_frame() { - entry.set_frame( - frame.frame(), - PageTableFlags::PRESENT | PageTableFlags::WRITABLE, - ); + entry.set_frame(frame, PageTableFlags::PRESENT | PageTableFlags::WRITABLE); created = true; } else { return Err(PageTableCreateError::FrameAllocationFailed); diff --git a/src/structures/paging/mapper/mod.rs b/src/structures/paging/mapper/mod.rs index 96b95a90c..80eb3c98f 100644 --- a/src/structures/paging/mapper/mod.rs +++ b/src/structures/paging/mapper/mod.rs @@ -5,9 +5,8 @@ pub use self::mapped_page_table::{MappedPageTable, PhysToVirt}; pub use self::{offset_page_table::OffsetPageTable, recursive_page_table::RecursivePageTable}; use crate::structures::paging::{ - frame_alloc::{FrameAllocator, UnusedPhysFrame}, - page_table::PageTableFlags, - Page, PageSize, PhysFrame, Size1GiB, Size2MiB, Size4KiB, + frame_alloc::FrameAllocator, page_table::PageTableFlags, Page, PageSize, PhysFrame, Size1GiB, + Size2MiB, Size4KiB, }; use crate::{PhysAddr, VirtAddr}; @@ -83,10 +82,13 @@ pub trait Mapper { /// /// This function might need additional physical frames to create new page tables. These /// frames are allocated from the `allocator` argument. At most three frames are required. - fn map_to( + /// + /// This function is unsafe because the caller must guarantee that passed `frame` is + /// unused, i.e. not used for any other mappings. + unsafe fn map_to( &mut self, page: Page, - frame: UnusedPhysFrame, + frame: PhysFrame, flags: PageTableFlags, frame_allocator: &mut A, ) -> Result, MapToError> @@ -116,11 +118,12 @@ pub trait Mapper { /// /// ## Safety /// - /// TODO: Should this function be safe? + /// This function is unsafe because the caller must guarantee that the passed `frame` is + /// unused, i.e. not used for any other mappings. #[inline] unsafe fn identity_map( &mut self, - frame: UnusedPhysFrame, + frame: PhysFrame, flags: PageTableFlags, frame_allocator: &mut A, ) -> Result, MapToError> diff --git a/src/structures/paging/mapper/offset_page_table.rs b/src/structures/paging/mapper/offset_page_table.rs index 76679113e..a646c93c1 100644 --- a/src/structures/paging/mapper/offset_page_table.rs +++ b/src/structures/paging/mapper/offset_page_table.rs @@ -53,10 +53,10 @@ impl PhysToVirt for PhysOffset { // delegate all trait implementations to inner impl<'a> Mapper for OffsetPageTable<'a> { - fn map_to( + unsafe fn map_to( &mut self, page: Page, - frame: UnusedPhysFrame, + frame: PhysFrame, flags: PageTableFlags, allocator: &mut A, ) -> Result, MapToError> @@ -91,10 +91,10 @@ impl<'a> Mapper for OffsetPageTable<'a> { impl<'a> Mapper for OffsetPageTable<'a> { #[inline] - fn map_to( + unsafe fn map_to( &mut self, page: Page, - frame: UnusedPhysFrame, + frame: PhysFrame, flags: PageTableFlags, allocator: &mut A, ) -> Result, MapToError> @@ -129,10 +129,10 @@ impl<'a> Mapper for OffsetPageTable<'a> { impl<'a> Mapper for OffsetPageTable<'a> { #[inline] - fn map_to( + unsafe fn map_to( &mut self, page: Page, - frame: UnusedPhysFrame, + frame: PhysFrame, flags: PageTableFlags, allocator: &mut A, ) -> Result, MapToError> diff --git a/src/structures/paging/mapper/recursive_page_table.rs b/src/structures/paging/mapper/recursive_page_table.rs index daeafdb0e..7c1c7f37f 100644 --- a/src/structures/paging/mapper/recursive_page_table.rs +++ b/src/structures/paging/mapper/recursive_page_table.rs @@ -113,7 +113,7 @@ impl<'a> RecursivePageTable<'a> { if entry.is_unused() { if let Some(frame) = allocator.allocate_frame() { - entry.set_frame(frame.frame(), Flags::PRESENT | Flags::WRITABLE); + entry.set_frame(frame, Flags::PRESENT | Flags::WRITABLE); created = true; } else { return Err(MapToError::FrameAllocationFailed); @@ -141,7 +141,7 @@ impl<'a> RecursivePageTable<'a> { fn map_to_1gib( &mut self, page: Page, - frame: UnusedPhysFrame, + frame: PhysFrame, flags: PageTableFlags, allocator: &mut A, ) -> Result, MapToError> @@ -167,7 +167,7 @@ impl<'a> RecursivePageTable<'a> { fn map_to_2mib( &mut self, page: Page, - frame: UnusedPhysFrame, + frame: PhysFrame, flags: PageTableFlags, allocator: &mut A, ) -> Result, MapToError> @@ -196,7 +196,7 @@ impl<'a> RecursivePageTable<'a> { fn map_to_4kib( &mut self, page: Page, - frame: UnusedPhysFrame, + frame: PhysFrame, flags: PageTableFlags, allocator: &mut A, ) -> Result, MapToError> @@ -217,17 +217,17 @@ impl<'a> RecursivePageTable<'a> { if !p1[page.p1_index()].is_unused() { return Err(MapToError::PageAlreadyMapped(frame)); } - p1[page.p1_index()].set_frame(frame.frame(), flags); + p1[page.p1_index()].set_frame(frame, flags); Ok(MapperFlush::new(page)) } } impl<'a> Mapper for RecursivePageTable<'a> { - fn map_to( + unsafe fn map_to( &mut self, page: Page, - frame: UnusedPhysFrame, + frame: PhysFrame, flags: PageTableFlags, allocator: &mut A, ) -> Result, MapToError> @@ -310,10 +310,10 @@ impl<'a> Mapper for RecursivePageTable<'a> { impl<'a> Mapper for RecursivePageTable<'a> { #[inline] - fn map_to( + unsafe fn map_to( &mut self, page: Page, - frame: UnusedPhysFrame, + frame: PhysFrame, flags: PageTableFlags, allocator: &mut A, ) -> Result, MapToError> @@ -415,10 +415,10 @@ impl<'a> Mapper for RecursivePageTable<'a> { } impl<'a> Mapper for RecursivePageTable<'a> { - fn map_to( + unsafe fn map_to( &mut self, page: Page, - frame: UnusedPhysFrame, + frame: PhysFrame, flags: PageTableFlags, allocator: &mut A, ) -> Result, MapToError> From 08c0df5e7f79fddcf0adb176766cab652323be10 Mon Sep 17 00:00:00 2001 From: Philipp Oppermann Date: Fri, 6 Mar 2020 13:31:14 +0100 Subject: [PATCH 2/5] Remove remaining uses of UnusedPhysFrame --- src/structures/paging/mapper/mod.rs | 2 +- src/structures/paging/mod.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/structures/paging/mapper/mod.rs b/src/structures/paging/mapper/mod.rs index 80eb3c98f..9a10b9828 100644 --- a/src/structures/paging/mapper/mod.rs +++ b/src/structures/paging/mapper/mod.rs @@ -175,7 +175,7 @@ pub enum MapToError { /// given page is part of an already mapped huge page. ParentEntryHugePage, /// The given page is already mapped to a physical frame. - PageAlreadyMapped(UnusedPhysFrame), + PageAlreadyMapped(PhysFrame), } /// An error indicating that an `unmap` call failed. diff --git a/src/structures/paging/mod.rs b/src/structures/paging/mod.rs index 2458953f8..c972cf803 100644 --- a/src/structures/paging/mod.rs +++ b/src/structures/paging/mod.rs @@ -3,7 +3,7 @@ //! Page tables translate virtual memory “pages” to physical memory “frames”. pub use self::frame::PhysFrame; -pub use self::frame_alloc::{FrameAllocator, FrameDeallocator, UnusedPhysFrame}; +pub use self::frame_alloc::{FrameAllocator, FrameDeallocator}; #[doc(no_inline)] pub use self::mapper::MappedPageTable; pub use self::mapper::{Mapper, MapperAllSizes}; From d2bcfe15cf6b590e5514cf6fc44bfb55a019ebcf Mon Sep 17 00:00:00 2001 From: Philipp Oppermann Date: Fri, 6 Mar 2020 13:31:58 +0100 Subject: [PATCH 3/5] Re-add UnusedPhysFrame as a deprecated type to reduce breakage --- src/structures/paging/frame_alloc.rs | 42 +++++++++++++++++++++++++++- src/structures/paging/mod.rs | 2 ++ 2 files changed, 43 insertions(+), 1 deletion(-) diff --git a/src/structures/paging/frame_alloc.rs b/src/structures/paging/frame_alloc.rs index 6e80f15fe..900424a8b 100644 --- a/src/structures/paging/frame_alloc.rs +++ b/src/structures/paging/frame_alloc.rs @@ -1,6 +1,7 @@ //! Traits for abstracting away frame allocation and deallocation. -use crate::structures::paging::{PageSize, PhysFrame}; +use crate::structures::paging::{PageSize, PhysFrame, Size4KiB}; +use core::ops::{Deref, DerefMut}; /// A trait for types that can allocate a frame of memory. /// @@ -16,3 +17,42 @@ pub trait FrameDeallocator { /// Deallocate the given frame of memory. fn deallocate_frame(&mut self, frame: PhysFrame); } + +/// Represents a physical frame that is not used for any mapping. +#[deprecated(note = "This wrapper type is no longer used. Use `PhysFrame` instead.")] +#[derive(Debug)] +pub struct UnusedPhysFrame(PhysFrame); + +#[allow(deprecated)] +impl UnusedPhysFrame { + /// Creates a new UnusedPhysFrame from the given frame. + /// + /// ## Safety + /// + /// This method is unsafe because the caller must guarantee + /// that the given frame is unused. + pub unsafe fn new(frame: PhysFrame) -> Self { + Self(frame) + } + + /// Returns the physical frame as `PhysFrame` type. + pub fn frame(self) -> PhysFrame { + self.0 + } +} + +#[allow(deprecated)] +impl Deref for UnusedPhysFrame { + type Target = PhysFrame; + + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +#[allow(deprecated)] +impl DerefMut for UnusedPhysFrame { + fn deref_mut(&mut self) -> &mut Self::Target { + &mut self.0 + } +} diff --git a/src/structures/paging/mod.rs b/src/structures/paging/mod.rs index c972cf803..5204fcff7 100644 --- a/src/structures/paging/mod.rs +++ b/src/structures/paging/mod.rs @@ -3,6 +3,8 @@ //! Page tables translate virtual memory “pages” to physical memory “frames”. pub use self::frame::PhysFrame; +#[allow(deprecated)] +pub use self::frame_alloc::UnusedPhysFrame; pub use self::frame_alloc::{FrameAllocator, FrameDeallocator}; #[doc(no_inline)] pub use self::mapper::MappedPageTable; From 5f578f73df028ce47d189a825b0e84cd19d94be0 Mon Sep 17 00:00:00 2001 From: Philipp Oppermann Date: Fri, 6 Mar 2020 14:02:15 +0100 Subject: [PATCH 4/5] Add more extensive docs on safety to `map_to` --- src/structures/paging/mapper/mod.rs | 34 +++++++++++++++++++++++++---- 1 file changed, 30 insertions(+), 4 deletions(-) diff --git a/src/structures/paging/mapper/mod.rs b/src/structures/paging/mapper/mod.rs index 9a10b9828..01c15cb36 100644 --- a/src/structures/paging/mapper/mod.rs +++ b/src/structures/paging/mapper/mod.rs @@ -83,8 +83,34 @@ pub trait Mapper { /// This function might need additional physical frames to create new page tables. These /// frames are allocated from the `allocator` argument. At most three frames are required. /// - /// This function is unsafe because the caller must guarantee that passed `frame` is - /// unused, i.e. not used for any other mappings. + /// ## Safety + /// + /// Creating page table mappings is a fundamentally unsafe operation because + /// there are various ways to break memory safety through it. For example, + /// re-mapping an in-use page to a different frame changes and invalidates + /// all values stored in that page, resulting in undefined behavior on the + /// next use. + /// + /// The caller must ensure that no undefined behavior or memory safety + /// violations can occur through the new mapping. Among other things, the + /// caller must prevent the following: + /// + /// - Aliasing of `&mut` references, i.e. two `&mut` references that point to + /// the same physical address. This is undefined behavior in Rust. + /// - This can be ensured by mapping each page to an individual physical + /// frame that is not mapped anywhere else. + /// - Creating uninitalized or invalid values: Rust requires that all values + /// have a correct memory layout. For example, a `bool` must be either a 0 + /// or a 1 in memory, but not a 3 or 4. An exception is the `MaybeUninit` + /// wrapper type, which abstracts over possibly uninitialized memory. + /// - This is only a problem when re-mapping pages to different physical + /// frames. Mapping a page that is not in use yet is fine. + /// + /// Special care must be taken when sharing pages with other address spaces, + /// e.g. by setting the `GLOBAL` flag. For example, a global mapping must be + /// the same in all address spaces, otherwise undefined behavior can occur + /// because of TLB races. It's worth noting that all the above requirements + /// also apply to shared mappings, including the aliasing requirements. unsafe fn map_to( &mut self, page: Page, @@ -118,8 +144,8 @@ pub trait Mapper { /// /// ## Safety /// - /// This function is unsafe because the caller must guarantee that the passed `frame` is - /// unused, i.e. not used for any other mappings. + /// This is a convencience function that invokes [`map_to`] internally, so + /// all safety requirements of it also apply for this function. #[inline] unsafe fn identity_map( &mut self, From 985d6dbbe0bb085b8b86c5d06d78f6990cb8f454 Mon Sep 17 00:00:00 2001 From: Philipp Oppermann Date: Fri, 6 Mar 2020 14:41:53 +0100 Subject: [PATCH 5/5] Make update_flags unsafe too --- src/structures/paging/mapper/mapped_page_table.rs | 6 +++--- src/structures/paging/mapper/mod.rs | 10 +++++++++- src/structures/paging/mapper/offset_page_table.rs | 6 +++--- src/structures/paging/mapper/recursive_page_table.rs | 12 +++++++++--- 4 files changed, 24 insertions(+), 10 deletions(-) diff --git a/src/structures/paging/mapper/mapped_page_table.rs b/src/structures/paging/mapper/mapped_page_table.rs index 4e706fa83..9a0486ec7 100644 --- a/src/structures/paging/mapper/mapped_page_table.rs +++ b/src/structures/paging/mapper/mapped_page_table.rs @@ -161,7 +161,7 @@ impl<'a, P: PhysToVirt> Mapper for MappedPageTable<'a, P> { Ok((frame, MapperFlush::new(page))) } - fn update_flags( + unsafe fn update_flags( &mut self, page: Page, flags: PageTableFlags, @@ -237,7 +237,7 @@ impl<'a, P: PhysToVirt> Mapper for MappedPageTable<'a, P> { Ok((frame, MapperFlush::new(page))) } - fn update_flags( + unsafe fn update_flags( &mut self, page: Page, flags: PageTableFlags, @@ -315,7 +315,7 @@ impl<'a, P: PhysToVirt> Mapper for MappedPageTable<'a, P> { Ok((frame, MapperFlush::new(page))) } - fn update_flags( + unsafe fn update_flags( &mut self, page: Page, flags: PageTableFlags, diff --git a/src/structures/paging/mapper/mod.rs b/src/structures/paging/mapper/mod.rs index 01c15cb36..85b6ac856 100644 --- a/src/structures/paging/mapper/mod.rs +++ b/src/structures/paging/mapper/mod.rs @@ -128,7 +128,15 @@ pub trait Mapper { fn unmap(&mut self, page: Page) -> Result<(PhysFrame, MapperFlush), UnmapError>; /// Updates the flags of an existing mapping. - fn update_flags( + /// + /// ## Safety + /// + /// This method is unsafe because changing the flags of a mapping + /// might result in undefined behavior. For example, setting the + /// `GLOBAL` and `MUTABLE` flags for a page might result in the corruption + /// of values stored in that page from processes running in other address + /// spaces. + unsafe fn update_flags( &mut self, page: Page, flags: PageTableFlags, diff --git a/src/structures/paging/mapper/offset_page_table.rs b/src/structures/paging/mapper/offset_page_table.rs index a646c93c1..23e4da4d2 100644 --- a/src/structures/paging/mapper/offset_page_table.rs +++ b/src/structures/paging/mapper/offset_page_table.rs @@ -75,7 +75,7 @@ impl<'a> Mapper for OffsetPageTable<'a> { } #[inline] - fn update_flags( + unsafe fn update_flags( &mut self, page: Page, flags: PageTableFlags, @@ -113,7 +113,7 @@ impl<'a> Mapper for OffsetPageTable<'a> { } #[inline] - fn update_flags( + unsafe fn update_flags( &mut self, page: Page, flags: PageTableFlags, @@ -151,7 +151,7 @@ impl<'a> Mapper for OffsetPageTable<'a> { } #[inline] - fn update_flags( + unsafe fn update_flags( &mut self, page: Page, flags: PageTableFlags, diff --git a/src/structures/paging/mapper/recursive_page_table.rs b/src/structures/paging/mapper/recursive_page_table.rs index 7c1c7f37f..a54455cea 100644 --- a/src/structures/paging/mapper/recursive_page_table.rs +++ b/src/structures/paging/mapper/recursive_page_table.rs @@ -267,7 +267,9 @@ impl<'a> Mapper for RecursivePageTable<'a> { Ok((frame, MapperFlush::new(page))) } - fn update_flags( + // allow unused_unsafe until https://github.com/rust-lang/rfcs/pull/2585 lands + #[allow(unused_unsafe)] + unsafe fn update_flags( &mut self, page: Page, flags: PageTableFlags, @@ -359,7 +361,9 @@ impl<'a> Mapper for RecursivePageTable<'a> { Ok((frame, MapperFlush::new(page))) } - fn update_flags( + // allow unused_unsafe until https://github.com/rust-lang/rfcs/pull/2585 lands + #[allow(unused_unsafe)] + unsafe fn update_flags( &mut self, page: Page, flags: PageTableFlags, @@ -465,7 +469,9 @@ impl<'a> Mapper for RecursivePageTable<'a> { Ok((frame, MapperFlush::new(page))) } - fn update_flags( + // allow unused_unsafe until https://github.com/rust-lang/rfcs/pull/2585 lands + #[allow(unused_unsafe)] + unsafe fn update_flags( &mut self, page: Page, flags: PageTableFlags,