Potential unsoundness in libbpf-cargo generated code

[danielocfb]

I think there is possibility of unsoundness in the generated code, depending on what C code does. E.g., in the capable example we have the tool_config struct, which contains the uniqueness enumeration:

libbpf-rs/examples/capable/src/bpf/capable.bpf.c

Lines 13 to 18 in cb4186d

	const volatile struct {
	gid_t tgid; //PID to filter
	bool verbose; // Include non audit logs
	enum uniqueness unique_type; // Only unique info traces for same pid or cgroup
	} tool_config = {};

libbpf-rs/examples/capable/src/bpf/capable.h

Lines 21 to 23 in cb4186d

	enum uniqueness {
	UNQ_OFF, UNQ_PID, UNQ_CGROUP
	};

Once we generate the skeleton we end up with:

pub mod capable_rodata_types {
    #[derive(Debug, Copy, Clone)]
    #[repr(C)]
    pub struct rodata {
        pub tool_config: __anon_1,
    }
    #[derive(Debug, Default, Copy, Clone)]
    #[repr(C)]
    pub struct __anon_1 {
        pub tgid: u32,
        pub verbose: bool,
        pub unique_type: uniqueness,
    }
    #[derive(Debug, Copy, Clone, Default, PartialEq, Eq)]
    #[repr(u32)]
    pub enum uniqueness {
        #[default]
        UNQ_OFF = 0,
        UNQ_PID = 1,
        UNQ_CGROUP = 2,
    }
}

and

pub fn rodata(&mut self) -> &'_ mut capable_rodata_types::rodata {
    unsafe {
        std::mem::transmute::<*mut std::ffi::c_void, &'_ mut capable_rodata_types::rodata>(
            self.skel_config.map_mmap_ptr(2).unwrap(),
        )
    }
}

That cast, however, is anything but safe. Rust's uniqueness requires that it has only any of the variant values 0, 1, or 2. But in C code it is fine to assign, say, 0xff to the enum object (without triggering UB, I believe). The result would be UB in Rust.

A similar thing can be observed for bool, though it may be that both languages have the same bool representation (questionable, because C leaves binary representation mostly undefined, I believe, but if it's all LLVM we probably luck out).

My reading is that this is basically a variant of rust-lang/rust#36927 (though I haven't gone through the issue in its entirety).

So at the very least we should make rodata (and similar methods) unsafe and document that it's on users. The better approach is probably to translate enums to constants on the Rust side, which I understand is done for a lot of foreign function interfaces.

[danielocfb]

Perhaps the most sensible solution is to wrap all enum instances in MaybeUninit. That moves the burden of check to the user, it is happening at the lowest level, which makes it clear what invariants need to be upheld (as opposed to making, say, bss an unsafe function), and it is the least intrusive (it would only affect the types that we identify as problematic).

[d-e-s-o]

char is another type that is not valid for all bit patterns.

  |     let c = unsafe { *(values.1.as_ptr() as *const u8).cast::<char>() };
  |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |                      constructing invalid value: encountered 0x00110000,
  |                      but expected a valid unicode scalar value
  |                      (in `0..=0x10FFFF` but not in `0xD800..=0xDFFF`)

[d-e-s-o]

The char point is probably moot, because I don't believe we ever generate a char attribute in a skeleton. But for enums and bools, I found a way to wrap them in MaybeUninit, which I still believe to represent the best overall trade off.