Changeset View
Standalone View
sys/kern/kern_exec.c
Context not available. | |||||
#include "opt_capsicum.h" | #include "opt_capsicum.h" | ||||
#include "opt_hwpmc_hooks.h" | #include "opt_hwpmc_hooks.h" | ||||
#include "opt_ktrace.h" | #include "opt_ktrace.h" | ||||
#include "opt_pax.h" | |||||
#include "opt_vm.h" | #include "opt_vm.h" | ||||
#include <sys/param.h> | #include <sys/param.h> | ||||
Context not available. | |||||
#include <sys/wait.h> | #include <sys/wait.h> | ||||
#include <sys/malloc.h> | #include <sys/malloc.h> | ||||
#include <sys/priv.h> | #include <sys/priv.h> | ||||
#include <sys/pax.h> | |||||
#include <sys/proc.h> | #include <sys/proc.h> | ||||
#include <sys/pioctl.h> | #include <sys/pioctl.h> | ||||
#include <sys/namei.h> | #include <sys/namei.h> | ||||
rwatson: MAC-policy headers should never be included outside of their specific policy modules; only… | |||||
Not Done Inline ActionsShouldn't this non-static function prototype be in a header? rwatson: Shouldn't this non-static function prototype be in a header? | |||||
Not Done Inline ActionsIt can be made static. lattera-gmail.com: It can be made static. | |||||
Not Done Inline ActionsWhat is CACM 56(2)? lattera-gmail.com: What is CACM 56(2)? | |||||
Not Done Inline ActionsRobert N. M. Watson. A decade of OS access-control extensibility. Communications of the ACM 56(2), February 2013. Robert N. M. Watson. A decade of OS access-control extensibility. ACM Queue 11(1), January 2013. (Open access, extended version of CACM article.) rwatson: Robert N. M. Watson. A decade of OS access-control extensibility. Communications of the ACM 56… | |||||
Not Done Inline ActionsIf it's not referenced outside of the file and isn't 'public', then it should be static. rwatson: If it's not referenced outside of the file and isn't 'public', then it should be static. | |||||
Context not available. | |||||
imgp->pagesizes = 0; | imgp->pagesizes = 0; | ||||
imgp->pagesizeslen = 0; | imgp->pagesizeslen = 0; | ||||
imgp->stack_prot = 0; | imgp->stack_prot = 0; | ||||
imgp->pax_flags = 0; | |||||
#if defined(PAX_ASLR) | |||||
pax_elf(imgp, 0); | |||||
#endif | |||||
Not Done Inline ActionsHow does PAX_MPROTECT get defined without PAX_ASLR? And if so, how does the prototype from sys/pax.h for pax_elf get defined in that case? imp: How does PAX_MPROTECT get defined without PAX_ASLR? And if so, how does the prototype from… | |||||
Not Done Inline ActionsI should have left the PAX_MPROTECT stuff out. We haven't actually started on that feature, but will be worked on in the future. lattera-gmail.com: I should have left the PAX_MPROTECT stuff out. We haven't actually started on that feature, but… | |||||
#ifdef MAC | #ifdef MAC | ||||
error = mac_execve_enter(imgp, mac_p); | error = mac_execve_enter(imgp, mac_p); | ||||
Context not available. | |||||
map = &vmspace->vm_map; | map = &vmspace->vm_map; | ||||
} | } | ||||
#ifdef PAX_ASLR | |||||
pax_aslr_init(curthread, imgp); | |||||
#endif | |||||
/* Map a shared page */ | /* Map a shared page */ | ||||
obj = sv->sv_shared_page_obj; | obj = sv->sv_shared_page_obj; | ||||
if (obj != NULL) { | if (obj != NULL) { | ||||
Not Done Inline ActionsAs an example, this #ifdef block can be unconditionally replaced by But you do not understand that exec not always reinitialize the vmspace, which causes weird reduction of the available stack size for deep chains of execed images. kib: As an example, this #ifdef block can be unconditionally replaced by
p->p_vmspace->vm_maxsaddr… | |||||
Not Done Inline ActionsThe placed #ifdef blocks are because performance decision. If someone choose to disable the ASLR completely, that this #ifdef blocks saves some CPU cycle. For the reinitialization, could you please create a simple test-case or proof of concept code? The deltas which corresponds to the newly execed image should be reinitialized in every exec calls, independently from reusing the previous vmspace or create a new one. This is an design decision. "In practice brute forcing can be applied to bugs that are in network daemons that fork() on each connection since fork() preserves the randomized layout, as opposed to execve() which replaces it with a new one." - http://pax.grsecurity.net/docs/aslr.txt op: The placed #ifdef blocks are because performance decision. If someone choose to disable the… | |||||
Context not available. | |||||
*/ | */ | ||||
vmspace->vm_ssize = sgrowsiz >> PAGE_SHIFT; | vmspace->vm_ssize = sgrowsiz >> PAGE_SHIFT; | ||||
vmspace->vm_maxsaddr = (char *)sv->sv_usrstack - ssiz; | vmspace->vm_maxsaddr = (char *)sv->sv_usrstack - ssiz; | ||||
#ifdef PAX_ASLR | |||||
vmspace->vm_maxsaddr -= vmspace->vm_aslr_delta_stack; | |||||
#endif | |||||
return (0); | return (0); | ||||
} | } | ||||
Context not available. | |||||
szsigcode = *(p->p_sysent->sv_szsigcode); | szsigcode = *(p->p_sysent->sv_szsigcode); | ||||
} | } | ||||
destp = (uintptr_t)arginfo; | destp = (uintptr_t)arginfo; | ||||
#ifdef PAX_ASLR | |||||
pax_aslr_stack(curthread, &destp); | |||||
#endif | |||||
/* | /* | ||||
* install sigcode | * install sigcode | ||||
Context not available. | |||||
Not Done Inline ActionsShouldn't the mac code be making this choice? imp: Shouldn't the mac code be making this choice? | |||||
Not Done Inline ActionsNo. MAC is not involved at all here. We had previously implemented mac_bsdextended/ugidfw integration and used mac_bsdextended's main header as the integration point for per-binary flags. However, the mac_bsdxtended integration has been left out due to concerns other FreBSD developers had that were indirectly brought to our attention. lattera-gmail.com: No. MAC is not involved at all here. We had previously implemented mac_bsdextended/ugidfw… | |||||
Not Done Inline ActionsMAC is also a source of exec-time credential change. If you need to reuse the credential-changing decision, it needs to be passed safely from the authoritative context to this one -- e.g., via image_params. Otherwise you may miss instances of credential change and/or suffer race conditions. rwatson: MAC is also a source of exec-time credential change. If you need to reuse the credential… | |||||
Not Done Inline ActionsAvoid using pointers as booleans; instead, compare explicitly with NULL. rwatson: Avoid using pointers as booleans; instead, compare explicitly with NULL. | |||||
Not Done Inline ActionsThis credential changing logic was copied literally from elsewhere in this file. Should that code be modified to use MAC as part of this ASLR change? lattera-gmail.com: This credential changing logic was copied literally from elsewhere in this file. Should that… | |||||
Not Done Inline ActionsWill address with the next patch. lattera-gmail.com: Will address with the next patch. | |||||
Not Done Inline ActionsCan you instead eliminate the copying by making the choice once and then caching the result? Making the decision twice opens the opportunity for race conditions, locking problems, errors in updating the code in the future, etc. E.g.: Mac OS X contains a fix to differentiate "credential change" from "can't debug" for MAC, which we should likely pick up so that informational labelling schemes don't break debugging. When we make that change, it would be nice to have to make it in only one place. rwatson: Can you instead eliminate the copying by making the choice once and then caching the result? | |||||
Not Done Inline ActionsYou continue to ignore my request that this logic should *not* be copied and pasted, but instead be centralised -- ideally with the decision made once, atomically, and cached, rather than in multiple places, non-atomically. This is for both functionality and code maintenance reasons. rwatson: You continue to ignore my request that this logic should *not* be copied and pasted, but… | |||||
Not Done Inline ActionsI have pointed out in prior reviews that you have incorrectly replicated credential-changing logic from elsewhere. It is still wrong. Please centralise the logic so that it doesn't appear in multiple places, as I requested before, and make sure you use the right version. rwatson: I have pointed out in prior reviews that you have incorrectly replicated credential-changing… | |||||
Not Done Inline ActionsThis local variable 'error' is unnecessary -- just return (pax_elf(...)). rwatson: This local variable 'error' is unnecessary -- just return (pax_elf(...)). | |||||
Not Done Inline ActionsThe next patch will include MAC integration into this logic. However, the spot where I got the credential changing logic from needs additional details. There's no way to remove code duplication. If you'd like to see a different approach than the one I take with the next patch, please provide instruction on how to do so and I'd be happy to make the change. lattera-gmail.com: The next patch will include MAC integration into this logic. However, the spot where I got the… |
MAC-policy headers should never be included outside of their specific policy modules; only framework headers should be visible to the kernel as a whole. This is an explicit and critical KBI design choice: please see the article in CACM 56(2) for why this is the case.