From 4b80d28d7187682725358ed45f0454d50f810d14 Mon Sep 17 00:00:00 2001 From: Philipp Oppermann Date: Fri, 16 Feb 2024 15:47:35 +0100 Subject: [PATCH 1/3] Fix invalid mapping to zero page caused by off-by-one bug The `zero_end` bound is exclusive, but we treat the `end_page` as inclusive. So when `zero_end` is page-aligned, we allocate one additional bss page. If this page was already mapped to some other segment, we remap it to a page with random content. This is the same bug as https://github.com/rust-osdev/bootloader/pull/362. --- src/page_table.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/page_table.rs b/src/page_table.rs index fce0bd22..5daca1d5 100644 --- a/src/page_table.rs +++ b/src/page_table.rs @@ -187,7 +187,7 @@ pub(crate) fn map_segment( zero_start.as_u64(), Size4KiB::SIZE, ))); - let end_page = Page::containing_address(zero_end); + let end_page = Page::containing_address(zero_end - 1usize); for page in Page::range_inclusive(start_page, end_page) { let frame = frame_allocator .allocate_frame(MemoryRegionType::Kernel) From 265314f5582937422e0b226dfd799fa6dafb85a6 Mon Sep 17 00:00:00 2001 From: Philipp Oppermann Date: Fri, 16 Feb 2024 15:50:45 +0100 Subject: [PATCH 2/3] Panic again when segements overlap This seems like a good indicator for bugs in the mapping code. This reverts PR #423 and commit https://github.com/rust-osdev/bootloader/pull/422/commits/f317b0de5f8b3a5d8719e7d30433aa0b454dd4a8. --- src/page_table.rs | 29 ++++++++--------------------- 1 file changed, 8 insertions(+), 21 deletions(-) diff --git a/src/page_table.rs b/src/page_table.rs index 5daca1d5..35880725 100644 --- a/src/page_table.rs +++ b/src/page_table.rs @@ -97,27 +97,14 @@ pub(crate) fn map_segment( for frame in PhysFrame::range_inclusive(start_frame, end_frame) { let offset = frame - start_frame; let page = start_page + offset; - match unsafe { - map_page(page, frame, page_table_flags, page_table, frame_allocator) - } { - Ok(flusher) => flusher.flush(), - Err(MapToError::PageAlreadyMapped(to)) if to == frame => { - let flags = match page_table.translate(page.start_address()) { - TranslateResult::Mapped { flags, .. } => flags, - _ => unreachable!(), - }; - if flags != page_table_flags { - unsafe { - page_table - .update_flags(page, flags | page_table_flags) - .unwrap() - .flush() - }; - } - // nothing to do, page is already mapped to the correct frame - } - Err(err) => return Err(err), - } + unsafe { map_page(page, frame, page_table_flags, page_table, frame_allocator) } + .unwrap_or_else(|err| { + panic!( + "failed to map segment starting at {:?}: failed to map page {:?} to frame {:?}: {:?}", + start_page, page, frame, err + ) + }) + .flush(); } if mem_size > file_size { From 14e926e96b39294b65d62c01d05fabb4f9e1fe7b Mon Sep 17 00:00:00 2001 From: Philipp Oppermann Date: Fri, 16 Feb 2024 15:51:55 +0100 Subject: [PATCH 3/3] Update changelog for #424 --- Changelog.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Changelog.md b/Changelog.md index dc2e07bb..2f2ed992 100644 --- a/Changelog.md +++ b/Changelog.md @@ -1,6 +1,6 @@ # Unreleased -- [Fix: unify flags if multiple segments are mapped to same frame with different flags](https://github.com/rust-osdev/bootloader/pull/423) +- [Fix invalid mapping to zero page caused by off-by-one bug](https://github.com/rust-osdev/bootloader/pull/424) # 0.9.26 – 2024-02-16