From 4986a9a3fd3fd37e29dbd35a4b138c6e93ffb454 Mon Sep 17 00:00:00 2001 From: Michael Lotz Date: Sun, 24 May 2020 22:14:09 +0200 Subject: [PATCH] Revert "kernel: Remove the B_KERNEL_AREA protection flag." This reverts parts of hrev52546 that removed the B_KERNEL_AREA protection flag and replaced it with an address space comparison. Checking for areas in the kernel address space inside a user address space does not work, as areas can only ever belong to one address space. This rendered these checks ineffective and allowed to unmap, delete or resize kernel managed areas from their respective userland teams. That protection was meant to be applied to the team user data area which was introduced to reduce the kernel to userland overhead by directly sharing some data between the two. It was intended to be set up in such a manner that this is safe on the kernel side and the B_KERNEL_AREA flag was introduced specifically for this purpose. Incidentally the actual application of the B_KERNEL_AREA flag on the team user data area was apparently forgotten in the original commit. The absence of that protection allowed applications to induce KDLs by modifying the user area and generating a signal for example. This change restores the B_KERNEL_AREA flag and also applies it to the team user data area. Change-Id: I993bb1cf7c6ae10085100db7df7cc23fe66f4edd Reviewed-on: https://review.haiku-os.org/c/haiku/+/2836 Reviewed-by: waddlesplash --- headers/private/system/vm_defs.h | 3 +++ .../debugger/user_interface/util/UiUtils.cpp | 1 + src/system/kernel/commpage.cpp | 2 +- src/system/kernel/team.cpp | 3 ++- src/system/kernel/vm/vm.cpp | 20 +++++++++++++------ 5 files changed, 21 insertions(+), 8 deletions(-) diff --git a/headers/private/system/vm_defs.h b/headers/private/system/vm_defs.h index fa318288e7..eefa895e60 100644 --- a/headers/private/system/vm_defs.h +++ b/headers/private/system/vm_defs.h @@ -23,6 +23,9 @@ // flags region in the protection field. #define B_OVERCOMMITTING_AREA (1 << 12) #define B_SHARED_AREA (1 << 13) +#define B_KERNEL_AREA (1 << 14) + // Usable from userland according to its protection flags, but the area + // itself is not deletable, resizable, etc from userland. #define B_USER_AREA_FLAGS \ (B_USER_PROTECTION | B_OVERCOMMITTING_AREA | B_CLONEABLE_AREA) diff --git a/src/kits/debugger/user_interface/util/UiUtils.cpp b/src/kits/debugger/user_interface/util/UiUtils.cpp index 5e65d72c8c..848981f3a8 100644 --- a/src/kits/debugger/user_interface/util/UiUtils.cpp +++ b/src/kits/debugger/user_interface/util/UiUtils.cpp @@ -230,6 +230,7 @@ UiUtils::AreaProtectionFlagsToString(uint32 protection, BString& _output) ADD_AREA_FLAG_IF_PRESENT(B_OVERCOMMITTING_AREA, protection, _output, "o", ""); ADD_AREA_FLAG_IF_PRESENT(B_SHARED_AREA, protection, "S", _output, ""); + ADD_AREA_FLAG_IF_PRESENT(B_KERNEL_AREA, protection, "k", _output, ""); if (protection != 0) { char buffer[32]; diff --git a/src/system/kernel/commpage.cpp b/src/system/kernel/commpage.cpp index 4aa05f6640..d895feb001 100644 --- a/src/system/kernel/commpage.cpp +++ b/src/system/kernel/commpage.cpp @@ -66,7 +66,7 @@ clone_commpage_area(team_id team, void** address) if (*address == NULL) *address = (void*)KERNEL_USER_DATA_BASE; return vm_clone_area(team, "commpage", address, - B_RANDOMIZED_BASE_ADDRESS, B_READ_AREA | B_EXECUTE_AREA, + B_RANDOMIZED_BASE_ADDRESS, B_READ_AREA | B_EXECUTE_AREA | B_KERNEL_AREA, REGION_PRIVATE_MAP, sCommPageArea, true); } diff --git a/src/system/kernel/team.cpp b/src/system/kernel/team.cpp index e5f159de18..ec5339a5ae 100644 --- a/src/system/kernel/team.cpp +++ b/src/system/kernel/team.cpp @@ -1378,7 +1378,8 @@ create_team_user_data(Team* team, void* exactAddress = NULL) physical_address_restrictions physicalRestrictions = {}; team->user_data_area = create_area_etc(team->id, "user area", - kTeamUserDataInitialSize, B_FULL_LOCK, B_READ_AREA | B_WRITE_AREA, 0, 0, + kTeamUserDataInitialSize, B_FULL_LOCK, + B_READ_AREA | B_WRITE_AREA | B_KERNEL_AREA, 0, 0, &virtualRestrictions, &physicalRestrictions, &address); if (team->user_data_area < 0) return team->user_data_area; diff --git a/src/system/kernel/vm/vm.cpp b/src/system/kernel/vm/vm.cpp index ecf90c5bc8..105bfc8181 100644 --- a/src/system/kernel/vm/vm.cpp +++ b/src/system/kernel/vm/vm.cpp @@ -822,7 +822,7 @@ unmap_address_range(VMAddressSpace* addressSpace, addr_t address, addr_t size, VMArea* area = it.Next();) { addr_t areaLast = area->Base() + (area->Size() - 1); if (area->Base() < lastAddress && address < areaLast) { - if (area->address_space == VMAddressSpace::Kernel()) { + if ((area->protection & B_KERNEL_AREA) != 0) { dprintf("unmap_address_range: team %" B_PRId32 " tried to " "unmap range of kernel area %" B_PRId32 " (%s)\n", team_get_current_team_id(), area->id, area->name); @@ -2147,6 +2147,9 @@ vm_clone_area(team_id team, const char* name, void** address, if (status != B_OK) return status; + if (!kernel && (sourceArea->protection & B_KERNEL_AREA) != 0) + return B_NOT_ALLOWED; + sourceArea->protection |= B_SHARED_AREA; protection |= B_SHARED_AREA; } @@ -2172,6 +2175,9 @@ vm_clone_area(team_id team, const char* name, void** address, if (sourceArea == NULL) return B_BAD_VALUE; + if (!kernel && (sourceArea->protection & B_KERNEL_AREA) != 0) + return B_NOT_ALLOWED; + VMCache* cache = vm_area_get_locked_cache(sourceArea); if (!kernel && sourceAddressSpace != targetAddressSpace @@ -2348,8 +2354,8 @@ vm_delete_area(team_id team, area_id id, bool kernel) cacheLocker.Unlock(); - // SetFromArea will have returned an error if the area's owning team is not - // the same as the passed team, so we don't need to do those checks here. + if (!kernel && (area->protection & B_KERNEL_AREA) != 0) + return B_NOT_ALLOWED; delete_area(locker.AddressSpace(), area, false); return B_OK; @@ -2633,7 +2639,8 @@ vm_set_area_protection(team_id team, area_id areaID, uint32 newProtection, cacheLocker.SetTo(cache, true); // already locked - if (!kernel && area->address_space == VMAddressSpace::Kernel()) { + if (!kernel && (area->address_space == VMAddressSpace::Kernel() + || (area->protection & B_KERNEL_AREA) != 0)) { dprintf("vm_set_area_protection: team %" B_PRId32 " tried to " "set protection %#" B_PRIx32 " on kernel area %" B_PRId32 " (%s)\n", team, newProtection, areaID, area->name); @@ -5065,7 +5072,8 @@ vm_resize_area(area_id areaID, size_t newSize, bool kernel) cacheLocker.SetTo(cache, true); // already locked // enforce restrictions - if (!kernel && area->address_space == VMAddressSpace::Kernel()) { + if (!kernel && (area->address_space == VMAddressSpace::Kernel() + || (area->protection & B_KERNEL_AREA) != 0)) { dprintf("vm_resize_area: team %" B_PRId32 " tried to " "resize kernel area %" B_PRId32 " (%s)\n", team_get_current_team_id(), areaID, area->name); @@ -6542,7 +6550,7 @@ _user_set_memory_protection(void* _address, size_t size, uint32 protection) if (area == NULL) return B_NO_MEMORY; - if (area->address_space == VMAddressSpace::Kernel()) + if ((area->protection & B_KERNEL_AREA) != 0) return B_NOT_ALLOWED; // TODO: For (shared) mapped files we should check whether the new