From ce2b9f5ec2c2523f7a8cbb19fd639784f0e6850d Mon Sep 17 00:00:00 2001 From: Augustin Cavalier Date: Wed, 2 Oct 2024 16:39:36 -0400 Subject: [PATCH] kernel/vm: Fix interaction between set_area_protection and set_memory_protection. * When the page_protections array is allocated, we should clear the protections from the area's flags, since they aren't used for anything when the page_protections array is activated. * When set_area_protection is called and there is a page_protections array in use, it should be freed, and we should reset the protections on all pages. * Add some tests related to these behaviors. --- src/system/kernel/vm/vm.cpp | 17 ++++- src/tests/system/kernel/mmap_cut_tests.cpp | 89 +++++++++++++++++++--- 2 files changed, 95 insertions(+), 11 deletions(-) diff --git a/src/system/kernel/vm/vm.cpp b/src/system/kernel/vm/vm.cpp index 76f72f0f3b..a55ffed6ef 100644 --- a/src/system/kernel/vm/vm.cpp +++ b/src/system/kernel/vm/vm.cpp @@ -546,8 +546,11 @@ allocate_area_page_protections(VMArea* area) // init the page protections for all pages to that of the area uint32 areaProtection = area->protection & (B_READ_AREA | B_WRITE_AREA | B_EXECUTE_AREA); - memset(area->page_protections, areaProtection | (areaProtection << 4), - bytes); + memset(area->page_protections, areaProtection | (areaProtection << 4), bytes); + + // clear protections from the area + area->protection &= ~(B_READ_AREA | B_WRITE_AREA | B_EXECUTE_AREA + | B_KERNEL_READ_AREA | B_KERNEL_WRITE_AREA | B_KERNEL_EXECUTE_AREA); return B_OK; } @@ -3031,6 +3034,16 @@ vm_set_area_protection(team_id team, area_id areaID, uint32 newProtection, } } while (restart); + if (area->page_protections != NULL) { + // Get rid of the per-page protections. + free_etc(area->page_protections, + area->address_space == VMAddressSpace::Kernel() ? HEAP_DONT_LOCK_KERNEL_SPACE : 0); + area->page_protections = NULL; + + // Assume the existing protections don't match the new ones. + isWritable = !becomesWritable; + } + bool changePageProtection = true; bool changeTopCachePagesOnly = false; diff --git a/src/tests/system/kernel/mmap_cut_tests.cpp b/src/tests/system/kernel/mmap_cut_tests.cpp index cf58bff398..61e2bcbc0c 100644 --- a/src/tests/system/kernel/mmap_cut_tests.cpp +++ b/src/tests/system/kernel/mmap_cut_tests.cpp @@ -50,7 +50,7 @@ map_cut_compare_test() int -map_protect_cut_test() +map_protect_cut_test1() { uint8* ptr = (uint8*)mmap(NULL, B_PAGE_SIZE * 4, PROT_NONE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); @@ -62,8 +62,9 @@ map_protect_cut_test() ptr[B_PAGE_SIZE * 3] = 'a'; // cut the area in the middle, before the accessible tail - mmap(ptr + B_PAGE_SIZE, B_PAGE_SIZE, PROT_READ | PROT_WRITE, - MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED, -1, 0); + if (mmap(ptr + B_PAGE_SIZE, B_PAGE_SIZE, PROT_READ | PROT_WRITE, + MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED, -1, 0) == NULL) + return -1; // validate that this does not crash if (ptr[B_PAGE_SIZE * 3] != 'a') { @@ -74,6 +75,73 @@ map_protect_cut_test() } +int +map_protect_cut_test2() +{ + uint8* ptr = (uint8*)mmap(NULL, B_PAGE_SIZE * 4, PROT_READ | PROT_WRITE, + MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); + + // store any value + ptr[B_PAGE_SIZE * 3] = 'a'; + + // make the tail un-accessible + if (mprotect(ptr + B_PAGE_SIZE * 3, B_PAGE_SIZE, PROT_NONE) != 0) + return -1; + + // cut the area in the middle, before the un-accessible tail + if (mmap(ptr + B_PAGE_SIZE, B_PAGE_SIZE, PROT_NONE, + MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED, -1, 0) == NULL) + return -1; + + // make the tail accessible again (changes commitment size) + if (mprotect(ptr + B_PAGE_SIZE * 3, B_PAGE_SIZE, PROT_READ | PROT_WRITE) != 0) + return -1; + + // validate that this does not crash + if (ptr[B_PAGE_SIZE * 3] != 'a') { + printf("map-protect-cut test failed!\n"); + return -1; + } + + // store another value + ptr[B_PAGE_SIZE * 3] = 'b'; + + // make the tail un-accessible again + if (mprotect(ptr + B_PAGE_SIZE * 3, B_PAGE_SIZE, PROT_NONE) != 0) + return -1; + + // clear page protections and reset to area protections + set_area_protection(area_for(ptr + B_PAGE_SIZE * 3), B_READ_AREA | B_WRITE_AREA); + + // validate that this does not crash + if (ptr[B_PAGE_SIZE * 3] != 'b') { + printf("map-protect-cut test failed!\n"); + return -1; + } + return 0; +} + + +int +map_cut_protect_test() +{ + uint8* ptr = (uint8*)mmap(NULL, B_PAGE_SIZE * 4, PROT_NONE, + MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); + + // cut the area in the middle + if (mmap(ptr + B_PAGE_SIZE, B_PAGE_SIZE, PROT_READ | PROT_WRITE, + MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED, -1, 0) == NULL) + return -1; + + // now make the tail accessible + mprotect(ptr + B_PAGE_SIZE * 3, B_PAGE_SIZE, PROT_READ | PROT_WRITE); + + // store any value + ptr[B_PAGE_SIZE * 3] = 'a'; + return 0; +} + + int map_cut_fork_test() { @@ -111,12 +179,9 @@ map_cut_fork_test() // validate that the fork does not crash the kernel int pid = fork(); - if (pid == 0) - { + if (pid == 0) { exit(0); - } - else if (pid < 0) - { + } else if (pid < 0) { printf("failed to fork the test process!\n"); return pid; } @@ -148,7 +213,13 @@ main() if ((status = map_cut_compare_test()) != 0) return status; - if ((status = map_protect_cut_test()) != 0) + if ((status = map_protect_cut_test1()) != 0) + return status; + + if ((status = map_protect_cut_test2()) != 0) + return status; + + if ((status = map_cut_protect_test()) != 0) return status; if ((status = map_cut_fork_test()) != 0)