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

loaded_image: make load options API safer #375

Merged

Conversation

nicholasbishop
Copy link
Member

The spec describes LoadOptions as "A pointer to the image’s binary
load options. See the OptionalData parameter in [section 3.1.3 Load
Options] of the Boot Manager chapter for information on the source of
the LoadOptions data." The OptionalData field is described as "The
remaining bytes in the load option descriptor are a binary data buffer
that is passed to the loaded image."

So there's no guarantee made about the contents of LoadOptions; it's
arbitrary binary data as far as the UEFI spec is concerned. So it's not
necessarily safe to treat the load_options as a Char16 pointer, since
it might not be aligned and might not be null-terminated. That said, the
data is usually a null-terminated UCS-2 string. The UEFI Shell spec
specifies that command-line parameters are passed that way.

To safely support both cases, change the load_options field to a u8
pointer, drop the load_options method that converts the data to a
&str, and add two new methods: load_options_as_bytes and
load_options_as_cstr16. The set_load_options has also been changed
to take a u8 pointer instead of a Char16 pointer.

#73

@nicholasbishop nicholasbishop force-pushed the bishop-clean-up-load-options branch 2 times, most recently from 0b3f481 to a5d29a1 Compare February 27, 2022 20:20
The spec describes `LoadOptions` as "A pointer to the image’s binary
load options. See the OptionalData parameter in [section 3.1.3 Load
Options] of the Boot Manager chapter for information on the source of
the LoadOptions data." The `OptionalData` field is described as "The
remaining bytes in the load option descriptor are a binary data buffer
that is passed to the loaded image."

So there's no guarantee made about the contents of `LoadOptions`; it's
arbitrary binary data as far as the UEFI spec is concerned. So it's not
necessarily safe to treat the load_options as a `Char16` pointer, since
it might not be aligned and might not be null-terminated. That said, the
data is *usually* a null-terminated UCS-2 string. The UEFI Shell spec
specifies that command-line parameters are passed that way.

To safely support both cases, change the `load_options` field to a `u8`
pointer, drop the `load_options` method that converts the data to a
`&str`, and add two new methods: `load_options_as_bytes` and
`load_options_as_cstr16`. The `set_load_options` has also been changed
to take a `u8` pointer instead of a `Char16` pointer.

rust-osdev#73
@nicholasbishop nicholasbishop force-pushed the bishop-clean-up-load-options branch from a5d29a1 to bfe391b Compare February 27, 2022 20:23
@GabrielMajeri
Copy link
Collaborator

Good catch, I didn't realize the UEFI spec allows for non-string extra data

@GabrielMajeri GabrielMajeri merged commit 8c35ae6 into rust-osdev:main Feb 27, 2022
@nicholasbishop nicholasbishop deleted the bishop-clean-up-load-options branch March 13, 2022 18:07
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