Skip to content

multiboot2: Fix EFI Memory Map 'last_area' calculation #119

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 9, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion multiboot2-header/src/builder/information_request.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ pub struct InformationRequestHeaderTagBuilder {
#[cfg(feature = "builder")]
impl InformationRequestHeaderTagBuilder {
/// New builder.
pub fn new(flag: HeaderTagFlag) -> Self {
pub const fn new(flag: HeaderTagFlag) -> Self {
Self {
irs: BTreeSet::new(),
flag,
Expand Down
3 changes: 1 addition & 2 deletions multiboot2-header/src/header.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,7 @@ impl<'a> Multiboot2Header<'a> {
assert_eq!(
reference.header_magic(),
MULTIBOOT2_HEADER_MAGIC,
"The Multiboot2 header must contain the MULTIBOOT2_HEADER_MAGIC={:x}",
MULTIBOOT2_HEADER_MAGIC
"The Multiboot2 header must contain the MULTIBOOT2_HEADER_MAGIC={MULTIBOOT2_HEADER_MAGIC:x}"
);
assert!(
reference.verify_checksum(),
Expand Down
14 changes: 12 additions & 2 deletions multiboot2/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1415,13 +1415,23 @@ mod tests {
assert!(efi_mmap.is_none());
}

#[test]
/// Compile time test for `MemoryMapTag`.
fn e820_memory_map_tag_size() {
use super::MemoryMapTag;
unsafe {
// `MemoryMapTag` is 16 bytes without the 1st entry
core::mem::transmute::<[u8; 16], MemoryMapTag>([0u8; 16]);
}
}

#[test]
/// Compile time test for `EFIMemoryMapTag`.
fn efi_memory_map_tag_size() {
use super::EFIMemoryMapTag;
unsafe {
// `EFIMemoryMapTag` is 16 bytes + `EFIMemoryDesc` is 40 bytes.
core::mem::transmute::<[u8; 56], EFIMemoryMapTag>([0u8; 56]);
// `EFIMemoryMapTag` is 16 bytes without the 1st entry
core::mem::transmute::<[u8; 16], EFIMemoryMapTag>([0u8; 16]);
}
}
}
17 changes: 11 additions & 6 deletions multiboot2/src/memory_map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ pub struct MemoryMapTag {
size: u32,
entry_size: u32,
entry_version: u32,
first_area: MemoryArea,
first_area: [MemoryArea; 0],
}

impl MemoryMapTag {
Expand All @@ -31,10 +31,13 @@ impl MemoryMapTag {
/// Return an iterator over all marked memory areas.
pub fn all_memory_areas(&self) -> impl Iterator<Item = &MemoryArea> {
let self_ptr = self as *const MemoryMapTag;
let start_area = (&self.first_area) as *const MemoryArea;
let start_area = self.first_area.as_ptr();

MemoryAreaIter {
current_area: start_area as u64,
last_area: (self_ptr as u64 + (self.size - self.entry_size) as u64),
// NOTE: `last_area` is only a bound, it doesn't necessarily point exactly to the last element
last_area: (self_ptr as u64
+ (self.size as u64 - core::mem::size_of::<MemoryMapTag>() as u64)),
entry_size: self.entry_size,
phantom: PhantomData,
}
Expand Down Expand Up @@ -127,7 +130,7 @@ pub struct EFIMemoryMapTag {
size: u32,
desc_size: u32,
desc_version: u32,
first_desc: EFIMemoryDesc,
first_desc: [EFIMemoryDesc; 0],
}

impl EFIMemoryMapTag {
Expand All @@ -137,10 +140,12 @@ impl EFIMemoryMapTag {
/// available memory areas for tables and such.
pub fn memory_areas(&self) -> EFIMemoryAreaIter {
let self_ptr = self as *const EFIMemoryMapTag;
let start_area = (&self.first_desc) as *const EFIMemoryDesc;
let start_area = self.first_desc.as_ptr();
EFIMemoryAreaIter {
current_area: start_area as u64,
last_area: (self_ptr as u64 + self.size as u64),
// NOTE: `last_area` is only a bound, it doesn't necessarily point exactly to the last element
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An oddly formed multiboot tag could have a combination of "tag size" and "EFI descriptor size" that represents a non-integer multiple of descriptors. Thus, I wanted to make it clear that "last_area" should only be used as a bound and not a direct pointer to the last descriptor.

last_area: (self_ptr as u64
+ (self.size as u64 - core::mem::size_of::<EFIMemoryMapTag>() as u64)),
entry_size: self.desc_size,
phantom: PhantomData,
}
Expand Down