uefi-rs APIs which want a well-aligned buffer but accept &mut [u8] are unsafe

[HadrienG2]

If the user-provided slice doesn't actually match the expected alignment, undefined behavior will ensue.

FWIW, many of these buffer-wranging APIs actually want to receive an *mut [u8], because they are often meant to manipulate uninitialized memory and that concept doesn't exist at the Rust reference layer.

[GabrielMajeri]

Would having some assertions for each such respective function ensure safety?

[HadrienG2]

It would ensure safety in the presence of misaligned buffers (since [u8] itself has no such requirements), but using uninitialized memory as returned by e.g. UEFI's memory allocator would still be UB (although I don't think rustc can harmfully exploit this particular UB at the moment).

[kotauskas]

The unitialized memory UB can be solved by using &mut [MaybeUninit<u8>] You can go a bit further and just use &mut [MaybeUninit<MemoryDescriptor>] in memory_map().

[GabrielMajeri]

@kotauskas the idea with &mut [MaybeUninit<u8>] could work. The one with &mut [MaybeUninit<MemoryDescriptor>] wouldn't necessarily, since the memory_map API doesn't guarantee the returned memory descriptors are contiguous / occupy a fixed amount of spaced (you need to check each one to get the offset of the next.)

[Soveu]

Currently memory_map would panic if the buffer is not aligned, but it could just take a slice that is aligned

let buf = &mut buf[buf.as_ptr().align_offset(8)..];

[hanmertens]

Alternatively, a new type could be defined to represent a possibly uninitialized buffer with certain alignment requirements, e.g.:

#[repr(C, align(8))]
pub struct MemoryMapBuffer([MaybeUninit<u8>]);

A &mut MemoryMapBuffer could then be constructed unsafely from a pointer and length, or safely from a byte slice. In the former case the caller is responsible for providing the correct alignment, in the latter case an incorrectly aligned slice can be handled by taking a subslice like @Soveu suggested.

[Soveu]

Alternatively, a new type could be defined to represent a possibly uninitialized buffer with certain alignment requirements, e.g.:

#[repr(C, align(8))]
pub struct MemoryMapBuffer([MaybeUninit<u8>]);

align(8) also requires that length is a multiple of 8, so really this is just [u64] (assuming u64 is aligned to 8)
Otherwise it is UB according to Miri

[hanmertens]

Minor nitpick: it's not that the length has to be a multiple of 8, because changing let newlen = newlen - (newlen % align); to let newlen = newlen - 8; stops Miri from complaining as well. I think the UB is caused by the Vec allocation not backing the buffer's length rounded up to a multiple of 8 (although I don't understand why that would be UB). In any case, just using a slice is probably a better solution then.

[Soveu]

It is UB, because size_of_val % align_of_val must be 0
In this case, the wrapper struct has size in multiple of 8, even though the inner slice can have any size (so a multiple of 1)

[phip1611]

MaybeUninit<u8>

I think this doesn't make sense. Every possible memory value is a valid u8. MaybeUninit is only needed for sophisticated types IMHO, such as MaybeUninit<MyCoolStruct>.

I'm closing this due to inactivity. Please re-open with more specific pointers and suggestions matching the current code-base if necessary.

ping @nicholasbishop

[kotauskas]

Every possible memory value is a valid u8.

@phip1611 Due to the value of an uninitialized byte being considered "floating" (unfrozen) by the compiler, it is UB to read an uninitialized integer despite every bit pattern being valid.

[nicholasbishop]

I agree this is fine to close, I think we've resolved the places where UB due to unalignment was possible. (If more such places are found, we can create a fresh bug.)

Every possible memory value is a valid u8. MaybeUninit is only needed for sophisticated types IMHO, such as MaybeUninit.

This isn't correct unfortunately due to the way optimizing compilers work. (There are probably more places in uefi-rs where it would be good to use MaybeUninit, but again we can open fresh or PRs if we find such places.) https://www.ralfj.de/blog/2019/07/14/uninit.html is a good explanation of why it's not sufficient to just think about the physical hardware value.