Merge pull request #115 from rust-osdev/multiboot2-str-safety

Multiboot2: More Safety for Getters of Strings from Tags (Prepare Release v0.14.0)

Author: phip1611

multiboot2/Cargo.toml

@@ -6,7 +6,7 @@ Multiboot2-compliant bootloaders, like GRUB. It supports all tags from the speci
 including full support for the sections of ELF-64. This library is `no_std` and can be
 used in a Multiboot2-kernel.
 """
-version = "0.13.3"
+version = "0.14.0"
 authors = [
     "Philipp Oppermann <dev@phil-opp.com>",
     "Calvin Lee <cyrus296@gmail.com>",

multiboot2/Changelog.md

@@ -1,6 +1,20 @@
 # CHANGELOG for crate `multiboot2`
 
-## 0.13.3 (2022-05-03)
+## 0.14.0 (2022-05-xx)
+- **BREAKING CHANGES** \
+  This version includes a few small breaking changes that brings more safety when parsing strings from the
+  multiboot information structure.
+  - `BootLoaderNameTag::name` now returns a Result instead of just the value
+  - `CommandLineTag::command_line` now returns a Result instead of just the value
+  - `ModuleTag::cmdline` now returns a Result instead of just the value
+  - `RsdpV1Tag::signature` now returns a Result instead of an Option
+  - `RsdpV1Tag::oem_id` now returns a Result instead of an Option
+  - `RsdpV2Tag::signature` now returns a Result instead of an Option
+  - `RsdpV2Tag::oem_id` now returns a Result instead of an Option
+- internal code improvements
+
+
+## 0.13.3 (2022-06-03)
 - impl `Send` for `BootInformation`
 
 ## 0.13.2 (2022-05-02)

multiboot2/src/boot_loader_name.rs

@@ -1,4 +1,5 @@
 use crate::TagType;
+use core::str::Utf8Error;
 
 /// This tag contains the name of the bootloader that is booting the kernel.
 ///
@@ -9,11 +10,14 @@ use crate::TagType;
 pub struct BootLoaderNameTag {
     typ: TagType,
     size: u32,
+    /// Null-terminated UTF-8 string
     string: u8,
 }
 
 impl BootLoaderNameTag {
     /// Read the name of the bootloader that is booting the kernel.
+    /// This is an null-terminated UTF-8 string. If this returns `Err` then perhaps the memory
+    /// is invalid or the bootloader doesn't follow the spec.
     ///
     /// # Examples
     ///
@@ -23,11 +27,49 @@ impl BootLoaderNameTag {
     ///     assert_eq!("GRUB 2.02~beta3-5", name);
     /// }
     /// ```
-    pub fn name(&self) -> &str {
+    pub fn name(&self) -> Result<&str, Utf8Error> {
         use core::{mem, slice, str};
-        unsafe {
-            let strlen = self.size as usize - mem::size_of::<BootLoaderNameTag>();
-            str::from_utf8_unchecked(slice::from_raw_parts((&self.string) as *const u8, strlen))
-        }
+        // strlen without null byte
+        let strlen = self.size as usize - mem::size_of::<BootLoaderNameTag>();
+        let bytes = unsafe { slice::from_raw_parts((&self.string) as *const u8, strlen) };
+        str::from_utf8(bytes)
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use crate::TagType;
+
+    const MSG: &str = "hello";
+
+    /// Returns the tag structure in bytes in native endian format.
+    fn get_bytes() -> std::vec::Vec<u8> {
+        // size is: 4 bytes for tag + 4 bytes for size + length of null-terminated string
+        let size = (4 + 4 + MSG.as_bytes().len() + 1) as u32;
+        [
+            &((TagType::BootLoaderName as u32).to_ne_bytes()),
+            &size.to_ne_bytes(),
+            MSG.as_bytes(),
+            // Null Byte
+            &[0],
+        ]
+        .iter()
+        .flat_map(|bytes| bytes.iter())
+        .copied()
+        .collect()
+    }
+
+    /// Tests to parse a string with a terminating null byte from the tag (as the spec defines).
+    #[test]
+    fn test_parse_str() {
+        let tag = get_bytes();
+        let tag = unsafe {
+            tag.as_ptr()
+                .cast::<super::BootLoaderNameTag>()
+                .as_ref()
+                .unwrap()
+        };
+        assert_eq!({ tag.typ }, TagType::BootLoaderName);
+        assert_eq!(tag.name().expect("must be valid UTF-8"), MSG);
     }
 }

multiboot2/src/command_line.rs

@@ -1,4 +1,9 @@
+//! Module for [CommandLineTag].
+
 use crate::TagType;
+use core::mem;
+use core::slice;
+use core::str;
 
 /// This tag contains the command line string.
 ///
@@ -9,11 +14,14 @@ use crate::TagType;
 pub struct CommandLineTag {
     typ: TagType,
     size: u32,
+    /// Null-terminated UTF-8 string
     string: u8,
 }
 
 impl CommandLineTag {
     /// Read the command line string that is being passed to the booting kernel.
+    /// This is an null-terminated UTF-8 string. If this returns `Err` then perhaps the memory
+    /// is invalid or the bootloader doesn't follow the spec.
     ///
     /// # Examples
     ///
@@ -23,11 +31,48 @@ impl CommandLineTag {
     ///     assert_eq!("/bootarg", command_line);
     /// }
     /// ```
-    pub fn command_line(&self) -> &str {
-        use core::{mem, slice, str};
-        unsafe {
-            let strlen = self.size as usize - mem::size_of::<CommandLineTag>();
-            str::from_utf8_unchecked(slice::from_raw_parts((&self.string) as *const u8, strlen))
-        }
+    pub fn command_line(&self) -> Result<&str, str::Utf8Error> {
+        // strlen without null byte
+        let strlen = self.size as usize - mem::size_of::<CommandLineTag>();
+        let bytes = unsafe { slice::from_raw_parts((&self.string) as *const u8, strlen) };
+        str::from_utf8(bytes)
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use crate::TagType;
+
+    const MSG: &str = "hello";
+
+    /// Returns the tag structure in bytes in native endian format.
+    fn get_bytes() -> std::vec::Vec<u8> {
+        // size is: 4 bytes for tag + 4 bytes for size + length of null-terminated string
+        let size = (4 + 4 + MSG.as_bytes().len() + 1) as u32;
+        [
+            &((TagType::Cmdline as u32).to_ne_bytes()),
+            &size.to_ne_bytes(),
+            MSG.as_bytes(),
+            // Null Byte
+            &[0],
+        ]
+        .iter()
+        .flat_map(|bytes| bytes.iter())
+        .copied()
+        .collect()
+    }
+
+    /// Tests to parse a string with a terminating null byte from the tag (as the spec defines).
+    #[test]
+    fn test_parse_str() {
+        let tag = get_bytes();
+        let tag = unsafe {
+            tag.as_ptr()
+                .cast::<super::CommandLineTag>()
+                .as_ref()
+                .unwrap()
+        };
+        assert_eq!({ tag.typ }, TagType::Cmdline);
+        assert_eq!(tag.command_line().expect("must be valid UTF-8"), MSG);
     }
 }

multiboot2/src/elf_sections.rs

@@ -190,6 +190,8 @@ impl ElfSection {
         use core::{slice, str};
 
         let name_ptr = unsafe { self.string_table().offset(self.get().name_index() as isize) };
+
+        // strlen without null byte
         let strlen = {
             let mut len = 0;
             while unsafe { *name_ptr.offset(len) } != 0 {

multiboot2/src/framebuffer.rs

@@ -3,7 +3,7 @@ use crate::Reader;
 use core::slice;
 
 /// The VBE Framebuffer information Tag.
-#[derive(Debug, PartialEq)]
+#[derive(Debug, PartialEq, Eq)]
 pub struct FramebufferTag<'a> {
     /// Contains framebuffer physical address.
     ///
@@ -29,7 +29,7 @@ pub struct FramebufferTag<'a> {
 }
 
 /// The type of framebuffer.
-#[derive(Debug, PartialEq)]
+#[derive(Debug, PartialEq, Eq)]
 pub enum FramebufferType<'a> {
     /// Indexed color.
     Indexed {
@@ -55,7 +55,7 @@ pub enum FramebufferType<'a> {
 }
 
 /// An RGB color type field.
-#[derive(Debug, PartialEq)]
+#[derive(Debug, PartialEq, Eq)]
 pub struct FramebufferField {
     /// Color field position.
     pub position: u8,
@@ -65,7 +65,7 @@ pub struct FramebufferField {
 }
 
 /// A framebuffer color descriptor in the palette.
-#[derive(Clone, Copy, Debug, PartialEq)]
+#[derive(Clone, Copy, Debug, PartialEq, Eq)]
 #[repr(C, packed)] // only repr(C) would add unwanted padding at the end
 pub struct FramebufferColor {
     /// The Red component of the color.

multiboot2/src/lib.rs

@@ -352,14 +352,14 @@ impl fmt::Debug for BootInformation {
                 "boot_loader_name_tag",
                 &self
                     .boot_loader_name_tag()
-                    .map(|x| x.name())
+                    .and_then(|x| x.name().ok())
                     .unwrap_or("<unknown>"),
             )
             .field(
                 "command_line",
                 &self
                     .command_line_tag()
-                    .map(|x| x.command_line())
+                    .and_then(|x| x.command_line().ok())
                     .unwrap_or(""),
             )
             .field("memory_areas", &self.memory_map_tag())
@@ -532,7 +532,13 @@ mod tests {
         assert!(bi.elf_sections_tag().is_none());
         assert!(bi.memory_map_tag().is_none());
         assert!(bi.module_tags().next().is_none());
-        assert_eq!("name", bi.boot_loader_name_tag().unwrap().name());
+        assert_eq!(
+            "name",
+            bi.boot_loader_name_tag()
+                .expect("tag must be present")
+                .name()
+                .expect("must be valid utf8")
+        );
         assert!(bi.command_line_tag().is_none());
     }
 
@@ -1231,9 +1237,18 @@ mod tests {
         assert!(bi.module_tags().next().is_none());
         assert_eq!(
             "GRUB 2.02~beta3-5",
-            bi.boot_loader_name_tag().unwrap().name()
+            bi.boot_loader_name_tag()
+                .expect("tag must be present")
+                .name()
+                .expect("must be valid utf-8")
+        );
+        assert_eq!(
+            "",
+            bi.command_line_tag()
+                .expect("tag must present")
+                .command_line()
+                .expect("must be valid utf-8")
         );
-        assert_eq!("", bi.command_line_tag().unwrap().command_line());
 
         // Test the Framebuffer tag
         let fbi = bi.framebuffer_tag().unwrap();

multiboot2/src/module.rs

@@ -1,5 +1,6 @@
 use crate::tag_type::{Tag, TagIter, TagType};
 use core::fmt::{Debug, Formatter};
+use core::str::Utf8Error;
 
 /// This tag indicates to the kernel what boot module was loaded along with
 /// the kernel image, and where it can be found.
@@ -10,25 +11,24 @@ pub struct ModuleTag {
     size: u32,
     mod_start: u32,
     mod_end: u32,
-    /// Begin of the command line string.
+    /// Null-terminated UTF-8 string
     cmdline_str: u8,
 }
 
 impl ModuleTag {
-    // The multiboot specification defines the module str as valid utf-8 (zero terminated string),
-    // therefore this function produces defined behavior
-    /// Get the cmdline of the module. If the GRUB configuration contains
-    /// `module2 /foobar/some_boot_module --test cmdline-option`, then this method
+    /// Returns the cmdline of the module.
+    /// This is an null-terminated UTF-8 string. If this returns `Err` then perhaps the memory
+    /// is invalid or the bootloader doesn't follow the spec.
+    ///
+    /// For example: If the GRUB configuration contains
+    /// `module2 /foobar/some_boot_module --test cmdline-option` then this method
     /// will return `--test cmdline-option`.
-    pub fn cmdline(&self) -> &str {
+    pub fn cmdline(&self) -> Result<&str, Utf8Error> {
         use core::{mem, slice, str};
+        // strlen without null byte
         let strlen = self.size as usize - mem::size_of::<ModuleTag>();
-        unsafe {
-            str::from_utf8_unchecked(slice::from_raw_parts(
-                &self.cmdline_str as *const u8,
-                strlen,
-            ))
-        }
+        let bytes = unsafe { slice::from_raw_parts((&self.cmdline_str) as *const u8, strlen) };
+        str::from_utf8(bytes)
     }
 
     /// Start address of the module.
@@ -89,3 +89,39 @@ impl<'a> Debug for ModuleIter<'a> {
         list.finish()
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use crate::TagType;
+
+    const MSG: &str = "hello";
+
+    /// Returns the tag structure in bytes in native endian format.
+    fn get_bytes() -> std::vec::Vec<u8> {
+        // size is: 4 bytes for tag + 4 bytes for size + length of null-terminated string
+        //          4 bytes mod_start + 4 bytes mod_end
+        let size = (4 + 4 + 4 + 4 + MSG.as_bytes().len() + 1) as u32;
+        [
+            &((TagType::Module as u32).to_ne_bytes()),
+            &size.to_ne_bytes(),
+            &0_u32.to_ne_bytes(),
+            &0_u32.to_ne_bytes(),
+            MSG.as_bytes(),
+            // Null Byte
+            &[0],
+        ]
+        .iter()
+        .flat_map(|bytes| bytes.iter())
+        .copied()
+        .collect()
+    }
+
+    /// Tests to parse a string with a terminating null byte from the tag (as the spec defines).
+    #[test]
+    fn test_parse_str() {
+        let tag = get_bytes();
+        let tag = unsafe { tag.as_ptr().cast::<super::ModuleTag>().as_ref().unwrap() };
+        assert_eq!({ tag.typ }, TagType::Module);
+        assert_eq!(tag.cmdline().expect("must be valid UTF-8"), MSG);
+    }
+}