Overallocation bug in "valloc" #1

New issue

Closed

opened 2025-06-14 14:09:43 +00:00 by Raphaël M. · 1 comment

Raphaël M. commented

2025-06-14 14:09:43 +00:00

     while (region) {
         if (region->next == NULL ||
             region->start + (region->pages * PAGE_SIZE) < region->next->start) {
             new = (vregion_t*)palloc(1, true);
             if (!new)
                 return NULL;
             memset(new, 0, sizeof(vregion_t));
             new->pages = pages;
             new->flags = VFLAGS_TO_PFLAGS(flags);
             new->start = region->start + (region->pages * PAGE_SIZE);
             new->next = region->next;
             new->prev = region;
             region->next = new;
             for (uint64_t i = 0; i < pages; i++) {
                 uint64_t page = (uint64_t)palloc(1, false);
                 if (page == 0)
                     return NULL;
                 vmap(ctx->pagemap, new->start + (i * PAGE_SIZE), page,
                      new->flags);
             }
             return (void*)new->start;
         }
         region = region->next;
     }

At line 49, it checks if the next region is NULL, or if the region's start address + the region's size is smaller than the next region, and it only check if there's a gap between region->start + region->size and region->next->start, but not if the gap is big enough to fit the requested number of pages in.

For example: let's say the user allocated 3 memory blocks of 1 page each from 0x10000 to 0x12000 and then freed the memory block at 0x11000. If the user requests a memory block of 3 pages, it will check if there's a gap between 0x10000 and 0x12000, and then will proceed to allocate those 3 blocks. But there's only a gap of 1 page, not 3, and so, the VMA will overwrite 0x12000 to be part of the requested 3 pages.

Here's a demo of the VMA bug:

We can see from the line vma: vaddr 15000, vflags 3 -> pflags 3, pm: ... that ramfs_lookup allocated a new vnode at 0x15000. And when vma_alloc was called to allocate a 12 page-sized buffer, and found that there's a free region at 0x13000, it didn't check if there was enough space, and overwrote the vnode, resulting in a vfs_read error: Read -1 bytes from /dir/hey:

The requested fix

The fix would be that instead of checking if there's a gap between 2 regions, like defined here at line 49 of vmm.c:

        if (region->next == NULL ||
            region->start + (region->pages * PMM_PAGE_SIZE) < region->next->start) {

, it checks if the region is big enough to hold pages, like in the now patched version in Soaplin:

        if (region->next == NULL ||
            region->start + (pages * PMM_PAGE_SIZE) < region->next->start) {

https://git.piraterna.org/EMK/emk/src/commit/9231fabb0cadcba2922e527da8ae325f93c9ecc3/kernel/src/mm/vmm.c#L48-L73 At line 49, it checks if the next region is NULL, or if **the region's start address + the region's size** is smaller than the next region, and it only check if there's a gap between `region->start + region->size` and `region->next->start`, but not if the gap is big enough to fit the requested number of pages in. For example: let's say the user allocated 3 memory blocks of 1 page each from `0x10000` to `0x12000` and then freed the memory block at `0x11000`. If the user requests a memory block of 3 pages, it will check if there's a gap between 0x10000 and 0x12000, and then will proceed to allocate those 3 blocks. But there's only a gap of 1 page, not 3, and so, the VMA will overwrite 0x12000 to be part of the requested 3 pages. Here's a demo of the VMA bug: ![image](/attachments/f2af6e7f-72e0-4773-8fce-813252261900) We can see from the line `vma: vaddr 15000, vflags 3 -> pflags 3, pm: ...` that ramfs_lookup allocated a new vnode at 0x15000. And when vma_alloc was called to allocate a 12 page-sized buffer, and found that there's a free region at 0x13000, it didn't check if there was enough space, and overwrote the vnode, resulting in a vfs_read error: `Read -1 bytes from /dir/hey: ` ## The requested fix The fix would be that instead of checking if there's a gap between 2 regions, like defined here at line 49 of vmm.c: ```c if (region->next == NULL || region->start + (region->pages * PMM_PAGE_SIZE) < region->next->start) { ``` , it checks if the region is big enough to hold `pages`, like in the now patched version in Soaplin: ```c if (region->next == NULL || region->start + (pages * PMM_PAGE_SIZE) < region->next->start) { ```

image.png

55 KiB