Skip to content

[Bug] Assertion failed when parsing EFI Multiboot memory region header #215

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

Closed
junyang-zh opened this issue May 22, 2024 · 4 comments · Fixed by #216
Closed

[Bug] Assertion failed when parsing EFI Multiboot memory region header #215

junyang-zh opened this issue May 22, 2024 · 4 comments · Fixed by #216

Comments

@junyang-zh
Copy link

junyang-zh commented May 22, 2024

Known versions

The bug can be reproduced with version 0.19.0 and 0.20.0. While 0.18.1 is functioning.

Bug behavior

Panicked with

panicked at .cargo/registry/src/index.crates.io-6f17d22bba15001f/multiboot2-0.19.0/src/memory_map.rs:335:9:
assertion `left == right` failed
  left: 24
 right: 0

at

impl TagTrait for EFIMemoryMapTag {
const ID: TagType = TagType::EfiMmap;
fn dst_size(base_tag: &Tag) -> usize {
assert!(base_tag.size as usize >= EFI_METADATA_SIZE);
let size = base_tag.size as usize - EFI_METADATA_SIZE;
assert_eq!(size % mem::size_of::<EFIMemoryDesc>(), 0);
size / mem::size_of::<EFIMemoryDesc>()
}
}

Context

I used let mb2_info = BootInformation::load(addr) to load the header, and try to debug print it with "{:#?}", mb2_info.

The following information is printed:

Multiboot2BootInformation {
    start_address: 589824,
    end_address: 599200,
    total_size: 9376,
    basic_memory_info: Some(
        BasicMemoryInfoTag {
            typ: BasicMeminfo,
            size: 16,
            memory_lower: 640,
            memory_upper: 7168,
        },
    ),
    boot_loader_name: Some(
        BootLoaderNameTag {
            typ: BootLoaderName,
            size: 18,
            name: Ok(
                "GRUB 2.12",
            ),
        },
    ),
    command_line: Some(
        CommandLineTag {
            typ: Cmdline,
            size: 104,
            cmdline: Ok(
                "SHELL=/bin/sh LOGNAME=root HOME=/ USER=root PATH=/bin:/benchmark init=/usr/bin/busybox -- sh -l",
            ),
        },
    ),
    efi_bs_not_exited: None,

And it stops printing with a panic here.

Environment

  • OVMF built with EDK2 version edk2-stable202402 ;
  • QEMU built from source version 8.2.1.

I don't have time build a minimal reproduction demo. But one can do it under our kernel project https://github.com/asterinas/asterinas. There's a development image for the exact environment https://hub.docker.com/r/asterinas/asterinas.

Plans to fix it

We may use 0.18.1 for our project currently. I am happy to provide assistance for anyone taking it. I can also take it but I need guidance.

@phip1611
Copy link
Member

phip1611 commented May 22, 2024

Yep, my fault. Sorry! UEFI doesn't want you to ever rely on size_of::<MemoryDescriptor> - which I did.

In the spec, this is not really that clear, but in the implementation, it is. https://github.com/tianocore/edk2/blob/7142e648416ff5d3eac6c6d607874805f5de0ca8/MdeModulePkg/Core/PiSmmCore/Page.c#L1059

@phip1611
Copy link
Member

@junyang-zh Can you please check if #216 fixes the problems for you?

@junyang-zh
Copy link
Author

@junyang-zh Can you please check if #216 fixes the problems for you?

Tried! #216 is fully functioning in our specific use case. Thanks very much for fixing it such quickly.

@phip1611
Copy link
Member

Released as [email protected] at crates.io. I hope it works smoothly now - nice project you're working on!

junyang-zh added a commit to junyang-zh/asterinas that referenced this issue Jun 19, 2024
This fix relates to an upstream bug <rust-osdev/multiboot2#215>.
junyang-zh added a commit to junyang-zh/asterinas that referenced this issue Jun 20, 2024
junyang-zh added a commit to junyang-zh/asterinas that referenced this issue Jun 20, 2024
junyang-zh added a commit to junyang-zh/asterinas that referenced this issue Jun 20, 2024
junyang-zh added a commit to junyang-zh/asterinas that referenced this issue Jun 20, 2024
junyang-zh added a commit to junyang-zh/asterinas that referenced this issue Jun 20, 2024
junyang-zh added a commit to junyang-zh/asterinas that referenced this issue Jun 20, 2024
junyang-zh added a commit to junyang-zh/asterinas that referenced this issue Jun 20, 2024
junyang-zh added a commit to junyang-zh/asterinas that referenced this issue Jun 21, 2024
tatetian pushed a commit to asterinas/asterinas that referenced this issue Jun 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants