Skip to content

Conversation

nicholasbishop
Copy link
Member

@nicholasbishop nicholasbishop commented Feb 5, 2023

Exiting boot services is a one-way operation, which is why exit_boot_services
takes self rather than a reference. If it succeeds then you get back a
SystemTable<Runtime>, but even if it fails you can't continue to use boot
services except for a very limited subset. Specifically, in UEFI 2.8 and
earlier, only get_memory_map and exit_boot_services are allowed. Starting in
UEFI 2.9 other memory allocation functions may also be called.

To properly support pre-2.9 firmware then, the right thing to do is to
make sure that the initial buffer allocated to hold the memory map has
sufficient extra room in case the first attempt to get the memory map
and exit boot services fails. We now add room for 8 extra entries if
that occurs, matching the behavior of the Linux kernel:
https://github.com/torvalds/linux/blob/e544a07438/drivers/firmware/efi/libstub/efistub.h#L173

If something unexpected happens and exiting boot services still fails
after two attempts, the system is in an undefined state that we can't
reasonably recover from. The best thing we can do then is to simply
reset the system, essentially a strong panic.

Closes #523

Checklist

  • Sensible git history (for example, squash "typo" or "fix" commits). See the Rewriting History guide for help.
  • Update the changelog (if necessary)

@nicholasbishop nicholasbishop force-pushed the bishop-exit-bs-1 branch 2 times, most recently from f94bf86 to 4f18d63 Compare February 5, 2023 20:59
@phil-opp
Copy link
Member

phil-opp commented Feb 6, 2023

Instead of increasing the extra_entries for each allocation, it might be possible to query the memory map size again just before the free_pool call. We're using this approach in the bootloader crate and it seems to work fine: https://github.com/rust-osdev/bootloader/blob/ca98936b24e21ff8af3d94d8fbf6fc21f48a1c18/uefi/src/main.rs#L153-L159. I'm not sure which approach is better, I just wanted to give another data point.

for _ in 0..max_attempts {
let memory_map_size = self.boot_services().memory_map_size();

// Get a buffer size that should be able to hold the memory
Copy link
Member

Choose a reason for hiding this comment

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

Which kind of extra allocations do we try to work-around here?
I think we should instead try to have a fine-grained control over which allocations may happen in this specific code path

Copy link
Member Author

@nicholasbishop nicholasbishop Feb 7, 2023

Choose a reason for hiding this comment

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

I don't think it's possible precisely control which allocations occur here, because a higher-priority task could be executed via timer, which could create additional allocations. This table shows restrictions on what priority level various functions can be called at. ExitBootServices must be called at the application priority level, which is the lowest level. So we can't change what TPL this code is running at.

We also don't control the implementation of allocate_pool. Intuitively I wouldn't expect that allocation to increase the number of memory map entries by more than one, but I don't think that's is guaranteed by the spec.

In general I would expect this loop to succeed on the very first try, but there's a small chance it won't.

Copy link
Member

Choose a reason for hiding this comment

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

I don't know if it's valid, but we can disable interrupts for a short period of time.. For the scope of this function. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure if disabling interrupts is valid -- it's not explicitly mentioned anywhere in the spec that I have seen.

I spent some time thinking more about the problem, and also looking into how it's handled in the Linux kernel since that provides some better proof of how stuff works on real firmware out in the wild, not just OVMF in QEMU.

One interesting thing I learned is that the spec for exit boot services changed slightly starting in UEFI 2.9. In UEFI 2.8 and earlier, only get_memory_map and exit_boot_services may be called after the first call to exit_boot_services. Starting in UEFI 2.9, other memory allocation functions may also be called.

The way the Linux kernel handles the memory allocation is to add space for 8 extra entries: https://github.com/torvalds/linux/blob/e544a07438/drivers/firmware/efi/libstub/efistub.h#L173. If exit boot services fails the first time, they try it only one more time: https://github.com/torvalds/linux/blob/e544a0743/drivers/firmware/efi/libstub/efi-stub-helper.c#L375. I've updated this PR to also use that constant and retry-once-behavior.

In the Linux kernel, if exit boot services fails after the second time, they end up calling boot_services.exit to return control to the firmware (or to another boot services program that launched the current one). I think technically that's against the spec since exit isn't one of the allowed boot services functions to call after exit_boot_services fails. So in this PR I've changed it to just reset() the system. I think that's probably the right behavior to have since the system is pretty much in an unknown and unrecoverable situation otherwise. And exiting boot services is considered a security boundary as well, so I think any other action would run the risk of introducing a secure boot bypass.

I think the most important thing is to make the implementation reasonably robust so that it doesn't fail in the first place. Exit boot services really isn't supposed to fail as long as you've retrieved the memory map properly, so all other failure handling is really an exceptional panic situation.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the investigation! 🤘

Exiting boot services is a one-way operation, which is why `exit_boot_services`
takes `self` rather than a reference. If it succeeds then you get back a
`SystemTable<Runtime>`, but even if it fails you can't continue to use boot
services except for a very limited subset. Specifically, in UEFI 2.8 and
earlier, only `get_memory_map` and `exit_boot_services` are allowed. Starting in
UEFI 2.9 other memory allocation functions may also be called.

To properly support pre-2.9 firmware then, the right thing to do is to
make sure that the initial buffer allocated to hold the memory map has
sufficient extra room in case the first attempt to get the memory map
and exit boot services fails. We now add room for 8 extra entries if
that occurs, matching the behavior of the Linux kernel:
https://github.com/torvalds/linux/blob/e544a07438/drivers/firmware/efi/libstub/efistub.h#L173

If something unexpected happens and exiting boot services still fails
after two attempts, the system is in an undefined state that we can't
reasonably recover from. The best thing we can do then is to simply
reset the system, essentially a strong panic.
@nicholasbishop nicholasbishop changed the title uefi: Make exit_boot_services handle memory map allocation uefi: Rework exit_boot_services API Feb 11, 2023
@nicholasbishop nicholasbishop merged commit dddbf51 into rust-osdev:main Feb 11, 2023
@nicholasbishop nicholasbishop deleted the bishop-exit-bs-1 branch February 11, 2023 22:05
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 this pull request may close these issues.

exit_boot_services doesn't handle scenario where memory map size increases
3 participants