multiboot2: prevent panic in framebuffer_tag()

Author: phip1611

multiboot2/Changelog.md

@@ -2,6 +2,12 @@
 
 ## Unreleased
 - MSRV is 1.56.1
+- **BREAKING** fixed lifetime issues: `VBEInfoTag` is no longer static
+- fixed another internal lifetime issue
+- `BootInformation::framebuffer_tag()` now returns
+  `Option<Result<FramebufferTag, UnknownFramebufferType>>` instead of
+  `Option<FramebufferTag>` which prevents a possible panic. If the `--unstable`
+  feature is used, `UnknownFramebufferType` implements `core::error::Error`.
 
 ## 0.14.2 (2023-03-17)
 - documentation fixes

multiboot2/src/framebuffer.rs

@@ -1,6 +1,7 @@
 use crate::tag_type::Tag;
 use crate::Reader;
 use core::slice;
+use derive_more::Display;
 
 /// The VBE Framebuffer information Tag.
 #[derive(Debug, PartialEq, Eq)]
@@ -28,6 +29,17 @@ pub struct FramebufferTag<'a> {
     pub buffer_type: FramebufferType<'a>,
 }
 
+/// Helper struct for [`FramebufferType`].
+#[derive(Debug, PartialEq, Eq)]
+#[repr(u8)]
+pub enum FramebufferTypeId {
+    Indexed = 0,
+    #[allow(clippy::upper_case_acronyms)]
+    RGB = 1,
+    Text = 2,
+    // spec says: there may be more variants in the future
+}
+
 /// The type of framebuffer.
 #[derive(Debug, PartialEq, Eq)]
 pub enum FramebufferType<'a> {
@@ -79,7 +91,16 @@ pub struct FramebufferColor {
     pub blue: u8,
 }
 
