Kernel: sys$msync fuzzer crash with large enough size parameter.

[bgianfo]

Happened across this today while playing with fuzz-syscalls:

217.439 fuzz-syscalls(75:75): #164: Calling msync(0x00000000, 0xffffffff, 0x00000001) with 0x3bd24000 containing [0xbffff000, 0x3bd24000, 0x3bd24000, 0x00000000, 0xc0000000, 0x3bd24000, 0x3bd24000, 0x0000000d, 0x7c7928c1, 0xc0000000]
[fuzz-syscalls(75:75)]: ASSERTION FAILED: x == 0 || rounded != 0
[fuzz-syscalls(75:75)]: ././Kernel/Memory/MemoryManager.h:36 in constexpr FlatPtr Kernel::Memory::page_round_up(FlatPtr)
[fuzz-syscalls(75:75)]: KERNEL PANIC! :^(
[fuzz-syscalls(75:75)]: Aborted
[fuzz-syscalls(75:75)]: at ./Kernel/Arch/x86/common/CPU.cpp:35 in void abort()
[fuzz-syscalls(75:75)]: Kernel + 0x00c08a81  Kernel::__panic(char const*, unsigned int, char const*) +0xf1
[fuzz-syscalls(75:75)]: Kernel + 0x00fa5ab9  abort.localalias +0x245
[fuzz-syscalls(75:75)]: Kernel + 0x00fa5874  abort.localalias +0x0
[fuzz-syscalls(75:75)]: Kernel + 0x00d81a0c  Kernel::Process::sys$msync(AK::Userspace<void*>, unsigned long, int) +0x46c
[fuzz-syscalls(75:75)]: Kernel + 0x00cd0831  syscall_handler +0x1f31
[fuzz-syscalls(75:75)]: Kernel + 0x00cce8f1  syscall_asm_entry +0x31

So we end up calling msync(0x00000000, 0xffffffff, 0x00000001), and we correctly detect that page_round_up() on the size would overflow. However we should return an error, not crash the Kernel, :smile:.

Looks like we need to add a way so this function can propagate error instead of asserting:

constexpr FlatPtr page_round_up(FlatPtr x)
{
    FlatPtr rounded = (((FlatPtr)(x)) + PAGE_SIZE - 1) & (~(PAGE_SIZE - 1));
    // Rounding up >0xfffff000 wraps back to 0. That's never what we want.
    VERIFY(x == 0 || rounded != 0);
    return rounded;
}

[BenWiederhake]

I'm positively baffled that this "stupid" fuzzer still finds bugs. :D

[eggpi]

Hi, new contributor here! I thought I'd look into this one if that's OK.

The easy fix is probably to call page_round_up_would_wrap before page_round_up in msync, like mmap does. However, there are a number of other calls to page_round_up that are not guarded by page_round_up_would_wrap (e.g. KBuffer.h), and I'm not sure those are safe.

I wanted to double check how comprehensive we want to be here. I suppose the options are:

1. Fix only msync.
2. Ensure every page_round_up is guarded by page_round_up_would_wrap.
3. Allow page_round_up to propagate an error, update every caller and possibly obsolete page_round_up_would_wrap.

Thoughts? I'm happy to open a PR for any of those approaches!

[bgianfo]

3 seems like the right way path forward for a robust system, especially for all cases where the data is controlled by user mode. I could imagine there being a lot of cases in the kernel that would make that hard today though. Maybe it makes sense to fix msync first and follow up with fixing the rest in a later PR?

[eggpi]

Agreed and thanks! I've opened a PR for option 1, and I can look more deeply into option 3 over the next few days. I'm guessing I can file a separate issue to track the further work (so my current PR is set to resolve this one), but let me know if you just want to reuse this.