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

Let's discuss UEFI's pointer conventions #40

Closed
HadrienG2 opened this issue Sep 24, 2018 · 7 comments
Closed

Let's discuss UEFI's pointer conventions #40

HadrienG2 opened this issue Sep 24, 2018 · 7 comments
Labels

Comments

@HadrienG2
Copy link
Contributor

HadrienG2 commented Sep 24, 2018

The "Calling conventions" section of the the "Overview" chapter of the UEFI spec starts with an interesting read about what can be expected of UEFI when passing pointers to the API, which may or may not influence our FFI interface (the "extern" functions that we use internally), unsafe APIs (those that ingest pointers), and what we consider to be safe APIs.

This is from page 20 of UEFI spec 2.7A, with numbering added to ease discussion:

When passing pointer arguments to Boot Services, Runtime Services, and Protocol Interfaces, the
caller has the following responsibilities:

  1. It is the caller’s responsibility to pass pointer parameters that reference physical memory
    locations. If a pointer is passed that does not point to a physical memory location (i.e., a
    memory mapped I/O region), the results are unpredictable and the system may halt.
  2. It is the caller’s responsibility to pass pointer parameters with correct alignment. If an
    unaligned pointer is passed to a function, the results are unpredictable and the system may
    halt.
  3. It is the caller’s responsibility to not pass in a NULL parameter to a function unless it is explicitly
    allowed. If a NULL pointer is passed to a function, the results are unpredictable and the system
    may hang.
  4. Unless otherwise stated, a caller should not make any assumptions regarding the state of
    pointer parameters if the function returns with an error.

Requirement 2 (correct alignment) is in line with Unsafe Rust's normal expectations: unaligned pointers are very special in Rust, and may only be used when one has special permission to do so (here, the answer is "never in the public API"). So we don't need to do anything about it.

Requirement 3 (no NULLs unless given permission) is also common in Unsafe Rust. If we wanted to encode the non-nullness requirement in the type system, we could use NonNulls (which are repr(transparent) and therefore ABI-compatible with C pointers), but a lot of unsafe Rust APIs don't bother and we probably don't need to either.

Requirement 1 (only physical memory, no MMIO and such) is where things become more interesting. The concern is very specific to low-level code, and this is not a normal expectation of an unsafe Rust API. It is also prohibitively expensive to check in software (you basically need to take a memory map and walk through it). Therefore, it could be a contract which we want to expose in the FFI, or in unsafe APIs that consume pointers like memmove/memset. Since every UEFI entry point is concerned by this contract, a way to move it into the API would be to take inspiration from NonNull and build our own wrapper type (e.g. "EfiPtr") which encodes this requirement.

Finally, requirement 4 (EFI may freely garble pointer parameters on error) is I think the most interesting from the perspective of Rust's safety guarantees. A pessimistic interpretation of this sentence would mean that every EFI function is unsafe with an unknown contract, since an out-of-bounds pointer access can corrupt everything, and so the only thing to do on error would be to abort immediately. Not something very pleasant to build upon. But if we disregard the spec's advice and make the IMO reasonable assumption that all invalid pointer accesses during erronerous execution will be in bounds, then it becomes something that we can build safe APIs upon. The only thing we need to take care of is that any function which takes as input a mutable pointer parameter to a type which has safety invariants must be kept unsafe with a "may corrupt input on error" contract.

I'll try to see if I can encode these requirements in the unsafe APIs and check the safe APIs against them somehow.

@GabrielMajeri
Copy link
Collaborator

Req. 1: is it even possible to allocate a structure in memory mapped space? I'm pretty sure even regular unsafe Rust code would fail to work in such cases.
I don't think there's anything we can do to guard against this, any caller giving us references into non-physical memory is looking for trouble.

Req. 2 & Req. 3: we can get around this by simply never exposing raw pointers in safe functions. All functions which take non-null pointers use references, and the ones which do allow null pointers take in an Option<&T>. It's the callers responsability for the reference to be valid and aligned.

Req. 4: the way I understand this line, it doesn't talk about the state of the "parameters pointed by a pointer", but rather the pointer parameters.

In other words, if UEFI takes a reference to a Handle, we pass in a &mut Handle, there's no guarantee UEFI will fill the handle with null_ptr on error.

@HadrienG2
Copy link
Contributor Author

HadrienG2 commented Sep 24, 2018

Req. 1: is it even possible to allocate a structure in memory mapped space? I'm pretty sure even regular unsafe Rust code would fail to work in such cases.

As a matter of fact, we do already expose a memory-mapped device: the GOP frame buffer. And indeed, come to think of it, our current testing code probably uses it incorrectly, as volatile writes should be used to avoid optimization problems. Instead of a slice, we should probably expose a slice wrapper which enforces volatile write semantics in order to avoid that kind of issue in the future.

