Changeset View
Standalone View
sys/vm/vm_map.c
Show First 20 Lines • Show All 1,665 Lines • ▼ Show 20 Lines | KASSERT((prot & ~max) == 0, | ||||
("prot %#x is not subset of max_prot %#x", prot, max)); | ("prot %#x is not subset of max_prot %#x", prot, max)); | ||||
/* | /* | ||||
* Check that the start and end points are not bogus. | * Check that the start and end points are not bogus. | ||||
*/ | */ | ||||
if (start == end || !vm_map_range_valid(map, start, end)) | if (start == end || !vm_map_range_valid(map, start, end)) | ||||
return (KERN_INVALID_ADDRESS); | return (KERN_INVALID_ADDRESS); | ||||
if ((map->flags & MAP_WXORX) != 0 && (prot & (VM_PROT_WRITE | | |||||
VM_PROT_EXECUTE)) == (VM_PROT_WRITE | VM_PROT_EXECUTE)) | |||||
return (KERN_PROTECTION_FAILURE); | |||||
/* | /* | ||||
* Find the entry prior to the proposed starting address; if it's part | * Find the entry prior to the proposed starting address; if it's part | ||||
* of an existing entry, this range is bogus. | * of an existing entry, this range is bogus. | ||||
*/ | */ | ||||
if (vm_map_lookup_entry(map, start, &prev_entry)) | if (vm_map_lookup_entry(map, start, &prev_entry)) | ||||
return (KERN_NO_SPACE); | return (KERN_NO_SPACE); | ||||
/* | /* | ||||
▲ Show 20 Lines • Show All 1,063 Lines • ▼ Show 20 Lines | vm_map_protect(vm_map_t map, vm_offset_t start, vm_offset_t end, | ||||
int rv; | int rv; | ||||
if (start == end) | if (start == end) | ||||
return (KERN_SUCCESS); | return (KERN_SUCCESS); | ||||
again: | again: | ||||
in_tran = NULL; | in_tran = NULL; | ||||
vm_map_lock(map); | vm_map_lock(map); | ||||
if ((map->flags & MAP_WXORX) != 0 && (new_prot & | |||||
jhb: I do think it would be fairly simple to only do this for `!set_max` to exempt changes to… | |||||
Not Done Inline ActionsYes, agreed emaste: Yes, agreed | |||||
Done Inline Actionskern_mprotect() first does set_max call for max_prot, and then !set_max for active prot. But I do not believe that vm_map_protect() allows to change map entry protection to larger than was initially created. Look below for lines 2789-2791. Otherwise e.g. you could map readonly file, and then change protection on the mapping. kib: kern_mprotect() first does set_max call for max_prot, and then !set_max for active prot. But I… | |||||
Not Done Inline ActionsBut we should be able to set_max with RWX even with allow_wx=0 emaste: But we should be able to set_max with RWX even with allow_wx=0 | |||||
Done Inline ActionsAnd more, I believe that kern_mprotect() should be rewritten, that is vm_map_protect() should take whole prot arg and handle both max_prot and current prot in one locking pass, and in single check to avoid inconsistencies. kib: And more, I believe that kern_mprotect() should be rewritten, that is vm_map_protect() should… | |||||
Not Done Inline Actions
I see - I suspect @brooks wanted to avoid changing the vm_map_protect interface in D18880. emaste: > vm_map_protect() should take whole prot arg and handle both max_prot and current prot in one… | |||||
Done Inline ActionsI think there is no reason not to change vm_map_protect(). I will look at it shortly, but I do not believe this is in scope of this review. kib: I think there is no reason not to change vm_map_protect(). I will look at it shortly, but I do… | |||||
Not Done Inline Actions
I agree that it would be nicer to fix vm_map_protect(). I am fine with having that in a separate review. I do think we want the eventual state to be that prot_max isn't subject to the MAP_WXORX, but I'm ok with fixing that up later as part of fixing vm_map_protect(). jhb: > I think there is no reason not to change vm_map_protect(). I will look at it shortly, but I… | |||||
Not Done Inline ActionsIn regards to prot_max, I agree that it should not be subject to MAP_WXORX. Imagine a JIT compilation system that enables write access (but not execute access) during compilation, and then when compilation is finished removes write access and enables execute access. To support this, prot_max has to allow both write and execute access even if we don't allow the prot field to have write and execute access simultaneously enabled. alc: In regards to prot_max, I agree that it should not be subject to MAP_WXORX. Imagine a JIT… | |||||
(VM_PROT_WRITE | VM_PROT_EXECUTE)) == (VM_PROT_WRITE | | |||||
VM_PROT_EXECUTE)) { | |||||
vm_map_unlock(map); | |||||
return (KERN_PROTECTION_FAILURE); | |||||
} | |||||
/* | /* | ||||
* Ensure that we are not concurrently wiring pages. vm_map_wire() may | * Ensure that we are not concurrently wiring pages. vm_map_wire() may | ||||
* need to fault pages into the map and will drop the map lock while | * need to fault pages into the map and will drop the map lock while | ||||
* doing so, and the VM object may end up in an inconsistent state if we | * doing so, and the VM object may end up in an inconsistent state if we | ||||
* update the protection on the map entry in between faults. | * update the protection on the map entry in between faults. | ||||
*/ | */ | ||||
vm_map_wait_busy(map); | vm_map_wait_busy(map); | ||||
▲ Show 20 Lines • Show All 2,616 Lines • Show Last 20 Lines |
I do think it would be fairly simple to only do this for !set_max to exempt changes to max_prot from this. As it is, the current code allows PROT_MAX(RWX) for mmap() but not for mprotect() which I think is a bit odd. IIRC, mprotect() changes the non-max prot first, so a W^X mprotect() failure would not result in a partial update where only max_prot was changed but not prot.