Merge pull request #444 from Wasabi375/page_table_bug

Ensure all page table frames are mapped as writable
rust-osdev · May 29, 2024 · e10010e · e10010e
2 parents 972aaa7 + f8a629c
commit e10010e
Show file tree

Hide file tree

Showing 3 changed files with 43 additions and 4 deletions.
diff --git a/Changelog.md b/Changelog.md
@@ -1,5 +1,7 @@
 # Unreleased
 
+* Fix bug leading to page table frames that are not mapped as writable
+
 # 0.11.7 – 2024-02-16
 
 * Set `NO_EXECUTE` flag for all writable memory regions by @phil-opp in https://github.com/rust-osdev/bootloader/pull/409

diff --git a/common/src/lib.rs b/common/src/lib.rs
@@ -241,8 +241,18 @@ where
         context_switch_function_start_frame,
         context_switch_function_start_frame + 1,
     ) {
+        let page = Page::containing_address(VirtAddr::new(frame.start_address().as_u64()));
         match unsafe {
-            kernel_page_table.identity_map(frame, PageTableFlags::PRESENT, frame_allocator)
+            // The parent table flags need to be both readable and writable to
+            // support recursive page tables.
+            // See https://github.com/rust-osdev/bootloader/issues/443#issuecomment-2130010621
+            kernel_page_table.map_to_with_table_flags(
+                page,
+                frame,
+                PageTableFlags::PRESENT,
+                PageTableFlags::PRESENT | PageTableFlags::WRITABLE,
+                frame_allocator,
+            )
         } {
             Ok(tlb) => tlb.flush(),
             Err(err) => panic!("failed to identity map frame {:?}: {:?}", frame, err),
@@ -254,8 +264,17 @@ where
         .allocate_frame()
         .expect("failed to allocate GDT frame");
     gdt::create_and_load(gdt_frame);
+    let gdt_page = Page::containing_address(VirtAddr::new(gdt_frame.start_address().as_u64()));
     match unsafe {
-        kernel_page_table.identity_map(gdt_frame, PageTableFlags::PRESENT, frame_allocator)
+        // The parent table flags need to be both readable and writable to
+        // support recursive page tables.
+        kernel_page_table.map_to_with_table_flags(
+            gdt_page,
+            gdt_frame,
+            PageTableFlags::PRESENT,
+            PageTableFlags::PRESENT | PageTableFlags::WRITABLE,
+            frame_allocator,
+        )
     } {
         Ok(tlb) => tlb.flush(),
         Err(err) => panic!("failed to identity map frame {:?}: {:?}", gdt_frame, err),

diff --git a/common/src/load_kernel.rs b/common/src/load_kernel.rs
@@ -185,8 +185,17 @@ where
             let offset = frame - start_frame;
             let page = start_page + offset;
             let flusher = unsafe {
+                // The parent table flags need to be both readable and writable to
+                // support recursive page tables.
+                // See https://github.com/rust-osdev/bootloader/issues/443#issuecomment-2130010621
                 self.page_table
-                    .map_to(page, frame, segment_flags, self.frame_allocator)
+                    .map_to_with_table_flags(
+                        page,
+                        frame,
+                        segment_flags,
+                        Flags::PRESENT | Flags::WRITABLE,
+                        self.frame_allocator,
+                    )
                     .map_err(|_err| "map_to failed")?
             };
             // we operate on an inactive page table, so there's no need to flush anything
@@ -280,8 +289,17 @@ where
 
             // map frame
             let flusher = unsafe {
+                // The parent table flags need to be both readable and writable to
+                // support recursive page tables.
+                // See https://github.com/rust-osdev/bootloader/issues/443#issuecomment-2130010621
                 self.page_table
-                    .map_to(page, frame, segment_flags, self.frame_allocator)
+                    .map_to_with_table_flags(
+                        page,
+                        frame,
+                        segment_flags,
+                        Flags::PRESENT | Flags::WRITABLE,
+                        self.frame_allocator,
+                    )
                     .map_err(|_err| "Failed to map new frame for bss memory")?
             };
             // we operate on an inactive page table, so we don't need to flush our changes