loaded_image: make load options API safer

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

CHANGELOG.md

@@ -9,17 +9,24 @@
 - Added `Display` impl for `CString16`.
 - Added `Handle::from_ptr` and `SystemTable<View>::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
 

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::<u16>()) != 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;
     }

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!(