-pub fn framebuffer_tag(tag: &Tag) -> FramebufferTag {
+/// Error when an unknown [`FramebufferTypeId`] is found.
+#[derive(Debug, Copy, Clone, Display, PartialEq, Eq)]
+#[display(fmt = "Unknown framebuffer type _0")]
+pub struct UnknownFramebufferType(u8);
+
+#[cfg(feature = "unstable")]
+impl core::error::Error for UnknownFramebufferType {}
+
+/// Transforms a [`Tag`] into a [`FramebufferTag`].
+pub fn framebuffer_tag(tag: &Tag) -> Result<FramebufferTag, UnknownFramebufferType> {
     let mut reader = Reader::new(tag as *const Tag);
     reader.skip(8);
     let address = reader.read_u64();
@@ -88,20 +109,27 @@ pub fn framebuffer_tag(tag: &Tag) -> FramebufferTag {
     let height = reader.read_u32();
     let bpp = reader.read_u8();
     let type_no = reader.read_u8();
-    reader.skip(2); // In the multiboot spec, it has this listed as a u8 _NOT_ a u16.
-                    // Reading the GRUB2 source code reveals it is in fact a u16.
-    let buffer_type = match type_no {
-        0 => {
+    // In the multiboot spec, it has this listed as a u8 _NOT_ a u16.
+    // Reading the GRUB2 source code reveals it is in fact a u16.
+    reader.skip(2);
+    let buffer_type_id = match type_no {
+        0 => Ok(FramebufferTypeId::Indexed),
+        1 => Ok(FramebufferTypeId::RGB),
+        2 => Ok(FramebufferTypeId::Text),
+        id => Err(UnknownFramebufferType(id)),
+    }?;
+    let buffer_type = match buffer_type_id {
+        FramebufferTypeId::Indexed => {
             let num_colors = reader.read_u32();
             let palette = unsafe {
                 slice::from_raw_parts(
                     reader.current_address() as *const FramebufferColor,
                     num_colors as usize,
                 )
-            } as &'static [FramebufferColor];
+            } as &[FramebufferColor];
             FramebufferType::Indexed { palette }
         }
-        1 => {
+        FramebufferTypeId::RGB => {
             let red_pos = reader.read_u8(); // These refer to the bit positions of the LSB of each field
             let red_mask = reader.read_u8(); // And then the length of the field from LSB to MSB
             let green_pos = reader.read_u8();
@@ -123,16 +151,15 @@ pub fn framebuffer_tag(tag: &Tag) -> FramebufferTag {
                 },
             }
         }
-        2 => FramebufferType::Text,
-        _ => panic!("Unknown framebuffer type: {}", type_no),
+        FramebufferTypeId::Text => FramebufferType::Text,
     };
 
-    FramebufferTag {
+    Ok(FramebufferTag {
         address,
         pitch,
         width,
         height,
         bpp,
         buffer_type,
-    }
+    })
 }

multiboot2/src/lib.rs

@@ -40,6 +40,7 @@ extern crate std;
 use core::fmt;
 use derive_more::Display;
 
+use crate::framebuffer::UnknownFramebufferType;
 pub use boot_loader_name::BootLoaderNameTag;
 pub use command_line::CommandLineTag;
 pub use efi::{EFIImageHandle32, EFIImageHandle64, EFISdt32, EFISdt64};
@@ -244,8 +245,9 @@ impl BootInformation {
             .map(|tag| unsafe { &*(tag as *const Tag as *const CommandLineTag) })
     }
 
-    /// Search for the VBE framebuffer tag.
-    pub fn framebuffer_tag(&self) -> Option<FramebufferTag> {
+    /// Search for the VBE framebuffer tag. The result is `Some(Err(e))`, if the
+    /// framebuffer type is unknown, while the framebuffer tag is present.
+    pub fn framebuffer_tag(&self) -> Option<Result<FramebufferTag, UnknownFramebufferType>> {
         self.get_tag(TagType::Framebuffer)
             .map(framebuffer::framebuffer_tag)
     }
@@ -305,7 +307,7 @@ impl BootInformation {
     }
 
     /// Search for the VBE information tag.
-    pub fn vbe_info_tag(&self) -> Option<&'static VBEInfoTag> {
+    pub fn vbe_info_tag(&self) -> Option<&VBEInfoTag> {
         self.get_tag(TagType::Vbe)
             .map(|tag| unsafe { &*(tag as *const Tag as *const VBEInfoTag) })
     }
@@ -588,7 +590,7 @@ mod tests {
         use framebuffer::{FramebufferField, FramebufferTag, FramebufferType};
         assert_eq!(
             bi.framebuffer_tag(),
-            Some(FramebufferTag {
+            Some(Ok(FramebufferTag {
                 address: 4244635648,
                 pitch: 5120,
                 width: 1280,
@@ -608,7 +610,7 @@ mod tests {
                         size: 8
                     }
                 }
-            })
+            }))
         )
     }
 
@@ -643,7 +645,10 @@ mod tests {
         assert_eq!(bytes.0.len(), bi.total_size());
         use framebuffer::{FramebufferColor, FramebufferType};
         assert!(bi.framebuffer_tag().is_some());
-        let fbi = bi.framebuffer_tag().unwrap();
+        let fbi = bi
+            .framebuffer_tag()
+            .expect("Framebuffer info should be available")
+            .expect("Framebuffer info type should be valid");
         assert_eq!(fbi.address, 4244635648);
         assert_eq!(fbi.pitch, 5120);
         assert_eq!(fbi.width, 1280);
@@ -1255,7 +1260,10 @@ mod tests {
         );
 
         // Test the Framebuffer tag
-        let fbi = bi.framebuffer_tag().unwrap();
+        let fbi = bi
+            .framebuffer_tag()
+            .expect("Framebuffer info should be available")
+            .expect("Framebuffer info type should be valid");
         assert_eq!(fbi.address, 753664);
         assert_eq!(fbi.pitch, 160);
         assert_eq!(fbi.width, 80);