From 0b3f48183d7ae694bdf0d2af66bd8b47f8fb0943 Mon Sep 17 00:00:00 2001 From: Nicholas Bishop Date: Sat, 26 Feb 2022 17:11:47 -0500 Subject: [PATCH] loaded_image: make load options API safer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. https://github.com/rust-osdev/uefi-rs/issues/73 --- CHANGELOG.md | 7 ++ src/proto/loaded_image.rs | 78 ++++++++++++++++------ uefi-test-runner/src/proto/loaded_image.rs | 7 +- 3 files changed, 67 insertions(+), 25 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7ffc81aa3..4c7be6686 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,17 +9,24 @@ - Added `Display` impl for `CString16`. - Added `Handle::from_ptr` and `SystemTable::from_ptr`, which are `unsafe` methods for initializing from a raw pointer. +- Added `LoadedImage::load_options_as_bytes` and + `LoadedImage::load_options_as_cstr16`. ### Changed - `File::open` now takes the filename as `&CStr16` instead of `&str`, avoiding an implicit string conversion. +- `LoadImage::set_load_options` now takes a `u8` pointer instead of + `Char16`. ### Removed - Removed `CStr16::as_string` method. Use [`ToString`](https://doc.rust-lang.org/alloc/string/trait.ToString.html) instead. +- Removed `LoadedImage::load_options`, use + `LoadedImage::load_options_as_bytes` or + `LoadedImage::load_options_as_cstr16` instead. ### Fixed diff --git a/src/proto/loaded_image.rs b/src/proto/loaded_image.rs index ed7aabb18..6adf04d3c 100644 --- a/src/proto/loaded_image.rs +++ b/src/proto/loaded_image.rs @@ -1,12 +1,11 @@ //! `LoadedImage` protocol. use crate::{ - data_types::{CStr16, Char16}, - proto::Protocol, - table::boot::MemoryType, - unsafe_guid, Handle, Status, + data_types::FromSliceWithNulError, proto::Protocol, table::boot::MemoryType, unsafe_guid, + CStr16, Handle, Status, }; -use core::{ffi::c_void, str}; +use core::convert::TryFrom; +use core::{ffi::c_void, mem, slice}; /// The LoadedImage protocol. This can be opened on any image handle using the `HandleProtocol` boot service. #[repr(C)] @@ -24,7 +23,7 @@ pub struct LoadedImage { // Image load options load_options_size: u32, - load_options: *const Char16, + load_options: *const u8, // Location where image was loaded image_base: *const c_void, @@ -39,10 +38,14 @@ pub struct LoadedImage { /// Errors that can be raised during parsing of the load options. #[derive(Debug)] pub enum LoadOptionsError { - /// The passed buffer is not large enough to contain the load options. - BufferTooSmall, - /// The load options are not valid UTF-8. - NotValidUtf8, + /// Load options are not set. + NotSet, + + /// The start and/or length of the load options is not [`u16`]-aligned. + NotAligned, + + /// Not a valid null-terminated UCS-2 string. + InvalidString(FromSliceWithNulError), } impl LoadedImage { @@ -51,17 +54,52 @@ impl LoadedImage { self.device_handle } - /// Get the load options of the given image. If the image was executed from the EFI shell, or from a boot - /// option, this is the command line that was used to execute it as a string. If no options were given, this - /// returns `Ok("")`. - pub fn load_options<'a>(&self, buffer: &'a mut [u8]) -> Result<&'a str, LoadOptionsError> { + /// Get the load options of the image as a [`&CStr16`]. + /// + /// Load options are typically used to pass command-line options as + /// a null-terminated UCS-2 string. This format is not required + /// though; use [`load_options_as_bytes`] to access the raw bytes. + /// + /// [`&CStr16`]: `CStr16` + /// [`load_options_as_bytes`]: `Self::load_options_as_bytes` + pub fn load_options_as_cstr16(&self) -> Result<&CStr16, LoadOptionsError> { + if self.load_options.is_null() { + Err(LoadOptionsError::NotSet) + } else if (self.load_options_size % 2 != 0) + || (((self.load_options as usize) % mem::align_of::()) != 0) + { + Err(LoadOptionsError::NotAligned) + } else { + let s = unsafe { + slice::from_raw_parts( + self.load_options as *const u16, + usize::try_from(self.load_options_size / 2).unwrap(), + ) + }; + CStr16::from_u16_with_nul(s).map_err(LoadOptionsError::InvalidString) + } + } + + /// Get the load options of the image as raw bytes. + /// + /// UEFI allows arbitrary binary data in load options, but typically + /// the data is a null-terminated UCS-2 string. Use + /// [`load_options_as_cstr16`] to more conveniently access the load + /// options as a string. + /// + /// Returns `None` if load options are not set. + /// + /// [`load_options_as_cstr16`]: `Self::load_options_as_cstr16` + pub fn load_options_as_bytes(&self) -> Option<&[u8]> { if self.load_options.is_null() { - Ok("") + None } else { - let ucs2_slice = unsafe { CStr16::from_ptr(self.load_options).to_u16_slice() }; - let length = - ucs2::decode(ucs2_slice, buffer).map_err(|_| LoadOptionsError::BufferTooSmall)?; - str::from_utf8(&buffer[0..length]).map_err(|_| LoadOptionsError::NotValidUtf8) + unsafe { + Some(slice::from_raw_parts( + self.load_options, + usize::try_from(self.load_options_size).unwrap(), + )) + } } } @@ -104,7 +142,7 @@ impl LoadedImage { /// This function takes `options` as a raw pointer because the /// load options data is not owned by `LoadedImage`. The caller /// must ensure that the memory lives long enough. - pub unsafe fn set_load_options(&mut self, options: *const Char16, size: u32) { + pub unsafe fn set_load_options(&mut self, options: *const u8, size: u32) { self.load_options = options; self.load_options_size = size; } diff --git a/uefi-test-runner/src/proto/loaded_image.rs b/uefi-test-runner/src/proto/loaded_image.rs index 8335386e6..890bcbea2 100644 --- a/uefi-test-runner/src/proto/loaded_image.rs +++ b/uefi-test-runner/src/proto/loaded_image.rs @@ -17,11 +17,8 @@ pub fn test(image: Handle, bt: &BootServices) { .expect_success("Failed to open LoadedImage protocol"); let loaded_image = unsafe { &*loaded_image.interface.get() }; - let mut buffer = vec![0; 128]; - let load_options = loaded_image - .load_options(&mut buffer) - .expect("Failed to get load options"); - info!("LoadedImage options: \"{}\"", load_options); + let load_options = loaded_image.load_options_as_bytes(); + info!("LoadedImage options: {:?}", load_options); let (image_base, image_size) = loaded_image.info(); info!(