SerenityOS / serenity

The Serenity Operating System 🐞
https://serenityos.org
BSD 2-Clause "Simplified" License
30.65k stars 3.19k forks source link

Kernel memory corruption: Global variables crossing into kalloc pool #3257

Closed tomuta closed 4 years ago

tomuta commented 4 years ago

Trying to figure out some odd bugs related to #3229, I found that global variables are crossing into ETERNAL_BASE_PHYSICAL, which causes random memory corruption. One issue similar (though I don't believe the same) is related to the kernel command line getting corrupted, which is solved in a commit in #3248. However, I'm not sure how to solve this issue deterministically. It seems like BASE_PHYSICAL and ETERNAL_BASE_PHYSICAL need to account for global variables and other stuff. Maybe it should start no lower than end_of_kernel_image?

This is an example of what can happen:

Thread 1 hit Hardware watchpoint 2: *0xc0200000

Old value = 1
New value = 3
AK::Bitmap::set_range () at .././AK/Bitmap.h:100
100         for (size_t index = start; index < start + len; ++index) {
(gdb) bt
#0  AK::Bitmap::set_range () at .././AK/Bitmap.h:100
#1  0xc01a1c2a in kmalloc_allocate () at ../Kernel/Heap/kmalloc.cpp:130
#2  kmalloc_impl () at ../Kernel/Heap/kmalloc.cpp:184
#3  0xc012f989 in Kernel::Ext2FS::get_inode () at ../Kernel/FileSystem/Ext2FileSystem.cpp:653
#4  0xc012fe49 in Kernel::Ext2FSInode::lookup ()
    at ../Kernel/FileSystem/Ext2FileSystem.cpp:1501
#5  0xc0144ea8 in Kernel::VFS::resolve_path_without_veil ()
    at ../Kernel/FileSystem/VirtualFileSystem.cpp:933
#6  0xc014646d in Kernel::VFS::resolve_path ()
    at ../Kernel/FileSystem/VirtualFileSystem.cpp:885
#7  0xc0147077 in Kernel::VFS::access () at ../Kernel/FileSystem/VirtualFileSystem.cpp:441
#8  0xc016780c in Kernel::Process::sys$access () at ../Kernel/Syscalls/access.cpp:39
#9  0xc0167537 in handle () at ../Kernel/Syscall.cpp:118
#10 syscall_handler () at ../Kernel/Syscall.cpp:177
#11 0xc0166f6d in syscall_asm_entry ()
#12 0xc3965fac in ?? ()

In this case, 0xc0200000 is the first slab in the 16 byte allocator, which happened to be allocated to a PhysicalPage, but this call to kmalloc corrupted it because the global variable alloc_bitmap happened to be located at 0xc01ff200-0xc0202200.

tomuta commented 4 years ago

Tried this in kmalloc.cpp, but for some reason end_of_kernel_bss is always 0?

extern u32 end_of_kernel_bss;
#define BASE_PHYSICAL ((u8*)PAGE_ROUND_UP(end_of_kernel_bss))
#define ETERNAL_BASE_PHYSICAL (BASE_PHYSICAL + POOL_SIZE)
tomuta commented 4 years ago

Doh, it has to be &end_of_kernel_bss, not end_of_kernel_bss...