Skip to content
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

Fix uefi_services memory unsafety if application exits before exiting boot services #943

Open
nicholasbishop opened this issue Sep 16, 2023 · 1 comment

Comments

@nicholasbishop
Copy link
Contributor

nicholasbishop commented Sep 16, 2023

As described in #917, there's a problem with the current uefi_services::init API. That function creates an event that will call a function when exiting boot services. That function handles shutting down the logger and allocator, since both rely on boot services. However, the contents of the function aren't really important here. The problem is that if an application calls uefi_services::init and then exits (whether by returning from main, panicking, or calling EFI_BOOT_SERVICES.Exit()), the event will still exist and will trigger a call to the handler function if EFI_BOOT_SERVICES.ExitBootServices() is called. But the handler function will no longer exist in memory, leading to undefined behavior.

There are a number of possible solutions, but nothing is clearly jumping out at me as the best solution, so I wanted to lay them out here for comment.

  1. uefi-services: Return event in init #920 changes uefi_services::init to return the Event that it creates. It's up to the caller to call close_event before exiting. This is a nice minimal change, and for a typical bootloader application that never really exits (it just passes control to a child application), it has the advantage of not requiring any code changes in the caller. However, it's error prone for other applications, and I think we'd have to mark uefi_services::init as unsafe since it makes it possible to trigger UB if the event isn't cleaned up by the caller.

  2. We could change uefi_services::init returns a guard object with a custom drop impl that takes care of cleanup before the application exits. Downside: guard might be dropped too early. We can put #[must_use] on it to warn the user, but then they might reach for let _ = uefi_services::init(), which will unfortunately drop the guard immediately (in contrast to let _guard = uefi_services::init(), which will not drop until the scope ends). See this playground for a demonstration of this subtle and annoying opportunity for bugs.

  3. Deprecate uefi_services::init and add a new uefi_services::run function that takes a closure. A typical main would then look like this:

    #[entry]
    fn main(image_handle: Handle, mut system_table: SystemTable<Boot>) -> Status {
        uefi_services::run(system_table, |system_table| {
            do_things_here();
    
            Status::SUCCESS
        })
    }

    Straightforward, but aesthetically I find it annoying to force an extra level of indent.

  4. Provide a new #[uefi_services::main] macro that takes care of calling uefi_services::init and makes sure the cleanup code is properly called. Solves the aesthetic problem, at the cost of some more macro magic.

  5. (Speculative) I haven't actually tried this, but I imagine it could be possible to allocate some executable memory and copy the event handler function code to it. Then the memory would still exist after exiting the application and the function could still be safely called. Any memory that function accessed would need to be in a specially-allocated place as well. This is the only option that requires no change in the public API, so it would be an entirely transparent fix from the caller's perspective. Maybe too much magic though, and I haven't actually tried it.

@nicholasbishop
Copy link
Contributor Author

CC @JohnAZoidberg

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

No branches or pull requests

1 participant