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

uefi: Remove mentions of runtime usage from GOP docs #613

Merged

Conversation

nicholasbishop
Copy link
Contributor

As pointed out in #612, our GOP docs said the protocol can be used both during boot services and during runtime. As far as I can tell this is incorrect; as with all other protocols it can only be used during boot services. (Of course, a firmware implementation might leave the memory initialized, giving the appearance that GOP is usable after exiting boot services, but that would essentially be a UAF bug.)

For some supporting evidence, see this presentation: https://uefi.org/sites/default/files/resources/UPFS11_P4_UEFI_GOP_AMD.pdf

Major differences between GOP driver vs. legacy VGA BIOS:
...
2. Boot only services vs. both boot and OS run-time services

Checklist

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

@phip1611
Copy link
Contributor

phip1611 commented Dec 20, 2022

Yeah, only the framebuffer can be used at runtime. Good change!

@@ -1,8 +1,6 @@
//! Graphics output protocol.
//!
//! The UEFI GOP is meant to replace existing [VGA][vga] hardware interfaces.
//! It can be used in the boot environment as well as at runtime,
//! until a high-performance driver is loaded by the OS.
//!
//! The GOP provides access to a hardware frame buffer and allows UEFI apps
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to add sth like: "Although the protocol itself requires the boot services, once the framebuffer is initialized, it can also be used after the boot services have been exited." What do you think?

Copy link
Contributor Author

@nicholasbishop nicholasbishop Dec 20, 2022

Choose a reason for hiding this comment

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

I'm open to adding such language, but I want to make sure we have a clear citation for it. From Googling I see that sometimes this claim is made, but from my reading of sections 12.9 and 12.10 (in UEFI 2.10) I haven't found clear evidence that this is allowed. The GOP intro has this to say (emphasis mine):

Graphics output may also be required as part of the startup of an operating system. There are potentially times in modern operating systems prior to the loading of a high performance OS graphics driver where access to graphics output device is required. The Graphics Output Protocol supports this capability by providing the EFI OS loader access to a hardware frame buffer and enough information to allow the OS to draw directly to the graphics output device.

But it doesn't say whether this buffer can be used after exiting boot services. It's not required that the OS loader itself exits boot services, that can be deferred to the OS (and indeed that is how Linux is usually loaded).

The documentation for FrameBufferBase (the address of the buffer) says:

Base address of graphics linear frame buffer. Info contains information required to allow software
to draw directly to the frame buffer without using Blt().Offset zero in FrameBufferBase represents the upper left pixel of the display.

Again, no indication that we're allowed to keep a hold of that handle after closing the protocol or exiting boot services.

Under "Description" for EFI_GRAPHICS_OUTPUT_PROTOCOL it says (again, emphasis mine):

The EFI_GRAPHICS_OUTPUT_PROTOCOL provides a software abstraction to allow pixels to be drawn directly to the frame buffer. The EFI_GRAPHICS_OUTPUT_PROTOCOL is designed to be lightweight and to support the basic needs of graphics output prior to Operating System boot.

The UEFI Specification can be rather enigmatic, so in the end I'm really not sure what the right answer is. But unless we can find a clear and unambiguous place in the spec (or some other official document) I don't think we should make that claim in our documentation.

Copy link
Contributor

@phip1611 phip1611 Dec 20, 2022

Choose a reason for hiding this comment

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

The bootloader crate also handles it like this. It creates a GOP frame buffer, exits the boot services, and provides the frame buffer to OSs. I assume that the bootloader crate was already tested on a variety of different platforms/UEFI implementations. Does @phil-opp has any insights?

Copy link
Contributor

@phip1611 phip1611 Dec 20, 2022

Choose a reason for hiding this comment

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

However, I totally agree that we need some specification reference for that..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you OK with merging this as-is? Until we have unambiguous confirmation that the buffer pointer is safe to use during runtime, I think it's better to be conservative in the docs to avoid misleading into potentially unsafe usage.

Copy link
Contributor

@phip1611 phip1611 Feb 6, 2023

Choose a reason for hiding this comment

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

We could write a test that verifies if the memory map contains the frame buffer's memory, after the boot services were excited. Also, Multiboot2 allows to get a Framebuffer when Boot services are exited. These are no strong specs, but hints.

I asked a few colleagues if they have any idea where this could be specified. I come back to you shortly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could write a test that verifies if the memory map contains the frame buffer's memory, after the boot services were excited.

That would give us information about a particular UEFI implementation, but wouldn't necessarily mean it works in general.

I pushed an update to add this to the docstring for Framebuffer::as_mut_ptr():
"On some implementations this framebuffer pointer can be used after exiting boot services, but that is not guaranteed by the UEFI Specification."

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for the late reply, I'm quite busy in the past weeks.

"On some implementations this framebuffer pointer can be used after exiting boot services, but that is not guaranteed by the UEFI Specification."

Perfectly fine with me. Feel free to merge once you added the sentence.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the late reply, I'm quite busy in the past weeks.

No worries, thanks for the review :)

As pointed out in rust-osdev#612, our
GOP docs said the protocol can be used both during boot services and
during runtime. As far as I can tell this is incorrect; as with all
other protocols it can only be used during boot services. (Of course, a
firmware implementation might leave the memory initialized, giving the
appearance that GOP is usable after exiting boot services, but that
would essentially be a UAF bug.)

For some supporting evidence, see this presentation:
https://uefi.org/sites/default/files/resources/UPFS11_P4_UEFI_GOP_AMD.pdf

On some implementations it has been observed that the framebuffer pointer can
still be used after exiting boot services, but that isn't guaranteed by the
spec. The Framebuffer::as_mut_ptr() method now mentions this.

> Major differences between GOP driver vs. legacy VGA BIOS:
> ...
> 2. Boot only services vs. both boot and OS run-time services
@nicholasbishop nicholasbishop merged commit ba3343b into rust-osdev:main Feb 11, 2023
@nicholasbishop nicholasbishop deleted the bishop-no-gop-runtime branch February 11, 2023 18:40
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.

2 participants