multiboot2: Fix framebuffer code

phip1611 · phip1611 · commit 9987fa3c5565 · 2024-08-20T10:41:46.000+02:00
Now, the last `#[cfg_attr(miri, ignore)]` is gone!
diff --git a/multiboot2/Changelog.md b/multiboot2/Changelog.md
@@ -10,15 +10,27 @@ corer-cases removed. Further, the builder's API was streamlined.
 If you are interested in the internals of the major refactorings recently taken
 place, please head to the documentation of `multiboot2-common`.
 
-- **Breaking** The builder type is now just called `Builder`. This needs the
+- **Breaking:** The builder type is now just called `Builder`. This needs the
   `builder` feature.
-- **Breaking** The framebuffer tag has been marked `unsafe` as its
-  implementation contains UB and needs rework. This originates from no longer
-  active contributors. **Contributions are welcome.** This is the last place
-  with UB that I am aware of after the recent intense and tough refactorings.
-- The trait `TagTrait` was removed and was replaced by a new `Tag` trait coming
-  from `multiboot2-common`. This only affects you if you provide custom tag
-  types for the library.
+- **Breaking:** The framebuffer tag was refactored and several bugs, memory
+- issues, and UB were fixed. It is now safe to use this, but some existing
+  usages might break and need to be slightly adapted.
+- **Breaking:** The trait `TagTrait` was removed and was replaced by a new `Tag`
+  trait coming from `multiboot2-common`. This only affects you if you provide
+  custom tag types for the library.
+
+**General Note on Safety and UB (TL;DR: Crate is Safe)**
+
+The major refactorings of release `0.21` and `0.22` were an incredible step
+forward in code quality and memory safety. We have a comprehensive test coverage
+and all tests are passed by Miri. It might be that by using fuzzing, more
+corner and niche cases where UB can occur get uncovered. However, for every-day
+usage with sane bootloaders that do not intentionally create malformed tags, you
+are now absolutely good to go.
+
+Sorry for all the UB that silently slept insight many parts of the code base.
+This is a community project that has grown over the years. But now, the code
+base is in excellent shape!
 
 ## 0.21.0 (2024-08-17)
 