I don't think the spec dictates that firmware data structures cannot be in memory-mapped memory (which could make sense for things like the ACPI tables). That could be another source of hidden memory-mapped IO, but likely a harmless one: since the memory mapping must be transparent to the application, they can't do "dangerous" MMIO stuff like changing data structures under our feet. Therefore, only a very anal UEFI implementation could reject passing that as a parameter to a function (should we ever need to).

Not sure if UEFI exposes any memory-mapped I/O other than the GOP frame buffer and possibly some firmware data structures. I do intend to ultimately check the 2400 pages of spec in order to make uefi-rs achieve full API coverage, but that will take me a while, and right now I do not feel confidence to make a blanket statement about what all UEFI interfaces do and do not. 😄

Req. 2 & Req. 3: we can get around this by simply never exposing raw pointers in safe functions. All functions which take non-null pointers use references, and the ones which do allow null pointers take in an Option<&T>. It's the callers responsability for the reference to be valid and aligned.

As I said, I think we can mostly ignore these requirements since they match normal expectations of unsafe Rust code. Requirements 1 and 4 are the only ones that really get me concerned.

Req. 4: the way I understand this line, it doesn't talk about the state of the "parameters pointed by a pointer", but rather the pointer parameters.

In other words, if UEFI takes a reference to a Handle, we pass in a &mut Handle, there's no guarantee UEFI will fill the handle with null_ptr on error.

I'm not sure if I fully understand the nuance. I suspect that we might be saying the same thing with different words. Can you provide an example of a situation where our two understandings of this requirement differ?

My understanding is that if a UEFI function that takes an *mut T as an input fails, the data of type T behind that pointer should be considered corrupted, and any safety invariants of type T would have to be manually restored by a safe interface.

So for example, if we send in an *mut NotNull<T> to a faillible UEFI function that is exposed via a safe interface, then we should backup the original value of the NotNull pointer before calling UEFI and restore it if the UEFI function fails (since for all we know UEFI could have replaced it with a null pointer or something equally invalid).

@GabrielMajeri
Copy link
Collaborator

GabrielMajeri commented Sep 24, 2018

our current testing code probably uses it incorrectly, as volatile writes should be used to avoid optimization problems

It's debatable what the correct usage is, since the spec doesn't mention if this memory is write-combined or something.

if we send in an *mut NotNull to a faillible UEFI function that is exposed via a safe interface

Well, my point is that we should not use pointers in the safe API.

For example, look at the memory_map function: it is perfectly safe. It takes in a reference to a mutable buffer. We never specify what happens to that memory.

  • If the function fails, then the buffer can get corrupted, but it's OK, since it's just an array of u8s.
  • If the function succeeds, then we know the buffer contains valid data, and return an iterator which returns references to the memory descriptors.

At no point do we take a mutable reference to a T and risk corrupting it. All the functions in the crate take in parameters by value or by const reference (so we cannot fill them with invalid data), and we only return them when the function succeeds.

@HadrienG2
Copy link
Contributor Author

HadrienG2 commented Sep 24, 2018

It's debatable what the correct usage is, since the spec doesn't mention if this memory is write-combined or something.

It's true that the proper usage pattern on the hardware side is unclear. However, the current usage is provably incorrect at the language level. The Rust compiler is within its right to optimize out any non-volatile write to memory, especially if it does not get subsequently read by the program, since it works under the assumption that the program is the only entity that will ever read from memory. This is what I would like to address by providing a higher-level abstraction on top of the GOP framebuffer which enforces volatile writes.

I agree with you that the memory_map function has a very good way to deal with UEFI's limited data consistency guarantees. Here, I'm looking into ways to hint faillible uefi-rs developers like myself into remembering that this problem exists and must be dealt with (using designs like the one from memory_map) whenever a new API function is being interfaced.

@HadrienG2
Copy link
Contributor Author

These requirements cannot be cleanly encoded in the type system without something akin to the efiapi macro discussed in #41, which is itself unfeasible within Rust's current macro system. I'll try to document this kind of considerations in the contribution guide instead.

@GabrielMajeri
Copy link
Collaborator

@HadrienG2 We should try to make this lib as safe-ish as possible, but not any safer :)

Considering the huge amount of ways low-level developers can shoot themselves in the foot, it's doubtful we can truly provide memory safety in all cases without limiting the usage of the UEFI API.
Right now the API is based on a "don't do anything dubious and nothing bad should happen" principle.

If people find uefi-rs succesful in practice, then that's good enough for me.

@HadrienG2
Copy link
Contributor Author

HadrienG2 commented Oct 14, 2018

I'm trying to reach the level of safety that is normally guaranteed by Rust: APIs should be safe to use as long as...

  1. User triggers no undefined behavior
  2. The safety contracts of unsafe functions are upheld
  3. There is no bug in the implementation

This means that some UEFI APIs cannot be exposed in a safe form. But that's okay, we can just expose them as unsafe APIs with well-documented contracts :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants