Changeset View
Standalone View
sys/kern/kern_pax.c
- This file was added.
/*- | |||||
* Copyright (c) 2006 Elad Efrat <elad@NetBSD.org> | |||||
* Copyright (c) 2013-2014, by Oliver Pinter <oliver.pntr at gmail.com> | |||||
* Copyright (c) 2014, by Shawn Webb <lattera at gmail.com> | |||||
* All rights reserved. | |||||
* | |||||
* Redistribution and use in source and binary forms, with or without | |||||
* modification, are permitted provided that the following conditions | |||||
* are met: | |||||
* 1. Redistributions of source code must retain the above copyright | |||||
* notice, this list of conditions and the following disclaimer. | |||||
* 2. Redistributions in binary form must reproduce the above copyright | |||||
* notice, this list of conditions and the following disclaimer in the | |||||
* documentation and/or other materials provided with the distribution. | |||||
* 3. The name of the author may not be used to endorse or promote products | |||||
* derived from this software without specific prior written permission. | |||||
* | |||||
* THIS SOFTWARE IS PROVIDED BY THE AUTHOR ``AS IS'' AND ANY EXPRESS OR | |||||
* IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES | |||||
emaste: Can you find out if it's possible to switch the disclaimer block to the new standard text… | |||||
* OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. | |||||
Not Done Inline ActionsI would also like to see this change. rwatson: I would also like to see this change. | |||||
* IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT, | |||||
* INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT | |||||
* NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, | |||||
* DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY | |||||
* THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT | |||||
* (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF | |||||
* THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. | |||||
* | |||||
* $FreeBSD$ | |||||
*/ | |||||
#include <sys/cdefs.h> | |||||
__FBSDID("$FreeBSD$"); | |||||
Not Done Inline ActionsRemove extra line. rwatson: Remove extra line. | |||||
#include "opt_compat.h" | |||||
#include "opt_pax.h" | |||||
#include <sys/param.h> | |||||
#include <sys/systm.h> | |||||
#include <sys/kernel.h> | |||||
#include <sys/imgact.h> | |||||
#include <sys/imgact_elf.h> | |||||
#include <sys/lock.h> | |||||
#include <sys/mutex.h> | |||||
#include <sys/sysent.h> | |||||
#include <sys/stat.h> | |||||
#include <sys/proc.h> | |||||
#include <sys/elf_common.h> | |||||
#include <sys/mount.h> | |||||
#include <sys/sysctl.h> | |||||
#include <sys/vnode.h> | |||||
#include <sys/queue.h> | |||||
#include <sys/libkern.h> | |||||
#include <sys/jail.h> | |||||
#include <sys/mman.h> | |||||
#include <sys/libkern.h> | |||||
#include <sys/exec.h> | |||||
#include <sys/kthread.h> | |||||
#include <sys/syslimits.h> | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
#include <sys/param.h> | |||||
#include <vm/pmap.h> | |||||
#include <vm/vm_map.h> | |||||
#include <vm/vm_extern.h> | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
#include <machine/elf.h> | |||||
Not Done Inline ActionsThe above includes are very unsorted. Could they be more sorted? rwatson: The above includes are very unsorted. Could they be more sorted? | |||||
#include <sys/pax.h> | |||||
#include <security/mac_bsdextended/mac_bsdextended.h> | |||||
Not Done Inline ActionsCentral kernel code should never include MAC-policy-specific headers. See earlier comment about the design here. rwatson: Central kernel code should never include MAC-policy-specific headers. See earlier comment about… | |||||
Not Done Inline ActionsWill address with next patch. lattera-gmail.com: Will address with next patch. | |||||
SYSCTL_NODE(_security, OID_AUTO, pax, CTLFLAG_RD, 0, | |||||
"PaX (exploit mitigation) features."); | |||||
Not Done Inline ActionsCould this be included with prior <sys/> headers? rwatson: Could this be included with prior <sys/> headers? | |||||
const char *pax_status_str[] = { | |||||
[PAX_FEATURE_DISABLED] = "disabled", | |||||
[PAX_FEATURE_OPTIN] = "opt-in", | |||||
[PAX_FEATURE_OPTOUT] = "opt-out", | |||||
[PAX_FEATURE_FORCE_ENABLED] = "force enabled", | |||||
[PAX_FEATURE_UNKNOWN_STATUS] = "UNKNOWN -> changed to \"force enabled\"" | |||||
}; | |||||
const char *pax_status_simple_str[] = { | |||||
[PAX_FEATURE_SIMPLE_DISABLED] = "disabled", | |||||
[PAX_FEATURE_SIMPLE_ENABLED] = "enabled" | |||||
}; | |||||
struct prison * | |||||
pax_get_prison(struct proc *proc) | |||||
{ | |||||
Not Done Inline ActionsFirst off, did you encounter any threads without associated proc or proc without ucred? I could imagine this could be the case for kernel threads, but this does not seem likely. In other words, why can't this just require proc pointer? What guarantees that the prison will not be changed to something else? Are these guarantees needed? Note that multilevel jails are supported, so jail creation is no longer a trusted operation. With that in mind, what would happen in a multithreaded process using pax and jailing? Assume pax has different settings in a jail. What about concurrent pax-related calls (say mmap) vs jailing. Where there any cases when there was no prison? PROC_LOCK could ensure stability of creds for the entire processes across the call. So this functon should assert that PROC_LOCK is held. However, since this does not do anything aslr-specific, it should be moved somewhere as a separate helper. mjg: First off, did you encounter any threads without associated proc or proc without ucred? I could… | |||||
Not Done Inline ActionsKernel threads belong to proc0 or other kprocs, unjailed processes are in prison0, and the only time p_ucred can be NULL is during process creation. des: Kernel threads belong to proc0 or other kprocs, unjailed processes are in prison0, and the only… | |||||
Not Done Inline ActionsThis function makes me nervous -- one almost never wants p->p_proc, which requires the process lock to be held (which is not asserted here) -- we want td->td_proc. If you must use p->p_ucred, you should assert the lock here .. or do something very specific so that this is only used in execve() -- e.g., have the function static in kern_execve.c. rwatson: This function makes me nervous -- one almost never wants p->p_proc, which requires the process… | |||||
if ((proc == NULL) || (proc->p_ucred == NULL)) | |||||
Not Done Inline ActionsThis comment appears not to have been responded to / addressed. rwatson: This comment appears not to have been responded to / addressed. | |||||
return (NULL); | |||||
return (proc->p_ucred->cr_prison); | |||||
} | |||||
int | |||||
Not Done Inline ActionsWhen any of these can be NULL? mjg: When any of these can be NULL? | |||||
pax_get_flags(struct proc *proc, uint32_t *flags) | |||||
{ | |||||
Not Done Inline ActionsBlock comments should be sentences -- add a '.' at the end. rwatson: Block comments should be sentences -- add a '.' at the end. | |||||
*flags = 0; | |||||
if (proc != NULL) | |||||
Not Done Inline ActionsAssert process lock or comment on why it is not required to dereference p->p_ucred, which is not in general safe. rwatson: Assert process lock or comment on why it is not required to dereference p->p_ucred, which is… | |||||
*flags = proc->p_pax; | |||||
else | |||||
return (1); | |||||
Not Done Inline Actionsgiven the small range of the jump here, and the linear nature of the code, goto likely isn't the right tool. I'd just reverse the sense of the if and put the next if/elseif clause inside that... imp: given the small range of the jump here, and the linear nature of the code, goto likely isn't… | |||||
Not Done Inline ActionsTrue. The code underwent a lot of changes in which a few goto statements on error conditions were needed. I can certainly change that. I'll do something more like: if ((mode & MBI_ALLPAX) != MBI_ALLPAX) { lattera-gmail.com: True. The code underwent a lot of changes in which a few goto statements on error conditions… | |||||
return (0); | |||||
Not Done Inline ActionsSimilarly, when is this called with NULL proc? mjg: Similarly, when is this called with NULL proc? | |||||
} | |||||
int | |||||
pax_elf(struct image_params *imgp, uint32_t mode) | |||||
{ | |||||
Not Done Inline ActionsAssert (td == curthread) or otherwise assert a suitable lock for safety in dereferencing td->td_ucred? rwatson: Assert (td == curthread) or otherwise assert a suitable lock for safety in dereferencing td… | |||||
u_int flags, flags_aslr; | |||||
flags = 0; | |||||
flags_aslr = 0; | |||||
if ((mode & MBI_ALLPAX) != MBI_ALLPAX) { | |||||
if (mode & MBI_ASLR_ENABLED) | |||||
flags |= PAX_NOTE_ASLR; | |||||
if (mode & MBI_ASLR_DISABLED) | |||||
flags |= PAX_NOTE_NOASLR; | |||||
} | |||||
Not Done Inline ActionsThere seem to be a lot of extra blank lines in this function, which should be removed (e.g., like the one here.) rwatson: There seem to be a lot of extra blank lines in this function, which should be removed (e.g. | |||||
Not Done Inline ActionsWill address in next patch. lattera-gmail.com: Will address in next patch. | |||||
if ((flags & ~PAX_NOTE_ALL) != 0) { | |||||
printf("%s: unknown paxflags: %x\n", __func__, flags); | |||||
Not Done Inline ActionsI find this if() statement confusing; it could be made more clear, as well as a comment added. rwatson: I find this if() statement confusing; it could be made more clear, as well as a comment added. | |||||
return (1); | |||||
} | |||||
Not Done Inline ActionsWhat's up with return values here? Elsewhere you do: error = pax_elf(imgp, 0); goto exec_fail; 1 happens to be EPERM which may or may not be what a bad choice. Another question is how one can get flags which don't pass validation at this point? mjg: What's up with return values here?
Elsewhere you do:
error = pax_elf(imgp, 0);
if (error)… | |||||
if (((flags & PAX_NOTE_ALL_ENABLED) & ((flags & PAX_NOTE_ALL_DISABLED) >> 1)) != 0) { | |||||
/* | |||||
* indicate flags inconsistencies in dmesg and in user terminal | |||||
*/ | |||||
printf("%s: inconsistent paxflags: %x\n", __func__, flags); | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
return (1); | |||||
} | |||||
#ifdef PAX_ASLR | |||||
flags_aslr = pax_aslr_setup_flags(imgp, mode); | |||||
#endif | |||||
flags = flags_aslr; | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
CTR3(KTR_PAX, "%s : flags = %x mode = %x", | |||||
__func__, flags, mode); | |||||
Not Done Inline ActionsElsewhere in the callgraph, you make assumptions about mutually exclusive use of proc fields. Is there a reason not to do that here? rwatson: Elsewhere in the callgraph, you make assumptions about mutually exclusive use of proc fields. | |||||
if (imgp != NULL) { | |||||
imgp->pax_flags = flags; | |||||
if (imgp->proc != NULL) { | |||||
PROC_LOCK(imgp->proc); | |||||
imgp->proc->p_pax = flags; | |||||
PROC_UNLOCK(imgp->proc); | |||||
} | |||||
} | |||||
Not Done Inline ActionsI checked and there are no callers which can give NULL imgp. Also pax_flags is a write-only variable. mjg: I checked and there are no callers which can give NULL imgp.
Also pax_flags is a write-only… | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
return (0); | |||||
Not Done Inline ActionsI don't think this needs a kernel printf() -- if PaX is to be an out-of-the-box feature, it should be taken for granted. rwatson: I don't think this needs a kernel printf() -- if PaX is to be an out-of-the-box feature, it… | |||||
Not Done Inline ActionsKernel printf() is rarely the right way to report an error -- why does it need to be used here? rwatson: Kernel printf() is rarely the right way to report an error -- why does it need to be used here? | |||||
} | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
/* | |||||
* print out PaX settings on boot time, and validate some of them | |||||
*/ | |||||
Not Done Inline ActionsBlock comment should be a sentence; capitalise and use a '.'. rwatson: Block comment should be a sentence; capitalise and use a '.'. | |||||
static void | |||||
pax_sysinit(void) | |||||
Not Done Inline ActionsThis is going to flood the console -- is this not something we can detect earlier when modes are set? rwatson: This is going to flood the console -- is this not something we can detect earlier when modes… | |||||
{ | |||||
printf("PAX: initialize and check PaX and HardeneBSD features.\n"); | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
} | |||||
SYSINIT(pax, SI_SUB_PAX, SI_ORDER_FIRST, pax_sysinit, NULL); | |||||
void | |||||
Not Done Inline ActionsSometimes you check explicitly for prison0; other times, for no parent. I think I prefer a prison0 check, possibly with an assertion that the parent is non-zero. rwatson: Sometimes you check explicitly for prison0; other times, for no parent. I think I prefer a… | |||||
Not Done Inline ActionsWill address in next patch. lattera-gmail.com: Will address in next patch. | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
pax_init_prison(struct prison *pr) | |||||
{ | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
if (pr == NULL) | |||||
Not Done Inline ActionsUse an SDT probe. rwatson: Use an SDT probe. | |||||
Not Done Inline ActionsWith KTR(9), if a kernel panic happens, we can inspect ASLR post-mortem. As DTrace is time-of-use, that's not possible with SDT probes. Would losing the ability to do post-mortem analysis/inspection be preferred? lattera-gmail.com: With KTR(9), if a kernel panic happens, we can inspect ASLR post-mortem. As DTrace is time-of… | |||||
return; | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
if (pr->pr_pax_set) | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
return; | |||||
Not Done Inline ActionsWhen can this be the case? Can this be avoided? mjg: When can this be the case? Can this be avoided? | |||||
prison_lock(pr); | |||||
Not Done Inline ActionsWhat about this? Because there are several calls to this function (which I consider unnecessary), this indeed can be tested true due to a race. But would it be possible otherwise? mjg: What about this?
Because there are several calls to this function (which I consider… | |||||
CTR2(KTR_PAX, "%s: Setting prison %s PaX variables", | |||||
__func__, pr->pr_name); | |||||
Not Done Inline ActionsSince you don't re-check the condition after the lock is taken you can intialize the same jail multiple times. Seems mostly harmless with current code, but definitely a bug which could cause trouble in the future. I once more note that the only calls which seems to be needed is the one during jail creation. This also means that the pr_mtx is not needed since nobody can uset the jail at that time. mjg: Since you don't re-check the condition after the lock is taken you can intialize the same jail… | |||||
Not Done Inline ActionsBlock comments should be a sentence; capitalise and use a '.'. rwatson: Block comments should be a sentence; capitalise and use a '.'. | |||||
#ifdef PAX_ASLR | |||||
pr->pr_pax_aslr_status = pax_aslr_status; | |||||
Not Done Inline Actionsstyle(9) would prefer this variable to be declared above. rwatson: style(9) would prefer this variable to be declared above. | |||||
Not Done Inline ActionsWill address in next patch. lattera-gmail.com: Will address in next patch. | |||||
Not Done Inline ActionsCan you please point out, where can I find this style(9) man page? op@hardenedbsd ~> man 9 style | grep prison op: Can you please point out, where can I find this style(9) man page?
op@hardenedbsd ~> man 9… | |||||
Not Done Inline Actions... Do not put declarations inside blocks unless the routine is unusually complicated. rwatson: ... Do not put declarations inside
blocks unless the routine is unusually… | |||||
Not Done Inline ActionsOkay, than we have different meaning of the word complicated. op: Okay, than we have different meaning of the word complicated. | |||||
Not Done Inline ActionsI think we likely mostly agree on 'complicated' -- the keyword here is 'unusually', which is presumably relative to the remainder of the kernel :-). rwatson: I think we likely mostly agree on 'complicated' -- the keyword here is 'unusually', which is… | |||||
pr->pr_pax_aslr_debug = pax_aslr_debug; | |||||
pr->pr_pax_aslr_mmap_len = pax_aslr_mmap_len; | |||||
pr->pr_pax_aslr_stack_len = pax_aslr_stack_len; | |||||
Not Done Inline ActionsThis sysinit is unnecessary, as it doesn't actually do what it says. Remove. rwatson: This sysinit is unnecessary, as it doesn't actually do what it says. Remove. | |||||
pr->pr_pax_aslr_exec_len = pax_aslr_exec_len; | |||||
#ifdef COMPAT_FREEBSD32 | |||||
pr->pr_pax_aslr_compat_status = pax_aslr_compat_status; | |||||
pr->pr_pax_aslr_compat_mmap_len = pax_aslr_compat_mmap_len; | |||||
pr->pr_pax_aslr_compat_stack_len = pax_aslr_compat_stack_len; | |||||
pr->pr_pax_aslr_compat_exec_len = pax_aslr_compat_exec_len; | |||||
#endif /* COMPAT_FREEBSD32 */ | |||||
#endif /* PAX_ASLR */ | |||||
Not Done Inline ActionsReplace with an SDT probe. rwatson: Replace with an SDT probe. | |||||
pr->pr_pax_set = 1; | |||||
prison_unlock(pr); | |||||
} | |||||
Not Done Inline ActionsYou call this during jail creation, before it is visible anywhere. There is also nobody to call this function with a NULL pointer o twice for the same jail. As such, NULL checks, pr_pax_set variable and locking can be eliminated. Another thing is that the kernel supports multi-level jails. Possibly obtaining global configuration instead of inheriting it is a bad choice. mjg: You call this during jail creation, before it is visible anywhere. There is also nobody to call… |
Can you find out if it's possible to switch the disclaimer block to the new standard text (primarily AUTHOR AND CONTRIBUTORS where AUTHOR is here).