libbpf-cargo: Wrap generated "unsafe" types in MaybeUninit

Certain Rust types that we use in our generated skeleton are not valid
for all bit patterns. Bools, for example, are defined to be one byte in
size, but their representation allows only for 0 and 1 values to be used
in said byte -- everything else is undefined behavior. Enums have
similar problems, in that not the entire space of the backing type is
necessarily used by variants.
The result is an unsoundness issue in our generated skeletons, because C
code could accidentally set an enum to an invalid value and Rust code
would exhibit undefined behavior.
This change fixes this problem by wrapping the corresponding attributes
in MaybeUninit. In so doing users have to be explicit when accessing
them and make sure that the current representation is indeed valid.

Closes: #589
Signed-off-by: Daniel Müller <[email protected]>

Author: d-e-s-o

examples/capable/src/main.rs

@@ -187,8 +187,16 @@ fn main() -> Result<()> {
     let mut open_skel = skel_builder.open()?;
     //Pass configuration to BPF
     open_skel.rodata_mut().tool_config.tgid = opts.pid; //tgid in kernel is pid in userland
-    open_skel.rodata_mut().tool_config.verbose = opts.verbose;
-    open_skel.rodata_mut().tool_config.unique_type = opts.unique_type;
+    open_skel
+        .rodata_mut()
+        .tool_config
+        .verbose
+        .write(opts.verbose);
+    open_skel
+        .rodata_mut()
+        .tool_config
+        .unique_type
+        .write(opts.unique_type);
 
     let mut skel = open_skel.load()?;
     skel.attach()?;

libbpf-cargo/CHANGELOG.md

@@ -5,6 +5,8 @@ Unreleased
 - Adjusted `SkeletonBuilder::clang_args` to accept an iterator of
   arguments instead of a string
 - Added `--clang-args` argument to `make` and `build` sub-commands
+- Fixed potential unsoundness issues in generated skeletons by wrapping "unsafe"
+  type in `MaybeUninit`
 - Updated `libbpf-sys` dependency to `1.3.0`
 - Bumped minimum Rust version to `1.70`
 

libbpf-cargo/src/gen/btf.rs

@@ -24,6 +24,20 @@ use libbpf_rs::ReferencesType;
 
 const ANON_PREFIX: &str = "__anon_";
 
+/// Check whether the provided type is "unsafe" to use.
+///
+/// A type is considered unsafe by this function if it is not valid for
+/// any bit pattern.
+fn is_unsafe(ty: BtfType<'_>) -> bool {
+    let ty = ty.skip_mods_and_typedefs();
+
+    btf_type_match!(match ty {
+        BtfKind::Int(t) => matches!(t.encoding, types::IntEncoding::Bool),
+        BtfKind::Enum | BtfKind::Enum64 => true,
+        _ => false,
+    })
+}
+
 pub struct GenBtf<'s> {
     btf: Btf<'s>,
     // We use refcell here because the design of this type unfortunately causes a lot of borrowing
@@ -359,17 +373,23 @@ impl<'s> GenBtf<'s> {
                 }
 
                 // Rust does not implement `Default` for pointers, no matter if
-                // the pointee implements it.
+                // the pointee implements it, and it also doesn't do it for
+                // `MaybeUninit` constructs, which we use for "unsafe" types.
                 if self
                     .type_by_id::<types::Ptr<'_>>(field_ty.type_id())
                     .is_some()
+                    || is_unsafe(field_ty)
                 {
                     gen_impl_default = true
                 }
             }
 
             match self.type_default(field_ty) {
-                Ok(def) => {
+                Ok(mut def) => {
+                    if is_unsafe(field_ty) {
+                        def = format!("std::mem::MaybeUninit::new({def})")
+                    }
+
                     impl_default.push(format!(
                         r#"            {field_name}: {field_ty_str}"#,
                         field_name = if let Some(name) = member.name {
@@ -394,7 +414,13 @@ impl<'s> GenBtf<'s> {
             let field_name = if let Some(name) = member.name {
                 name.to_string_lossy()
             } else {
-                field_ty_str.as_str().into()
+                Cow::Borrowed(field_ty_str.as_str())
+            };
+
+            let field_ty_str = if is_unsafe(field_ty) {
+                Cow::Owned(format!("std::mem::MaybeUninit<{field_ty_str}>"))
+            } else {
+                Cow::Borrowed(field_ty_str.as_str())
             };
 
             agg_content.push(format!(r#"    pub {field_name}: {field_ty_str},"#));

libbpf-cargo/src/test.rs

@@ -2375,10 +2375,17 @@ struct Foo foo;
 "#;
 
     let expected_output = r#"
-#[derive(Debug, Default, Copy, Clone)]
+#[derive(Debug, Copy, Clone)]
 #[repr(C)]
 pub struct Foo {
-    pub test: __anon_1,
+    pub test: std::mem::MaybeUninit<__anon_1>,
+}
+impl Default for Foo {
+    fn default() -> Self {
+        Foo {
+            test: std::mem::MaybeUninit::new(__anon_1::default()),
+        }
+    }
 }
 #[derive(Debug, Copy, Clone, Default, PartialEq, Eq)]
 #[repr(u32)]
@@ -2414,15 +2421,26 @@ struct Foo foo;
 "#;
 
     let expected_output = r#"
-#[derive(Debug, Default, Copy, Clone)]
+#[derive(Debug, Copy, Clone)]
 #[repr(C)]
 pub struct Foo {
     pub a: i32,
     pub b: u16,
     pub c: i16,
-    pub d: bool,
+    pub d: std::mem::MaybeUninit<bool>,
     pub e: i8,
 }
+impl Default for Foo {
+    fn default() -> Self {
+        Foo {
+            a: i32::default(),
+            b: u16::default(),
+            c: i16::default(),
+            d: std::mem::MaybeUninit::new(bool::default()),
+            e: i8::default(),
+        }
+    }
+}
 "#;
 
     let mmap = build_btf_mmap(prog_text);