RGBCube/serenity
1
Fork 0
mirror of https://github.com/RGBCube/serenity synced 2025-05-28 03:35:09 +00:00
Code Issues Activity Actions

Kernel: Fix UB caused by taking a reference to a packed struct's member

Browse source 
Taking a reference or a pointer to a value that's not aligned properly
is undefined behavior. While `[[gnu::packed]]` ensures that reads from
and writes to fields of packed structs is a safe operation, the
information about the reduced alignment is lost when creating pointers
to these values.

Weirdly enough, GCC's undefined behavior sanitizer doesn't flag these,
even though the doc of `-Waddress-of-packed-member` says that it usually
leads to UB. In contrast, x86_64 Clang does flag these, which renders
the 64-bit kernel unable to boot.

For now, the `address-of-packed-member` warning will only be enabled in
the kernel, as it is absolutely crucial there because of KUBSAN, but
might get excessively noisy for the userland in the future.

Also note that we can't append to `CMAKE_CXX_FLAGS` like we do for other
flags in the kernel, because flags added via `add_compile_options` come
after these, so the `-Wno-address-of-packed-member` in the root would
cancel it out.
This commit is contained in:
Daniel Bertalan 2021-08-01 20:30:43 +02:00 • committed by Andreas Kling
parent 90caebe96a
commit fa8507d1ce
3 changed files with 45 additions and 25 deletions
Download patch file Download diff file Expand all files Collapse all files

34
Kernel/Memory/MemoryManager.cpp
View file

@ -218,10 +218,18 @@ UNMAP_AFTER_INIT void MemoryManager::parse_memory_map()
Vector<ContiguousPhysicalVirtualRange> contiguous_physical_ranges;
for (auto* mmap = mmap_begin; mmap < mmap_end; mmap++) {
dmesgln("MM: Multiboot mmap: address={:p}, length={}, type={}", mmap->addr, mmap->len, mmap->type);
auto start_address = PhysicalAddress(mmap->addr);
// We have to copy these onto the stack, because we take a reference to these when printing them out,
// and doing so on a packed struct field is UB.
auto address = mmap->addr;
auto length = mmap->len;
ArmedScopeGuard write_back_guard = [&]() {
mmap->addr = address;
mmap->len = length;
};
dmesgln("MM: Multiboot mmap: address={:p}, length={}, type={}", address, length, mmap->type);
auto start_address = PhysicalAddress(address);
switch (mmap->type) {
case (MULTIBOOT_MEMORY_AVAILABLE):
m_physical_memory_ranges.append(PhysicalMemoryRange { PhysicalMemoryRangeType::Usable, start_address, length });
@ -249,23 +257,23 @@ UNMAP_AFTER_INIT void MemoryManager::parse_memory_map()
continue;
// Fix up unaligned memory regions.
auto diff = (FlatPtr)mmap->addr % PAGE_SIZE;
auto diff = (FlatPtr)address % PAGE_SIZE;
if (diff != 0) {
dmesgln("MM: Got an unaligned physical_region from the bootloader; correcting {:p} by {} bytes", mmap->addr, diff);
dmesgln("MM: Got an unaligned physical_region from the bootloader; correcting {:p} by {} bytes", address, diff);
diff = PAGE_SIZE - diff;
mmap->addr += diff;
mmap->len -= diff;
address += diff;
length -= diff;
}
if ((mmap->len % PAGE_SIZE) != 0) {
dmesgln("MM: Got an unaligned physical_region from the bootloader; correcting length {} by {} bytes", mmap->len, mmap->len % PAGE_SIZE);
mmap->len -= mmap->len % PAGE_SIZE;
if ((length % PAGE_SIZE) != 0) {
dmesgln("MM: Got an unaligned physical_region from the bootloader; correcting length {} by {} bytes", length, length % PAGE_SIZE);
length -= length % PAGE_SIZE;
}
if (mmap->len < PAGE_SIZE) {
dmesgln("MM: Memory physical_region from bootloader is too small; we want >= {} bytes, but got {} bytes", PAGE_SIZE, mmap->len);
if (length < PAGE_SIZE) {
dmesgln("MM: Memory physical_region from bootloader is too small; we want >= {} bytes, but got {} bytes", PAGE_SIZE, length);
continue;
}
for (PhysicalSize page_base = mmap->addr; page_base <= (mmap->addr + mmap->len); page_base += PAGE_SIZE) {
for (PhysicalSize page_base = address; page_base <= (address + length); page_base += PAGE_SIZE) {
auto addr = PhysicalAddress(page_base);
// Skip used memory ranges.