The response accessors returned `&'a` instead of borrowing from `self`.
So any returned value was bound to the lifetime of the underlying buffer
which is wrong semantics.
`from_initialized_mem` only checked that `desc_size` was large enough,
unlike the `MemoryMapRef`/`MemoryMapRefMut` constructors, which also
require it to be a multiple of the descriptor alignment. With an
unaligned `desc_size`, every entry but the first was accessed through a
misaligned `&MemoryDescriptor`.
Reuse `validate_meta` so all constructors enforce the same invariant.
`read_chunked` trusted the byte count reported by the read callback. A
firmware reporting more than the requested size made `remaining_size`
underflow and advanced `output_ptr` past the end of the caller buffer,
causing out-of-bounds writes on the following chunk.
Clamp the reported length to the requested size.
The buffer is a byte-aligned `Vec<u8>`, but the code formed a
`&Ip4Config2InterfaceInfo` reference into it. That type has an alignment
of 8 (it contains a pointer), so the reference was misaligned -
undefined behavior.
Per spec and the EDK2 reference implementation, StrToFat writes only
the converted characters and no NUL terminator; the rest of the
caller's buffer keeps whatever content it had.
open_utility_protocol() searched for handles carrying DevicePathToText
and then opened DevicePathUtilities on the first match. This only
works if the firmware happens to install both protocols on the same
handle; otherwise DevicePath::to_pool, append_path, and append_node
(and PciTree::device_path, which builds on them) fail or open the
wrong handle.
Per spec, the EFI_BLOCK_IO_MEDIA members LowestAlignedLba and
LogicalBlocksPerPhysicalBlock are only present if the protocol
revision is at least 2, and OptimalTransferLengthGranularity only
with revision 3. The BlockIOMedia accessors read these fields
unconditionally, i.e. past the firmware-defined structure on
revision-1 implementations.
Since the media struct itself does not carry the revision, move the
three accessors to BlockIO (which does) and return None on old
revisions. They are intentionally not offered on BlockIO2: that
protocol has no revision field of its own, and the spec ties the
fields' presence to the Block I/O protocol revision.
The PI spec defines EFI_PROCESSOR_INFORMATION with a trailing
EXTENDED_PROCESSOR_INFORMATION member (a union whose only member is
the 24-byte EFI_CPU_PHYSICAL_LOCATION2), making the struct 48 bytes.
The Rust struct was only 24 bytes.
get_processor_info passes the processor number through unvalidated,
so a caller could already set CPU_V2_EXTENDED_TOPOLOGY (bit 24) and
make the firmware write the extended topology 24 bytes past the
stack-allocated struct. Firmware performing a whole-struct copy
overflowed it even without that flag.
Add the missing field plus the CPU_V2_EXTENDED_TOPOLOGY constant so
the extended topology is actually usable.
We missed two padding bytes. We cannot use the
`offset_of!(DiscoverInfo, srv_list)` unfortunately as the type is not
Sized. Further, the alignment check was missing. The new unit test
without the fixes uncovers all issues when executed in Miri.
Function consumes byte size, not number of elements. Previously, only
half of the data was passed.
Spec: 2.11, 7.5.1. EFI_BOOT_SERVICES.SetWatchdogTimer()
The spec mandates the standard CRC-32 (ITU-T V.42 / IEEE 802.3,
polynomial 0x04C11DB7) for EFI_TABLE_HEADER.CRC32, not CRC-32C
(Castagnoli, polynomial 0x1EDC6F41). The crate never computes this
checksum itself, but anyone verifying or recomputing table CRCs based
on this doc got mismatches on every conforming system.
Spec: 2.11, 4.2.1. EFI_TABLE_HEADER
PciRegister0/2/3 and PciHeader1Register6 map spec-defined PCI
configuration space dwords onto struct fields via transmute_copy, but
used the default Rust representation, which guarantees no field
order. The mapping only worked because current rustc happens to keep
declaration order for these structs. repr(C) makes the byte-to-field
mapping a language guarantee.
The spec defines EFI_IFR_TYPE_VALUE and its member types EFI_HII_REF,
EFI_HII_TIME, and EFI_HII_DATE with 1-byte packing (UEFI 2.11, 35.5.4;
EDK2 declares them inside #pragma pack(1)): the union is 22 bytes with
alignment 1. The natural repr(C) layout was 24 bytes with alignment 8.
The USB 2.0 specification defines all standard descriptors with 1-byte
packing, and EDK2 declares EFI_USB_*_DESCRIPTOR inside #pragma pack(1)
accordingly [0]. With natural repr(C) layout, ConfigDescriptor was 10
bytes (spec: 9) and EndpointDescriptor 8 bytes (spec: 7) due to the
u16 member; DeviceRequest and DeviceDescriptor deviated in alignment
only.
[0]: 7735ed4f8e/MdePkg/Include/IndustryStandard/Usb.h (L85)
TL;DR: Add an explicit 32-bit padding field after `MemoryDescriptor::ty`
so UEFI memory maps can be parsed correctly from non-UEFI targets such
as kernels and bootloaders compiled for generic i686 targets.
UEFI's `EFI_MEMORY_DESCRIPTOR` places `PhysicalStart` at offset 8. This
happens implicitly on x86_64 and also on IA32 UEFI, where 64-bit values
are 8-byte aligned. EDK II enforces this for GCC IA32 builds with
`-malign-double` [0], and Rust's `i686-unknown-uefi` target explicitly
uses `i64:64` in its LLVM data layout [1].
This differs from the standard x86-32 ABI. GCC normally aligns
`long long` to one word on x86-32 and documents `-malign-double` as an
ABI-changing option [2]. Rust/LLVM follows the standard x86-32 layout
for targets such as `i686-unknown-linux-gnu`, where `u64` has 4-byte ABI
alignment [3]. Consequently, `#[repr(C)]` alone places `phys_start` at
offset 4 instead of the UEFI-required offset 8.
This matters outside UEFI applications. Kernels and bootloaders may
receive and parse a UEFI memory map while being compiled for a generic
target such as a bare-metal i686 target.
For example, the multiboot2 crate re-exports this type as
`EFIMemoryDesc` and uses it to parse EFI memory-map data [4]. The type
therefore needs to describe the UEFI binary layout independently of the
consumer's target ABI.
Reproducer without the explicit padding:
```rust
#[repr(C)]
pub struct MemoryDescriptor {
pub ty: u32,
pub phys_start: u64,
pub virt_start: u64,
pub page_count: u64,
pub att: u64,
}
const _: () = {
assert!(core::mem::offset_of!(MemoryDescriptor, phys_start) == 8);
};
```
```console
# works
cargo check --target x86_64-unknown-linux-gnu
# fails: phys_start is at offset 4
cargo check --target i686-unknown-linux-gnu
# both UEFI targets already have the required layout
cargo check --target x86_64-unknown-uefi
cargo check --target i686-unknown-uefi
```
The explicit padding makes the UEFI-defined field offsets independent
of whether the compilation target itself uses the UEFI ABI.
[0]: 2970e5699b/BaseTools/Conf/tools_def.template (L620)
[1]: 9ba81c26fe/compiler/rustc_target/src/spec/targets/i686_unknown_uefi.rs (L96)
[2]: https://gcc.gnu.org/onlinedocs/gcc/x86-Options.html
[3]: 9ba81c26fe/compiler/rustc_target/src/spec/targets/i686_unknown_linux_gnu.rs (L36)
[4]: 5ccb9629df/multiboot2/src/memory_map.rs (L5)
This reflects the status-quo and updates the outdated CONTRIBUTING.md.
- expectations on code style
- expectations on commit style
- expextations on AI/LLM-assisted contributions
Add a `char16!` macro that creates a const `Char16` value from a
character literal. This is nicer than using `Char16::from_u16_unchecked`
since it doesn't require an unsafe block and will trigger a compiler
error if the character is not valid in UCS-2.
Also, go through and replace uses of `Char16::from_u16_unchecked` and
`Char16::try_from` with `char16!` where appropriate.
Also, add `PartialEq` to `CharConversionError` to make writing unit
tests easier.
On 64-bit, there is implicit padding. On 32-bit targets, this is missing
and makes parsing the memory map impossible. The issue was found in an
external 32-bit integration test [0].
[0] https://github.com/rust-osdev/multiboot2/pull/225
