Merge pull request #335 from rust-osdev/kernel-stack-fixes

Create kernel stack with correct size and set up a guard page

Author: phil-opp

Cargo.lock

@@ -25,6 +25,7 @@ members = [
     "tests/test_kernels/pie",
     "tests/test_kernels/lto",
     "tests/test_kernels/ramdisk",
+    "tests/test_kernels/min_stack",
 ]
 exclude = ["examples/basic", "examples/test_framework"]
 
@@ -65,6 +66,7 @@ test_kernel_map_phys_mem = { path = "tests/test_kernels/map_phys_mem", artifact
 test_kernel_pie = { path = "tests/test_kernels/pie", artifact = "bin", target = "x86_64-unknown-none" }
 test_kernel_ramdisk = { path = "tests/test_kernels/ramdisk", artifact = "bin", target = "x86_64-unknown-none" }
 test_kernel_config_file = { path = "tests/test_kernels/config_file", artifact = "bin", target = "x86_64-unknown-none" }
+test_kernel_min_stack = { path = "tests/test_kernels/min_stack", artifact = "bin", target = "x86_64-unknown-none" }
 
 [profile.dev]
 panic = "abort"
@@ -118,6 +120,9 @@ rustflags = [
     "code-model=large",
 ]
 
+[profile.test.package.test_kernel_min_stack]
+opt-level = 2
+
 [build-dependencies]
 llvm-tools = "0.1.1"
 async-process = "1.6.0"

Cargo.toml

@@ -22,8 +22,10 @@ pub struct BootloaderConfig {
     /// The size of the stack that the bootloader should allocate for the kernel (in bytes).
     ///
     /// The bootloader starts the kernel with a valid stack pointer. This setting defines
-    /// the stack size that the bootloader should allocate and map. The stack is created
-    /// with a guard page, so a stack overflow will lead to a page fault.
+    /// the stack size that the bootloader should allocate and map.
+    ///
+    /// The stack is created with a additional guard page, so a stack overflow will lead to
+    /// a page fault.
     pub kernel_stack_size: u64,
 
     /// Configuration for the frame buffer that can be used by the kernel to display pixels
@@ -360,6 +362,14 @@ impl Default for ApiVersion {
 #[non_exhaustive]
 pub struct Mappings {
     /// Configures how the kernel stack should be mapped.
+    ///
+    /// If a fixed address is set, it must be page aligned.
+    ///
+    /// Note that the first page of the kernel stack is intentionally left unmapped
+    /// to act as a guard page. This ensures that a page fault occurs on a stack
+    /// overflow. For example, setting the kernel stack address to
+    /// `FixedAddress(0xf_0000_0000)` will result in a guard page at address
+    /// `0xf_0000_0000` and the kernel stack starting at address `0xf_0000_1000`.
     pub kernel_stack: Mapping,
     /// Specifies where the [`crate::BootInfo`] struct should be placed in virtual memory.
     pub boot_info: Mapping,

api/src/config.rs

@@ -207,17 +207,20 @@ where
     .expect("no entry point");
     log::info!("Entry point at: {:#x}", entry_point.as_u64());
     // create a stack
-    let stack_start_addr = mapping_addr(
-        config.mappings.kernel_stack,
-        config.kernel_stack_size,
-        16,
-        &mut used_entries,
-    );
-    let stack_start: Page = Page::containing_address(stack_start_addr);
-    let stack_end = {
-        let end_addr = stack_start_addr + config.kernel_stack_size;
-        Page::containing_address(end_addr - 1u64)
+    let stack_start = {
+        // we need page-alignment because we want a guard page directly below the stack
+        let guard_page = mapping_addr_page_aligned(
+            config.mappings.kernel_stack,
+            // allocate an additional page as a guard page
+            Size4KiB::SIZE + config.kernel_stack_size,
+            &mut used_entries,
+            "kernel stack start",
+        );
+        guard_page + 1
     };
+    let stack_end_addr = stack_start.start_address() + config.kernel_stack_size;
+
+    let stack_end = Page::containing_address(stack_end_addr - 1u64);
     for page in Page::range_inclusive(stack_start, stack_end) {
         let frame = frame_allocator
             .allocate_frame()
@@ -265,13 +268,12 @@ where
         let framebuffer_start_frame: PhysFrame = PhysFrame::containing_address(framebuffer.addr);
         let framebuffer_end_frame =
             PhysFrame::containing_address(framebuffer.addr + framebuffer.info.byte_len - 1u64);
-        let start_page = Page::from_start_address(mapping_addr(
+        let start_page = mapping_addr_page_aligned(
             config.mappings.framebuffer,
             u64::from_usize(framebuffer.info.byte_len),
-            Size4KiB::SIZE,
             &mut used_entries,
-        ))
-        .expect("the framebuffer address must be page aligned");
+            "framebuffer",
+        );
         for (i, frame) in
             PhysFrame::range_inclusive(framebuffer_start_frame, framebuffer_end_frame).enumerate()
         {
@@ -292,19 +294,17 @@ where
     };
     let ramdisk_slice_len = system_info.ramdisk_len;
     let ramdisk_slice_start = if let Some(ramdisk_address) = system_info.ramdisk_addr {
-        let ramdisk_address_start = mapping_addr(
+        let start_page = mapping_addr_page_aligned(
             config.mappings.ramdisk_memory,
             system_info.ramdisk_len,
-            Size4KiB::SIZE,
             &mut used_entries,
+            "ramdisk start",
         );
         let physical_address = PhysAddr::new(ramdisk_address);
         let ramdisk_physical_start_page: PhysFrame<Size4KiB> =
             PhysFrame::containing_address(physical_address);
         let ramdisk_page_count = (system_info.ramdisk_len - 1) / Size4KiB::SIZE;
         let ramdisk_physical_end_page = ramdisk_physical_start_page + ramdisk_page_count;
-        let start_page = Page::from_start_address(ramdisk_address_start)
-            .expect("the ramdisk start address must be page aligned");
 
         let flags = PageTableFlags::PRESENT | PageTableFlags::WRITABLE;
         for (i, frame) in
@@ -320,7 +320,7 @@ where
                 ),
             };
         }
-        Some(ramdisk_address_start)
+        Some(start_page.start_address())
     } else {
         None
     };
@@ -334,7 +334,8 @@ where
 
         let size = max_phys.as_u64();
         let alignment = Size2MiB::SIZE;
-        let offset = mapping_addr(mapping, size, alignment, &mut used_entries);
+        let offset = mapping_addr(mapping, size, alignment, &mut used_entries)
+            .expect("start address for physical memory mapping must be 2MiB-page-aligned");
 
         for frame in PhysFrame::range_inclusive(start_frame, end_frame) {
             let page = Page::containing_address(offset + frame.start_address().as_u64());
@@ -390,7 +391,10 @@ where
     Mappings {
         framebuffer: framebuffer_virt_addr,
         entry_point,
-        stack_end,
+        // Use the configured stack size, even if it's not page-aligned. However, we
+        // need to align it down to the next 16-byte boundary because the System V
+        // ABI requires a 16-byte stack alignment.
+        stack_top: stack_end_addr.align_down(16u8),
         used_entries,
         physical_memory_offset,
         recursive_index,
@@ -407,8 +411,8 @@ where
 pub struct Mappings {
     /// The entry point address of the kernel.
     pub entry_point: VirtAddr,
-    /// The stack end page of the kernel.
-    pub stack_end: Page,
+    /// The (exclusive) end address of the kernel stack.
+    pub stack_top: VirtAddr,
     /// Keeps track of used entries in the level 4 page table, useful for finding a free
     /// virtual memory when needed.
     pub used_entries: UsedLevel4Entries,
@@ -462,11 +466,8 @@ where
             u64::from_usize(combined.size()),
             u64::from_usize(combined.align()),
             &mut mappings.used_entries,
-        );
-        assert!(
-            boot_info_addr.is_aligned(u64::from_usize(combined.align())),
-            "boot info addr is not properly aligned"
-        );
+        )
+        .expect("boot info addr is not properly aligned");
 
         let memory_map_regions_addr = boot_info_addr + memory_regions_offset;
         let memory_map_regions_end = boot_info_addr + combined.size();
@@ -561,7 +562,7 @@ pub fn switch_to_kernel(
     } = page_tables;
     let addresses = Addresses {
         page_table: kernel_level_4_frame,
-        stack_top: mappings.stack_end.start_address(),
+        stack_top: mappings.stack_top,
         entry_point: mappings.entry_point,
         boot_info,
     };
@@ -612,15 +613,32 @@ struct Addresses {
     boot_info: &'static mut BootInfo,
 }
 
+fn mapping_addr_page_aligned(
+    mapping: Mapping,
+    size: u64,
+    used_entries: &mut UsedLevel4Entries,
+    kind: &str,
+) -> Page {
+    match mapping_addr(mapping, size, Size4KiB::SIZE, used_entries) {
+        Ok(addr) => Page::from_start_address(addr).unwrap(),
+        Err(addr) => panic!("{kind} address must be page-aligned (is `{addr:?})`"),
+    }
+}
+
 fn mapping_addr(
     mapping: Mapping,
     size: u64,
     alignment: u64,
     used_entries: &mut UsedLevel4Entries,
-) -> VirtAddr {
-    match mapping {
+) -> Result<VirtAddr, VirtAddr> {
+    let addr = match mapping {
         Mapping::FixedAddress(addr) => VirtAddr::new(addr),
         Mapping::Dynamic => used_entries.get_free_address(size, alignment),
+    };
+    if addr.is_aligned(alignment) {
+        Ok(addr)
+    } else {
+        Err(addr)
     }
 }
 

common/src/lib.rs

@@ -0,0 +1,6 @@
+use bootloader_test_runner::run_test_kernel;
+
+#[test]
+fn basic_boot() {
+    run_test_kernel(env!("CARGO_BIN_FILE_TEST_KERNEL_MIN_STACK_basic_boot"));
+}

tests/min_stack.rs

@@ -0,0 +1,13 @@
+[package]
+name = "test_kernel_min_stack"
+version = "0.1.0"
+authors = ["Philipp Oppermann <dev@phil-opp.com>"]
+edition = "2021"
+
+[dependencies]
+bootloader_api = { path = "../../../api" }
+x86_64 = { version = "0.14.7", default-features = false, features = [
+    "instructions",
+    "inline_asm",
+] }
+uart_16550 = "0.2.10"

tests/test_kernels/min_stack/Cargo.toml

@@ -0,0 +1,26 @@
+#![no_std] // don't link the Rust standard library
+#![no_main] // disable all Rust-level entry points
+
+use bootloader_api::{entry_point, BootInfo, BootloaderConfig};
+use core::fmt::Write;
+use test_kernel_min_stack::{exit_qemu, serial, QemuExitCode};
+
+const BOOTLOADER_CONFIG: BootloaderConfig = {
+    let mut config = BootloaderConfig::new_default();
+    config.kernel_stack_size = 3000;
+    config
+};
+entry_point!(kernel_main, config = &BOOTLOADER_CONFIG);
+
+fn kernel_main(boot_info: &'static mut BootInfo) -> ! {
+    writeln!(serial(), "Entered kernel with boot info: {boot_info:?}").unwrap();
+    exit_qemu(QemuExitCode::Success);
+}
+
+/// This function is called on panic.
+#[panic_handler]
+#[cfg(not(test))]
+fn panic(info: &core::panic::PanicInfo) -> ! {
+    let _ = writeln!(serial(), "PANIC: {info}");
+    exit_qemu(QemuExitCode::Failed);
+}

tests/test_kernels/min_stack/src/bin/basic_boot.rs

@@ -0,0 +1,27 @@
+#![no_std]
+
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+#[repr(u32)]
+pub enum QemuExitCode {
+    Success = 0x10,
+    Failed = 0x11,
+}
+
+pub fn exit_qemu(exit_code: QemuExitCode) -> ! {
+    use x86_64::instructions::{nop, port::Port};
+
+    unsafe {
+        let mut port = Port::new(0xf4);
+        port.write(exit_code as u32);
+    }
+
+    loop {
+        nop();
+    }
+}
+
+pub fn serial() -> uart_16550::SerialPort {
+    let mut port = unsafe { uart_16550::SerialPort::new(0x3F8) };
+    port.init();
+    port
+}