diff --git a/multiboot2/src/builder.rs b/multiboot2/src/builder.rs
@@ -280,8 +280,9 @@ impl Builder {
 #[cfg(test)]
 mod tests {
     use super::*;
-    use crate::framebuffer::FramebufferTypeId;
-    use crate::{BootInformation, MemoryArea, MemoryAreaType, VBEControlInfo, VBEModeInfo};
+    use crate::{
+        BootInformation, FramebufferType, MemoryArea, MemoryAreaType, VBEControlInfo, VBEModeInfo,
+    };
     use uefi_raw::table::boot::MemoryDescriptor;
 
     #[test]
@@ -312,7 +313,7 @@ mod tests {
                 756,
                 1024,
                 8,
-                FramebufferTypeId::Text,
+                FramebufferType::Text,
             ))
             .elf_sections(ElfSectionsTag::new(0, 32, 0, &[]))
             .efi32(EFISdt32Tag::new(0x1000))
diff --git a/multiboot2/src/framebuffer.rs b/multiboot2/src/framebuffer.rs
@@ -10,39 +10,38 @@ use multiboot2_common::{MaybeDynSized, Tag};
 #[cfg(feature = "builder")]
 use {alloc::boxed::Box, multiboot2_common::new_boxed};
 
-/// TODO this memory reader is unsafe and causes UB, according to Miri.
-/// We need to replace it.
-///
 /// Helper struct to read bytes from a raw pointer and increase the pointer
 /// automatically.
-struct Reader {
-    ptr: *const u8,
+struct Reader<'a> {
+    buffer: &'a [u8],
     off: usize,
 }
 
-impl Reader {
-    const fn new<T>(ptr: *const T) -> Self {
-        Self {
-            ptr: ptr as *const u8,
-            off: 0,
-        }
+impl<'a> Reader<'a> {
+    const fn new(buffer: &'a [u8]) -> Self {
+        Self { buffer, off: 0 }
     }
 
     fn read_u8(&mut self) -> u8 {
+        let val = self
+            .buffer
+            .get(self.off)
+            .cloned()
+            // This is not a solution I'm proud of, but at least it is safe.
+            // The whole framebuffer tag code originally is not from me.
+            // I hope someone from the community wants to improve this overall
+            // functionality someday.
+            .expect("Embedded framebuffer info should be properly sized and available");
         self.off += 1;
-        unsafe { *self.ptr.add(self.off - 1) }
+        val
     }
 
     fn read_u16(&mut self) -> u16 {
         self.read_u8() as u16 | (self.read_u8() as u16) << 8
     }
 
-    fn read_u32(&mut self) -> u32 {
-        self.read_u16() as u32 | (self.read_u16() as u32) << 16
-    }
-
-    fn current_address(&self) -> usize {
-        unsafe { self.ptr.add(self.off) as usize }
+    const fn current_ptr(&self) -> *const u8 {
+        unsafe { self.buffer.as_ptr().add(self.off) }
     }
 }
 
@@ -71,15 +70,16 @@ pub struct FramebufferTag {
     /// Contains number of bits per pixel.
     bpp: u8,
 
-    /// The type of framebuffer, one of: `Indexed`, `RGB` or `Text`.
-    type_no: u8,
+    /// The type of framebuffer. See [`FramebufferTypeId`].
+    // TODO: Strictly speaking this causes UB for invalid values. However, no
+    //  sane bootloader puts something illegal there at the moment. When we
+    //  refactor this (newtype pattern?), we should also streamline other
+    //  parts in the code base accordingly.
+    framebuffer_type: FramebufferTypeId,
 
-    // 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.
-    _reserved: u16,
+    _padding: u16,
 
-    // TODO This situation is currently not properly typed. Someone needs to
-    // look into it.
+    /// This optional data and its meaning depend on the [`FramebufferTypeId`].
     buffer: [u8],
 }
 
@@ -93,14 +93,16 @@ impl FramebufferTag {
         width: u32,
         height: u32,
         bpp: u8,
-        buffer_type: FramebufferTypeId,
+        buffer_type: FramebufferType,
     ) -> Box<Self> {
         let header = TagHeader::new(Self::ID, 0);
         let address = address.to_ne_bytes();
         let pitch = pitch.to_ne_bytes();
         let width = width.to_ne_bytes();
         let height = height.to_ne_bytes();
+        let buffer_type_id = buffer_type.id();
         let padding = [0; 2];
+        let optional_buffer = buffer_type.serialize();
         new_boxed(
             header,
             &[
@@ -109,8 +111,9 @@ impl FramebufferTag {
                 &width,
                 &height,
                 &[bpp],
-                &[buffer_type as u8],
+                &[buffer_type_id as u8],
                 &padding,
+                &optional_buffer,
             ],
         )
     }
@@ -149,26 +152,33 @@ impl FramebufferTag {
         self.bpp
     }
 
-    /// TODO unsafe. Someone needs to fix this. This causes UB according to Miri.
-    ///  Dont forget to reenable all test usages once fixed.
-    ///
     /// The type of framebuffer, one of: `Indexed`, `RGB` or `Text`.
-    ///
-    /// # Safety
-    /// This function needs refactoring. This was never safe, since the beginning.
-    pub unsafe fn buffer_type(&self) -> Result<FramebufferType, UnknownFramebufferType> {
-        let mut reader = Reader::new(self.buffer.as_ptr());
-        let typ = FramebufferTypeId::try_from(self.type_no)?;
-        match typ {
+    pub fn buffer_type(&self) -> Result<FramebufferType, UnknownFramebufferType> {
+        let mut reader = Reader::new(&self.buffer);
+
+        // TODO: We should use the newtype pattern instead or so to properly
+        //  solve this.
+        let fb_type_raw = self.framebuffer_type as u8;
+        let fb_type = FramebufferTypeId::try_from(fb_type_raw)?;
+
+        match fb_type {
             FramebufferTypeId::Indexed => {
-                let num_colors = reader.read_u32();
-                // TODO static cast looks like UB?
-                let palette = unsafe {
-                    slice::from_raw_parts(
-                        reader.current_address() as *const FramebufferColor,
-                        num_colors as usize,
-                    )
-                } as &'static [FramebufferColor];
+                // TODO we can create a struct for this and implement
+                //  DynSizedStruct for it to leverage the already existing
+                //  functionality
+                let num_colors = reader.read_u16();
+
+                let palette = {
+                    // Ensure the slice can be created without causing UB
+                    assert_eq!(size_of::<FramebufferColor>(), 3);
+
+                    unsafe {
+                        slice::from_raw_parts(
+                            reader.current_ptr().cast::<FramebufferColor>(),
+                            num_colors as usize,
+                        )
+                    }
+                };
                 Ok(FramebufferType::Indexed { palette })
             }
             FramebufferTypeId::RGB => {
@@ -224,8 +234,7 @@ impl Debug for FramebufferTag {
         f.debug_struct("FramebufferTag")
             .field("typ", &self.header.typ)
             .field("size", &self.header.size)
-            // TODO unsafe. Fix in a follow-up commit
-            //.field("buffer_type", &self.buffer_type())
+            .field("buffer_type", &self.buffer_type())
             .field("address", &self.address)
             .field("pitch", &self.pitch)
             .field("width", &self.width)
@@ -243,12 +252,12 @@ impl PartialEq for FramebufferTag {
             && self.width == { other.width }
             && self.height == { other.height }
             && self.bpp == { other.bpp }
-            && self.type_no == { other.type_no }
+            && self.framebuffer_type == { other.framebuffer_type }
             && self.buffer == other.buffer
     }
 }
 
-/// Helper struct for [`FramebufferType`].
+/// ABI-compatible framebuffer type.
 #[derive(Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)]
 #[repr(u8)]
 #[allow(clippy::upper_case_acronyms)]
@@ -282,7 +291,8 @@ impl From<FramebufferType<'_>> for FramebufferTypeId {
     }
 }
 
-/// The type of framebuffer.
+/// Structured accessory to the provided framebuffer type that is not ABI
+/// compatible.
 #[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)]
 pub enum FramebufferType<'a> {
     /// Indexed color.
@@ -309,6 +319,46 @@ pub enum FramebufferType<'a> {
     Text,
 }
 
+impl<'a> FramebufferType<'a> {
+    #[must_use]
+    const fn id(&self) -> FramebufferTypeId {
+        match self {
+            FramebufferType::Indexed { .. } => FramebufferTypeId::Indexed,
+            FramebufferType::RGB { .. } => FramebufferTypeId::RGB,
+            FramebufferType::Text => FramebufferTypeId::Text,
+        }
+    }
+
+    #[must_use]
+    #[cfg(feature = "builder")]
+    fn serialize(&self) -> alloc::vec::Vec<u8> {
+        let mut data = alloc::vec::Vec::new();
+        match self {
+            FramebufferType::Indexed { palette } => {
+                // TODO we can create a struct for this and implement
+                //  DynSizedStruct for it to leverage the already existing
+                //  functionality
+                let num_colors = palette.len() as u16;
+                data.extend(&num_colors.to_ne_bytes());
+                for color in *palette {
+                    let serialized_color = [color.red, color.green, color.blue];
+                    data.extend(&serialized_color);
+                }
+            }
+            FramebufferType::RGB { red, green, blue } => data.extend(&[
+                red.position,
+                red.size,
+                green.position,
+                green.size,
+                blue.position,
+                blue.size,
+            ]),
+            FramebufferType::Text => {}
+        }
+        data
+    }
+}
+
 /// An RGB color type field.
 #[derive(Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)]
 #[repr(C)]
@@ -320,7 +370,9 @@ pub struct FramebufferField {
     pub size: u8,
 }
 
-/// A framebuffer color descriptor in the palette.
+/// A framebuffer color descriptor in the palette. On the ABI level, multiple
+/// values are consecutively without padding bytes. The spec is not precise in
+/// that regard, but looking at Limine's and GRUB's source code confirm that.
 #[derive(Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)]
 #[repr(C)] // no align(8) here is correct
 pub struct FramebufferColor {
@@ -355,7 +407,56 @@ mod tests {
     #[test]
     #[cfg(feature = "builder")]
     fn create_new() {
-        let tag = FramebufferTag::new(0x1000, 1, 1024, 1024, 8, FramebufferTypeId::Indexed);
+        let tag = FramebufferTag::new(0x1000, 1, 1024, 1024, 8, FramebufferType::Text);
+        // Good test for Miri
+        dbg!(tag);
+
+        let tag = FramebufferTag::new(
+            0x1000,
+            1,
+            1024,
+            1024,
+            8,
+            FramebufferType::Indexed {
+                palette: &[
+                    FramebufferColor {
+                        red: 255,
+                        green: 255,
+                        blue: 255,
+                    },
+                    FramebufferColor {
+                        red: 127,
+                        green: 42,
+                        blue: 73,
+                    },
+                ],
+            },
+        );
+        // Good test for Miri
+        dbg!(tag);
+
+        let tag = FramebufferTag::new(
+            0x1000,
+            1,
+            1024,
+            1024,
+            8,
+            FramebufferType::RGB {
+                red: FramebufferField {
+                    position: 0,
+                    size: 0,
+                },
+                green: FramebufferField {
+                    position: 10,
+                    size: 20,
+                },
+                blue: FramebufferField {
+                    position: 30,
+                    size: 40,
+                },
+            },
+        );
+        // Good test for Miri
         dbg!(tag);
     }
 }
diff --git a/multiboot2/src/lib.rs b/multiboot2/src/lib.rs
