Changeset View
Standalone View
sys/kern/kern_pax_aslr.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 | |||||
* OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. | |||||
* 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$"); | |||||
rwatson: Remove extra comment 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/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 <vm/pmap.h> | |||||
#include <vm/vm_map.h> | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
#include <vm/vm_extern.h> | |||||
#include <machine/elf.h> | |||||
Not Done Inline ActionsCould these headers be more sorted? rwatson: Could these headers be more sorted? | |||||
#include <sys/pax.h> | |||||
FEATURE(aslr, "Address Space Layout Randomization."); | |||||
Not Done Inline ActionsThis header should not be included here. See above. rwatson: This header should not be included here. See above. | |||||
int pax_aslr_status = PAX_ASLR_OPTOUT; | |||||
int pax_aslr_debug = 0; | |||||
#ifdef PAX_ASLR_MAX_SEC | |||||
int pax_aslr_mmap_len = PAX_ASLR_DELTA_MMAP_MAX_LEN; | |||||
int pax_aslr_stack_len = PAX_ASLR_DELTA_STACK_MAX_LEN; | |||||
int pax_aslr_exec_len = PAX_ASLR_DELTA_EXEC_MAX_LEN; | |||||
#else | |||||
int pax_aslr_mmap_len = PAX_ASLR_DELTA_MMAP_DEF_LEN; | |||||
Not Done Inline ActionsBegin formatted comments with "/*-" rather than "/*" so that automated formatting tools know not to reformat the comment. rwatson: Begin formatted comments with "/*-" rather than "/*" so that automated formatting tools know… | |||||
int pax_aslr_stack_len = PAX_ASLR_DELTA_STACK_DEF_LEN; | |||||
int pax_aslr_exec_len = PAX_ASLR_DELTA_EXEC_DEF_LEN; | |||||
#endif /* PAX_ASLR_MAX_SEC */ | |||||
#ifdef COMPAT_FREEBSD32 | |||||
int pax_aslr_compat_status = PAX_ASLR_OPTOUT; | |||||
#ifdef PAX_ASLR_MAX_SEC | |||||
int pax_aslr_compat_mmap_len = PAX_ASLR_COMPAT_DELTA_MMAP_MAX_LEN; | |||||
int pax_aslr_compat_stack_len = PAX_ASLR_COMPAT_DELTA_STACK_MAX_LEN; | |||||
int pax_aslr_compat_exec_len = PAX_ASLR_COMPAT_DELTA_EXEC_MAX_LEN; | |||||
#else | |||||
int pax_aslr_compat_mmap_len = PAX_ASLR_COMPAT_DELTA_MMAP_MIN_LEN; | |||||
int pax_aslr_compat_stack_len = PAX_ASLR_COMPAT_DELTA_STACK_MIN_LEN; | |||||
int pax_aslr_compat_exec_len = PAX_ASLR_COMPAT_DELTA_EXEC_MIN_LEN; | |||||
#endif /* PAX_ASLR_MAX_SEC */ | |||||
#endif /* COMPAT_FREEBSD32 */ | |||||
TUNABLE_INT("security.pax.aslr.status", &pax_aslr_status); | |||||
TUNABLE_INT("security.pax.aslr.mmap_len", &pax_aslr_mmap_len); | |||||
TUNABLE_INT("security.pax.aslr.debug", &pax_aslr_debug); | |||||
TUNABLE_INT("security.pax.aslr.stack_len", &pax_aslr_stack_len); | |||||
TUNABLE_INT("security.pax.aslr.exec_len", &pax_aslr_exec_len); | |||||
#ifdef COMPAT_FREEBSD32 | |||||
TUNABLE_INT("security.pax.aslr.compat.status", &pax_aslr_compat_status); | |||||
TUNABLE_INT("security.pax.aslr.compat.mmap", &pax_aslr_compat_mmap_len); | |||||
TUNABLE_INT("security.pax.aslr.compat.stack", &pax_aslr_compat_stack_len); | |||||
TUNABLE_INT("security.pax.aslr.compat.stack", &pax_aslr_compat_exec_len); | |||||
#endif | |||||
Not Done Inline ActionsThis should be "security.pax.aslr.compat.exec", I suppose. brueffer: This should be "security.pax.aslr.compat.exec", I suppose. | |||||
Not Done Inline ActionsGood catch. lattera-gmail.com: Good catch. | |||||
#ifdef PAX_SYSCTLS | |||||
/* | |||||
* sysctls and tunables | |||||
*/ | |||||
static int sysctl_pax_aslr_debug(SYSCTL_HANDLER_ARGS); | |||||
static int sysctl_pax_aslr_status(SYSCTL_HANDLER_ARGS); | |||||
static int sysctl_pax_aslr_mmap(SYSCTL_HANDLER_ARGS); | |||||
static int sysctl_pax_aslr_stack(SYSCTL_HANDLER_ARGS); | |||||
static int sysctl_pax_aslr_exec(SYSCTL_HANDLER_ARGS); | |||||
SYSCTL_DECL(_security_pax); | |||||
SYSCTL_NODE(_security_pax, OID_AUTO, aslr, CTLFLAG_RD, 0, | |||||
"Address Space Layout Randomization."); | |||||
SYSCTL_PROC(_security_pax_aslr, OID_AUTO, status, | |||||
CTLTYPE_INT|CTLFLAG_RWTUN|CTLFLAG_PRISON|CTLFLAG_SECURE, | |||||
NULL, 0, sysctl_pax_aslr_status, "I", | |||||
"Restrictions status. " | |||||
"0 - disabled, " | |||||
"1 - opt-in, " | |||||
"2 - opt-out, " | |||||
"3 - force enabled"); | |||||
SYSCTL_PROC(_security_pax_aslr, OID_AUTO, debug, | |||||
CTLTYPE_INT|CTLFLAG_RWTUN|CTLFLAG_PRISON|CTLFLAG_SECURE, | |||||
NULL, 0, sysctl_pax_aslr_debug, "I", | |||||
"ASLR debug mode"); | |||||
SYSCTL_PROC(_security_pax_aslr, OID_AUTO, mmap_len, | |||||
CTLTYPE_INT|CTLFLAG_RWTUN|CTLFLAG_PRISON|CTLFLAG_SECURE, | |||||
NULL, 0, sysctl_pax_aslr_mmap, "I", | |||||
"Number of bits randomized for mmap(2) calls. " | |||||
"32 bit: [8,16] 64 bit: [16,32]"); | |||||
SYSCTL_PROC(_security_pax_aslr, OID_AUTO, stack_len, | |||||
CTLTYPE_INT|CTLFLAG_RWTUN|CTLFLAG_PRISON|CTLFLAG_SECURE, | |||||
NULL, 0, sysctl_pax_aslr_stack, "I", | |||||
"Number of bits randomized for the stack. " | |||||
"32 bit: [6,12] 64 bit: [12,21]"); | |||||
SYSCTL_PROC(_security_pax_aslr, OID_AUTO, exec_len, | |||||
CTLTYPE_INT|CTLFLAG_RWTUN|CTLFLAG_PRISON|CTLFLAG_SECURE, | |||||
NULL, 0, sysctl_pax_aslr_exec, "I", | |||||
"Number of bits randomized for the PIE exec base. " | |||||
"32 bit: [6,12] 64 bit: [12,21]"); | |||||
static int | |||||
sysctl_pax_aslr_status(SYSCTL_HANDLER_ARGS) | |||||
{ | |||||
Not Done Inline Actionspax_get_prison() can suffer a race here -- you usually want the thread's prison, not the process's prison. rwatson: pax_get_prison() can suffer a race here -- you usually want the thread's prison, not the… | |||||
Not Done Inline ActionsThis will be addressed. Out of curiosity, under what conditions will the thread's prison not be the same as the process's prison? lattera-gmail.com: This will be addressed. Out of curiosity, under what conditions will the thread's prison not be… | |||||
Not Done Inline ActionsThere are two problems associated with multithreading here: (1) Dereferencing td->td_proc->p_ucred can suffer race conditions (e.g., dereferencing pointers to freed memory) if the process lock isn't held. (2) For multithreaded processes, most system calls / traps will acquire a reference to (and cache a pointer to) p->p_ucred as td->td_ucred for the duration of the call so that the credential is consistent over the course of the call. In those cases, td->td_proc->p_ucred may diverge from td->td_ucred for quite extended periods of time. There is also a brief divergence in the thread performing the credential update itself, which will be resolved quickly by an explicit call to cred_update_thread(), or more slowly for calls that don't do this (e.g., not until the next system call/trap starts) -- this is not done for jail calls (although possibly should be). rwatson: There are two problems associated with multithreading here:
(1) Dereferencing td->td_proc… | |||||
struct prison *pr; | |||||
Not Done Inline ActionsAll these sysctls should have CTFLAF_MPSAFE set. mjg: All these sysctls should have CTFLAF_MPSAFE set. | |||||
int err, val; | |||||
pr = pax_get_prison(req->td, NULL); | |||||
val = (pr != NULL) ? pr->pr_pax_aslr_status : pax_aslr_status; | |||||
err = sysctl_handle_int(oidp, &val, sizeof(int), req); | |||||
if (err || (req->newptr == NULL)) | |||||
return (err); | |||||
Not Done Inline ActionsThe numbers _must_ be aligned with the architectural size of pages and superpages. Putting voluntaristic MI values there breaks superpages. kib: The numbers _must_ be aligned with the architectural size of pages and superpages. Putting… | |||||
Not Done Inline ActionsIn FreeBSD's pmap and VM code, we find comments like the one found in link 1 (links below). There's also an interesting function called pmap_align_superpage() referenced at link 2. Given those two things, I'm curious to know how ASLR breaks superpage support. What ideas do you have to make ASLR and superpages to play well together without sacrificing the security of ASLR? Link 1: Link 2: lattera-gmail.com: In FreeBSD's pmap and VM code, we find comments like the one found in link 1 (links below). | |||||
Not Done Inline Actionscan pr really be null here? possibly for kernel threads, but it is unlikely any of them would call here. I suggest to check it and add prison0 to kernel threads if its not already set. mjg: can pr really be null here? possibly for kernel threads, but it is unlikely any of them would… | |||||
switch (val) { | |||||
case PAX_ASLR_DISABLED: | |||||
case PAX_ASLR_OPTIN: | |||||
case PAX_ASLR_OPTOUT: | |||||
case PAX_ASLR_FORCE_ENABLED: | |||||
if ((pr == NULL) || (pr == &prison0)) | |||||
pax_aslr_status = val; | |||||
if (pr != NULL) { | |||||
mtx_lock(&(pr->pr_mtx)); | |||||
pr->pr_pax_aslr_status = val; | |||||
mtx_unlock(&(pr->pr_mtx)); | |||||
} | |||||
break; | |||||
default: | |||||
return (EINVAL); | |||||
} | |||||
return (0); | |||||
} | |||||
static int | |||||
sysctl_pax_aslr_debug(SYSCTL_HANDLER_ARGS) | |||||
{ | |||||
struct prison *pr=NULL; | |||||
int err, val; | |||||
pr = pax_get_prison(req->td, NULL); | |||||
if ((pr != NULL) && !(pr->pr_pax_set)) | |||||
pax_init_prison(pr); | |||||
mjgUnsubmitted Not Done Inline ActionsWhat's up with these calls? Should not all prisons already have pr_pax_set? mjg: What's up with these calls? Should not all prisons already have pr_pax_set? | |||||
val = (pr != NULL) ? pr->pr_pax_aslr_debug : pax_aslr_debug; | |||||
Not Done Inline ActionsFar too much vertical whitespace here and elsewhere in this file. rwatson: Far too much vertical whitespace here and elsewhere in this file. | |||||
err = sysctl_handle_int(oidp, &val, sizeof(int), req); | |||||
if (err || !req->newptr) | |||||
return (err); | |||||
switch (val) { | |||||
case 0: | |||||
case 1: | |||||
break; | |||||
default: | |||||
return (EINVAL); | |||||
} | |||||
if ((pr == NULL) || (pr == &prison0)) | |||||
pax_aslr_debug = val; | |||||
if (pr != NULL) { | |||||
mtx_lock(&(pr->pr_mtx)); | |||||
pr->pr_pax_aslr_debug = val; | |||||
mtx_unlock(&(pr->pr_mtx)); | |||||
} | |||||
return (0); | |||||
} | |||||
static int | |||||
sysctl_pax_aslr_mmap(SYSCTL_HANDLER_ARGS) | |||||
{ | |||||
struct prison *pr=NULL; | |||||
int err, val; | |||||
pr = pax_get_prison(req->td, NULL); | |||||
if ((pr != NULL) && !(pr->pr_pax_set)) | |||||
pax_init_prison(pr); | |||||
val = (pr != NULL) ? pr->pr_pax_aslr_mmap_len : pax_aslr_mmap_len; | |||||
err = sysctl_handle_int(oidp, &val, sizeof(int), req); | |||||
if (err || !req->newptr) | |||||
return (err); | |||||
if (val < PAX_ASLR_DELTA_MMAP_MIN_LEN || | |||||
val > PAX_ASLR_DELTA_MMAP_MAX_LEN) | |||||
return (EINVAL); | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
if ((pr == NULL) || (pr == &prison0)) | |||||
pax_aslr_mmap_len = val; | |||||
if (pr != NULL) { | |||||
mtx_lock(&(pr->pr_mtx)); | |||||
pr->pr_pax_aslr_mmap_len = val; | |||||
mtx_unlock(&(pr->pr_mtx)); | |||||
} | |||||
return (0); | |||||
} | |||||
static int | |||||
sysctl_pax_aslr_stack(SYSCTL_HANDLER_ARGS) | |||||
{ | |||||
Not Done Inline ActionsWhat do you think about hiding this "expert" settings with adding CTLFLAG_SKIP? op: What do you think about hiding this "expert" settings with adding CTLFLAG_SKIP? | |||||
struct prison *pr=NULL; | |||||
Not Done Inline ActionsNot a fan -- with many tens of thousands of sysctls, CTLFLAG_SKIP is not really a cosmetic thing about hiding advanced settings, it's about skipping nodes that have a functional impact if viewed in 'sysctl -a'. rwatson: Not a fan -- with many tens of thousands of sysctls, CTLFLAG_SKIP is not really a cosmetic… | |||||
Not Done Inline ActionsAgreed. op: Agreed. | |||||
int err, val; | |||||
pr = pax_get_prison(req->td, NULL); | |||||
if ((pr != NULL) && !(pr->pr_pax_set)) | |||||
Not Done Inline ActionsAnd these. And all of the similar bit settings. op: And these. And all of the similar bit settings. | |||||
pax_init_prison(pr); | |||||
val = (pr != NULL) ? pr->pr_pax_aslr_stack_len : pax_aslr_stack_len; | |||||
err = sysctl_handle_int(oidp, &val, sizeof(int), req); | |||||
if (err || !req->newptr) | |||||
return (err); | |||||
if (val < PAX_ASLR_DELTA_STACK_MIN_LEN || | |||||
val > PAX_ASLR_DELTA_STACK_MAX_LEN) | |||||
return (EINVAL); | |||||
if ((pr == NULL) || (pr == &prison0)) | |||||
pax_aslr_stack_len = val; | |||||
if (pr != NULL) { | |||||
mtx_lock(&(pr->pr_mtx)); | |||||
pr->pr_pax_aslr_stack_len = val; | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
mtx_unlock(&(pr->pr_mtx)); | |||||
} | |||||
return (0); | |||||
} | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
static int | |||||
sysctl_pax_aslr_exec(SYSCTL_HANDLER_ARGS) | |||||
{ | |||||
struct prison *pr=NULL; | |||||
int err, val; | |||||
Not Done Inline ActionsHere, and in a number of places, you replicate the prison0 setting to the global variable. Is there a reason not to just use the prison0 setting elsewhere, and avoid duplication? prison0 is always defined... rwatson: Here, and in a number of places, you replicate the prison0 setting to the global variable. Is… | |||||
pr = pax_get_prison(req->td, NULL); | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
if ((pr != NULL) && !(pr->pr_pax_set)) | |||||
pax_init_prison(pr); | |||||
val = (pr != NULL) ? pr->pr_pax_aslr_exec_len : pax_aslr_exec_len; | |||||
err = sysctl_handle_int(oidp, &val, sizeof(int), req); | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
if (err || (req->newptr == NULL)) | |||||
return (err); | |||||
if (val < PAX_ASLR_DELTA_EXEC_MIN_LEN || | |||||
val > PAX_ASLR_DELTA_EXEC_MAX_LEN) | |||||
return (EINVAL); | |||||
if ((pr == NULL) || (pr == &prison0)) | |||||
pax_aslr_exec_len = val; | |||||
if (pr != NULL) { | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
mtx_lock(&(pr->pr_mtx)); | |||||
pr->pr_pax_aslr_exec_len = val; | |||||
Not Done Inline ActionsExcessive dots :-) brueffer: Excessive dots :-) | |||||
Not Done Inline ActionsGood catch. I'll fix that and also make it a single-line comment. lattera-gmail.com: Good catch. I'll fix that and also make it a single-line comment. | |||||
mtx_unlock(&(pr->pr_mtx)); | |||||
} | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
return (0); | |||||
} | |||||
/* | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
* COMPAT_FREEBSD32 and linuxulator.. | |||||
*/ | |||||
#ifdef COMPAT_FREEBSD32 | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
static int sysctl_pax_aslr_compat_status(SYSCTL_HANDLER_ARGS); | |||||
static int sysctl_pax_aslr_compat_mmap(SYSCTL_HANDLER_ARGS); | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
static int sysctl_pax_aslr_compat_stack(SYSCTL_HANDLER_ARGS); | |||||
static int sysctl_pax_aslr_compat_exec(SYSCTL_HANDLER_ARGS); | |||||
Not Done Inline ActionsRace condition per above. rwatson: Race condition per above. | |||||
SYSCTL_NODE(_security_pax_aslr, OID_AUTO, compat, CTLFLAG_RD, 0, | |||||
"Setting for COMPAT_FREEBSD32 and linuxulator."); | |||||
SYSCTL_PROC(_security_pax_aslr_compat, OID_AUTO, status, | |||||
CTLTYPE_INT|CTLFLAG_RWTUN|CTLFLAG_PRISON, | |||||
NULL, 0, sysctl_pax_aslr_compat_status, "I", | |||||
"Restrictions status. " | |||||
"0 - disabled, " | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
"1 - enabled, " | |||||
"2 - global enabled, " | |||||
"3 - force global enabled"); | |||||
SYSCTL_PROC(_security_pax_aslr_compat, OID_AUTO, mmap_len, | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
CTLTYPE_INT|CTLFLAG_RWTUN|CTLFLAG_PRISON, | |||||
NULL, 0, sysctl_pax_aslr_compat_mmap, "I", | |||||
"Number of bits randomized for mmap(2) calls. " | |||||
"32 bit: [8,16]"); | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
SYSCTL_PROC(_security_pax_aslr_compat, OID_AUTO, stack_len, | |||||
CTLTYPE_INT|CTLFLAG_RWTUN|CTLFLAG_PRISON, | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
NULL, 0, sysctl_pax_aslr_compat_stack, "I", | |||||
"Number of bits randomized for the stack. " | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
"32 bit: [6,12]"); | |||||
Not Done Inline ActionsRace condition. rwatson: Race condition. | |||||
SYSCTL_PROC(_security_pax_aslr_compat, OID_AUTO, exec_len, | |||||
CTLTYPE_INT|CTLFLAG_RWTUN|CTLFLAG_PRISON, | |||||
NULL, 0, sysctl_pax_aslr_compat_exec, "I", | |||||
"Number of bits randomized for the PIE exec base. " | |||||
"32 bit: [6,12]"); | |||||
static int | |||||
sysctl_pax_aslr_compat_status(SYSCTL_HANDLER_ARGS) | |||||
{ | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
struct prison *pr; | |||||
int err, val; | |||||
pr = pax_get_prison(req->td, NULL); | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
val = (pr != NULL) ?pr->pr_pax_aslr_compat_status : pax_aslr_compat_status; | |||||
err = sysctl_handle_int(oidp, &val, sizeof(int), req); | |||||
if (err || (req->newptr == NULL)) | |||||
return (err); | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
switch (val) { | |||||
case PAX_ASLR_DISABLED: | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
case PAX_ASLR_OPTIN: | |||||
case PAX_ASLR_OPTOUT: | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
case PAX_ASLR_FORCE_ENABLED: | |||||
if ((pr == NULL) || (pr == &prison0)) | |||||
Not Done Inline ActionsRace condition. rwatson: Race condition. | |||||
pax_aslr_compat_status = val; | |||||
if (pr != NULL) { | |||||
mtx_lock(&(pr->pr_mtx)); | |||||
pr->pr_pax_aslr_compat_status = val; | |||||
mtx_unlock(&(pr->pr_mtx)); | |||||
} | |||||
break; | |||||
default: | |||||
return (EINVAL); | |||||
} | |||||
return (0); | |||||
} | |||||
static int | |||||
sysctl_pax_aslr_compat_mmap(SYSCTL_HANDLER_ARGS) | |||||
{ | |||||
struct prison *pr; | |||||
int err, val; | |||||
pr = pax_get_prison(req->td, NULL); | |||||
val = (pr != NULL) ? pr->pr_pax_aslr_compat_mmap_len : pax_aslr_compat_mmap_len; | |||||
err = sysctl_handle_int(oidp, &val, sizeof(int), req); | |||||
if (err || !req->newptr) | |||||
return (err); | |||||
if (val < PAX_ASLR_COMPAT_DELTA_MMAP_MIN_LEN || | |||||
Not Done Inline ActionsStyle(9) requires a blank line here. Please check all other functions without local variables to ensure that they also have blank lines. rwatson: Style(9) requires a blank line here. Please check all other functions without local variables… | |||||
Not Done Inline ActionsWill be addressed in the next patch. lattera-gmail.com: Will be addressed in the next patch. | |||||
val > PAX_ASLR_COMPAT_DELTA_MMAP_MAX_LEN) | |||||
return (EINVAL); | |||||
if ((pr == NULL) || (pr == &prison0)) | |||||
pax_aslr_compat_mmap_len = val; | |||||
if (pr != NULL) { | |||||
mtx_lock(&(pr->pr_mtx)); | |||||
pr->pr_pax_aslr_compat_mmap_len = val; | |||||
mtx_unlock(&(pr->pr_mtx)); | |||||
} | |||||
return (0); | |||||
} | |||||
static int | |||||
sysctl_pax_aslr_compat_stack(SYSCTL_HANDLER_ARGS) | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
{ | |||||
struct prison *pr; | |||||
int err, val; | |||||
Not Done Inline ActionsIn general, the kernel does not use the 'bool' type; please use 'int'. rwatson: In general, the kernel does not use the 'bool' type; please use 'int'. | |||||
Not Done Inline ActionsWill be addressed in the next patch. lattera-gmail.com: Will be addressed in the next patch. | |||||
Not Done Inline Actionsop@hardenedbsd kern> git grep -A 3 "^bool" kern_pax_aslr.c- u_int flags;subr_bus.c:boolean_t subr_bus.c- return (devsoftc.inuse == 1);subr_capability.c:bool subr_capability.c- va_list ap;subr_capability.c:bool subr_capability.c- cap_rights_t allrights;subr_capability.c:bool subr_capability.c- unsigned int i, n;subr_sfbuf.c:boolean_t op: op@hardenedbsd kern> git grep -A 3 "^bool"
kern_pax_aslr.c:bool
kern_pax_aslr.c-pax_aslr_active… | |||||
Not Done Inline ActionsSounds like 'In general' to me. rwatson: Sounds like 'In general' to me. | |||||
pr = pax_get_prison(req->td, NULL); | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
val = (pr != NULL) ? pr->pr_pax_aslr_compat_stack_len : pax_aslr_compat_stack_len; | |||||
err = sysctl_handle_int(oidp, &val, sizeof(int), req); | |||||
if (err || !req->newptr) | |||||
return (err); | |||||
if (val < PAX_ASLR_COMPAT_DELTA_STACK_MIN_LEN || | |||||
val > PAX_ASLR_COMPAT_DELTA_STACK_MAX_LEN) | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
return (EINVAL); | |||||
Not Done Inline ActionsAdd blank line (for variety). rwatson: Add blank line (for variety). | |||||
if ((pr == NULL) || (pr == &prison0)) | |||||
pax_aslr_compat_stack_len = val; | |||||
if (pr != NULL) { | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
mtx_lock(&(pr->pr_mtx)); | |||||
pr->pr_pax_aslr_compat_stack_len = val; | |||||
mtx_unlock(&(pr->pr_mtx)); | |||||
} | |||||
Not Done Inline ActionsThis function name should not begin with '_'. Are there cases where 'vm' is not the p's vmspace requiring them to be passed separately? If so, please add a comment on why that happens. rwatson: This function name should not begin with '_'.
Are there cases where 'vm' is not the p's… | |||||
Not Done Inline ActionsDone in next patch. op: Done in next patch. | |||||
return (0); | |||||
} | |||||
static int | |||||
sysctl_pax_aslr_compat_exec(SYSCTL_HANDLER_ARGS) | |||||
{ | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
struct prison *pr; | |||||
int err, val; | |||||
pr = pax_get_prison(req->td, NULL); | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
if (pr != NULL) | |||||
val = pr->pr_pax_aslr_compat_exec_len; | |||||
else | |||||
val = pax_aslr_compat_exec_len; | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
err = sysctl_handle_int(oidp, &val, sizeof(int), req); | |||||
if (err || !req->newptr) | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
return (err); | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
if (val < PAX_ASLR_COMPAT_DELTA_EXEC_MIN_LEN || | |||||
val > PAX_ASLR_COMPAT_DELTA_EXEC_MAX_LEN) | |||||
return (EINVAL); | |||||
if ((pr == NULL) || (pr == &prison0)) | |||||
pax_aslr_compat_exec_len = val; | |||||
if (pr != NULL) { | |||||
mtx_lock(&(pr->pr_mtx)); | |||||
pr->pr_pax_aslr_compat_exec_len = val; | |||||
mtx_unlock(&(pr->pr_mtx)); | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
} | |||||
return (0); | |||||
} | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
#endif /* COMPAT_FREEBSD32 */ | |||||
#endif /* PAX_SYSCTLS */ | |||||
Not Done Inline ActionsIt would be nice to drop all the funky formatting here. The kernel generally uses output along the lines of: subsystem: warning text Without brackets, upper-case letters, exclamation marks, etc. rwatson: It would be nice to drop all the funky formatting here. The kernel generally uses output along… | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
/* | |||||
* ASLR functions | |||||
*/ | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
bool | |||||
pax_aslr_active(struct thread *td, struct proc *proc) | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
{ | |||||
Not Done Inline ActionsPlease remove these printfs, or at least boot them under boot_verbose. We don't need all this information on every boot. rwatson: Please remove these printfs, or at least boot them under boot_verbose. We don't need all this… | |||||
Not Done Inline ActionsWill be address in the next patch. lattera-gmail.com: Will be address in the next patch. | |||||
Not Done Inline ActionsI much more prefer to hide them under bootverbose rather then removing them... op: I much more prefer to hide them under bootverbose rather then removing them... | |||||
Not Done Inline ActionsThen do that. Please do make them much less ugly, however. In general, FreeBSD console messages are not placed in brackets. rwatson: Then do that. Please do make them much less ugly, however. In general, FreeBSD console messages… | |||||
Not Done Inline ActionsOkay! op: Okay! | |||||
int status; | |||||
struct prison *pr; | |||||
uint32_t flags; | |||||
if ((td == NULL) && (proc == NULL)) | |||||
Not Done Inline ActionsPlease rename this function to remove the '_'. Are there cases where 'vm' is not the p's vmspace requiring them to be passed separately? If so, please add a comment on why that happens. rwatson: Please rename this function to remove the '_'.
Are there cases where 'vm' is not the p's… | |||||
Not Done Inline Actions(I should probably have extended this comment to indicate that if this is believed never to happen, you should pass only one, and possibly have a KASSERT() documenting the assumption.) rwatson: (I should probably have extended this comment to indicate that if this is believed never to… | |||||
return (true); | |||||
pr = pax_get_prison(td, proc); | |||||
flags = (td != NULL) ? td->td_proc->p_pax : proc->p_pax; | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
if (((flags & 0xaaaaaaaa) & ((flags & 0x55555555) << 1)) != 0) { | |||||
pax_log_aslr(pr, __func__, "inconsistent paxflags: %x\n", flags); | |||||
pax_ulog_aslr(pr, NULL, "inconsistent paxflags: %x\n", flags); | |||||
return (true); | |||||
} | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
if (pr != NULL) | |||||
status = pr->pr_pax_aslr_status; | |||||
else | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
status = pax_aslr_status; | |||||
switch (status) { | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
case PAX_ASLR_DISABLED: | |||||
return (false); | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
case PAX_ASLR_FORCE_ENABLED: | |||||
return (true); | |||||
case PAX_ASLR_OPTIN: | |||||
if ((flags & PAX_NOTE_ASLR) == 0) { | |||||
pax_log_aslr(pr, __func__, | |||||
"ASLR is opt-in, and executable does not have ASLR enabled\n"); | |||||
pax_ulog_aslr(pr, NULL, | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
"ASLR is opt-in, and executable does not have ASLR enabled\n"); | |||||
return (false); | |||||
} | |||||
break; | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
case PAX_ASLR_OPTOUT: | |||||
if ((flags & PAX_NOTE_NOASLR) != 0) { | |||||
pax_log_aslr(pr, __func__, | |||||
"ASLR is opt-out, and executable explicitly disabled ASLR\n"); | |||||
pax_ulog_aslr(pr, NULL, | |||||
Not Done Inline ActionsIs this KASSERT() needed? rwatson: Is this KASSERT() needed? | |||||
"ASLR is opt-out, and executable explicitly disabled ASLR\n"); | |||||
return (false); | |||||
} | |||||
break; | |||||
default: | |||||
Not Done Inline ActionsAdd blank line. rwatson: Add blank line. | |||||
return (true); | |||||
Not Done Inline ActionsIs this local variable needed? rwatson: Is this local variable needed? | |||||
Not Done Inline ActionsNo. We'll remove it in the next patch. lattera-gmail.com: No. We'll remove it in the next patch. | |||||
} | |||||
return (true); | |||||
Not Done Inline ActionsHere's an example of a place where it seems like just using prison0 might be possible, to avoid replication all over the place. rwatson: Here's an example of a place where it seems like just using prison0 might be possible, to avoid… | |||||
} | |||||
void | |||||
_pax_aslr_init(struct vmspace *vm, struct prison *pr) | |||||
{ | |||||
if (vm == NULL) | |||||
panic("[PaX ASLR] %s: vm == NULL", __func__); | |||||
vm->vm_aslr_delta_mmap = PAX_ASLR_DELTA(arc4random(), | |||||
PAX_ASLR_DELTA_MMAP_LSB, (pr != NULL) ? | |||||
pr->pr_pax_aslr_mmap_len : | |||||
Not Done Inline Actionslen_32bit is used only if MAP_32BIT is defined -- it would make sense to move the len_32bit calculation below to where it is used, and to use the same ifdef. rwatson: len_32bit is used only if MAP_32BIT is defined -- it would make sense to move the len_32bit… | |||||
Not Done Inline ActionsWill be addressed in the next patch. lattera-gmail.com: Will be addressed in the next patch. | |||||
pax_aslr_mmap_len); | |||||
vm->vm_aslr_delta_stack = PAX_ASLR_DELTA(arc4random(), | |||||
PAX_ASLR_DELTA_STACK_LSB, (pr != NULL) ? | |||||
pr->pr_pax_aslr_stack_len : | |||||
pax_aslr_stack_len); | |||||
vm->vm_aslr_delta_stack = ALIGN(vm->vm_aslr_delta_stack); | |||||
vm->vm_aslr_delta_exec = PAX_ASLR_DELTA(arc4random(), | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
PAX_ASLR_DELTA_EXEC_LSB, (pr != NULL) ? | |||||
Not Done Inline ActionsSDT probe. rwatson: SDT probe. | |||||
pr->pr_pax_aslr_exec_len : | |||||
pax_aslr_exec_len); | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
if ((pr != NULL) && pr->pr_pax_aslr_debug) { | |||||
pax_log_aslr(pr, __func__, "vm_aslr_delta_mmap=%p\n", | |||||
Not Done Inline ActionsDon't use 'true' and 'false' in the kernel. rwatson: Don't use 'true' and 'false' in the kernel.
Remove blank line. | |||||
(void *) vm->vm_aslr_delta_mmap); | |||||
pax_log_aslr(pr, __func__, "vm_aslr_delta_stack=%p\n", | |||||
(void *) vm->vm_aslr_delta_stack); | |||||
Not Done Inline ActionsRemove blank line; true/false. rwatson: Remove blank line; true/false. | |||||
pax_log_aslr(pr, __func__, "vm_aslr_delta_exec=%p\n", | |||||
Not Done Inline ActionsDon't use true/false in the kernel. rwatson: Don't use true/false in the kernel. | |||||
(void *) vm->vm_aslr_delta_exec); | |||||
Not Done Inline ActionsI think there is a new convergence in the kernel: op: I think there is a new convergence in the kernel:
https://github. | |||||
pax_ulog_aslr(pr, NULL, "vm_aslr_delta_mmap=%p\n", | |||||
(void *) vm->vm_aslr_delta_mmap); | |||||
pax_ulog_aslr(pr, NULL, "vm_aslr_delta_stack=%p\n", | |||||
(void *) vm->vm_aslr_delta_stack); | |||||
pax_ulog_aslr(pr, NULL, "vm_aslr_delta_exec=%p\n", | |||||
(void *) vm->vm_aslr_delta_exec); | |||||
} | |||||
} | |||||
#ifdef COMPAT_FREEBSD32 | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
void | |||||
_pax_aslr_init32(struct vmspace *vm, struct prison *pr) | |||||
{ | |||||
if (vm == NULL) | |||||
panic("[PaX ASLR] %s: vm == NULL", __func__); | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
vm->vm_aslr_delta_mmap = PAX_ASLR_DELTA(arc4random(), | |||||
PAX_ASLR_COMPAT_DELTA_MMAP_LSB, (pr != NULL) ? | |||||
pr->pr_pax_aslr_compat_mmap_len : | |||||
pax_aslr_compat_mmap_len); | |||||
vm->vm_aslr_delta_stack = PAX_ASLR_DELTA(arc4random(), | |||||
PAX_ASLR_COMPAT_DELTA_STACK_LSB, (pr != NULL) ? | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
pr->pr_pax_aslr_compat_stack_len : | |||||
pax_aslr_compat_stack_len); | |||||
vm->vm_aslr_delta_stack = ALIGN(vm->vm_aslr_delta_stack); | |||||
vm->vm_aslr_delta_exec = PAX_ASLR_DELTA(arc4random(), | |||||
PAX_ASLR_DELTA_EXEC_LSB, (pr != NULL) ? | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
pr->pr_pax_aslr_compat_exec_len : | |||||
pax_aslr_compat_exec_len); | |||||
if ((pr != NULL) && pr->pr_pax_aslr_debug) { | |||||
pax_log_aslr(pr, __func__, "vm_aslr_delta_mmap=%p\n", | |||||
(void *) vm->vm_aslr_delta_mmap); | |||||
Not Done Inline ActionsUse SDT probes. rwatson: Use SDT probes. | |||||
pax_log_aslr(pr, __func__, "vm_aslr_delta_stack=%p\n", | |||||
(void *) vm->vm_aslr_delta_stack); | |||||
pax_log_aslr(pr, __func__, "vm_aslr_delta_exec=%p\n", | |||||
(void *) vm->vm_aslr_delta_exec); | |||||
pax_ulog_aslr(pr, NULL, "vm_aslr_delta_mmap=%p\n", | |||||
(void *) vm->vm_aslr_delta_mmap); | |||||
pax_ulog_aslr(pr, NULL, "vm_aslr_delta_stack=%p\n", | |||||
(void *) vm->vm_aslr_delta_stack); | |||||
pax_ulog_aslr(pr, NULL, "vm_aslr_delta_exec=%p\n", | |||||
(void *) vm->vm_aslr_delta_exec); | |||||
} | |||||
} | |||||
#endif | |||||
Not Done Inline ActionsAdd blank line here. rwatson: Add blank line here. | |||||
void | |||||
pax_aslr_init(struct thread *td, struct image_params *imgp) | |||||
{ | |||||
struct prison *pr; | |||||
struct vmspace *vm; | |||||
pr = pax_get_prison(td, NULL); | |||||
if (imgp == NULL) | |||||
panic("[PaX ASLR] %s: imgp == NULL", __func__); | |||||
if (!pax_aslr_active(td, NULL)) | |||||
return; | |||||
vm = imgp->proc->p_vmspace; | |||||
if (imgp->sysent->sv_pax_aslr_init != NULL) | |||||
imgp->sysent->sv_pax_aslr_init(vm, pr); | |||||
} | |||||
void | |||||
pax_aslr_mmap(struct thread *td, vm_offset_t *addr, vm_offset_t orig_addr, int flags) | |||||
{ | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
struct prison *pr; | |||||
if (!pax_aslr_active(td, NULL)) | |||||
return; | |||||
pr = pax_get_prison(td, NULL); | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
if (!(flags & MAP_FIXED) && ((orig_addr == 0) || !(flags & MAP_ANON))) { | |||||
pax_log_aslr(pr, __func__, "applying to %p orig_addr=%p flags=%x\n", | |||||
(void *)*addr, (void *)orig_addr, flags); | |||||
*addr += td->td_proc->p_vmspace->vm_aslr_delta_mmap; | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
pax_log_aslr(pr, __func__, "result %p\n", (void *)*addr); | |||||
} else { | |||||
pax_log_aslr(pr, __func__, "not applying to %p orig_addr=%p flags=%x\n", | |||||
(void *)*addr, (void *)orig_addr, flags); | |||||
} | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
} | |||||
void | |||||
pax_aslr_stack(struct thread *td, uintptr_t *addr) | |||||
{ | |||||
struct prison *pr; | |||||
Not Done Inline ActionsUse SDT probes. rwatson: Use SDT probes. | |||||
uintptr_t orig_addr; | |||||
if (!pax_aslr_active(td, NULL)) | |||||
return; | |||||
pr = pax_get_prison(td, NULL); | |||||
orig_addr = *addr; | |||||
*addr -= td->td_proc->p_vmspace->vm_aslr_delta_stack; | |||||
pax_log_aslr(pr, __func__, "orig_addr=%p, new_addr=%p\n", | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
(void *)orig_addr, (void *)*addr); | |||||
pax_ulog_aslr(pr, NULL, "orig_addr=%p, new_addr=%p\n", | |||||
(void *)orig_addr, (void *)*addr); | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
} | |||||
Not Done Inline ActionsYou use fixed offset for native binaries, but do per-call randomization for MAP_32BIT. You still completely mishandle mmap() calls from 32bit processes. As with stack and image activator, just adding the delta inline in the sys_mmap() would elminate the unneeded code and provide the same effect. kib: You use fixed offset for native binaries, but do per-call randomization for MAP_32BIT. You… | |||||
Not Done Inline ActionsWas this comment addressed? rwatson: Was this comment addressed? | |||||
Not Done Inline ActionsNot yet, but after the latest sys_mmap cleanup from jhb@ I changed and cleaned up the MAP_32BIT part. op: Not yet, but after the latest sys_mmap cleanup from jhb@ I changed and cleaned up the MAP_32BIT… | |||||
Not Done Inline ActionsUse SDT probe. rwatson: Use SDT probe. | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
Not Done Inline ActionsComment should be a sentence. If you were able to simply use prison0 throughout for globals, you wouldn't need explicit synchronisation code all over the place. Is that not possible? Seems like, otherwise, it's just an opportunity for bugs as people extend the implementation. rwatson: Comment should be a sentence.
If you were able to simply use prison0 throughout for globals… | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
Not Done Inline ActionsUse SDT probe. rwatson: Use SDT probe. | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
Not Done Inline ActionsComment should be a complete sentence. rwatson: Comment should be a complete sentence.
As above, would be preferable to just use prison0. | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
Not Done Inline ActionsUse SDT probe. rwatson: Use SDT probe. | |||||
Not Done Inline Actionsstyle(9) does not like variable declarations in blocks. rwatson: style(9) does not like variable declarations in blocks. | |||||
Not Done Inline ActionsUse SDT probe. rwatson: Use SDT probe. | |||||
Not Done Inline ActionsUse SDT probe. rwatson: Use SDT probe. | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
Not Done Inline ActionsCall variable 'addrp' indicating that it is a pointer to an address. rwatson: Call variable 'addrp' indicating that it is a pointer to an address. | |||||
Not Done Inline ActionsUse SDT probe. rwatson: Use SDT probe. | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
Not Done Inline ActionsName variable 'et_dyn_addrp' to hint that this is a pointer to an address. rwatson: Name variable 'et_dyn_addrp' to hint that this is a pointer to an address. | |||||
Not Done Inline ActionsI wonder if, in these cases, some sort of KASSERT() for wraparound would be a good idea. Not really necessary, I guess. rwatson: I wonder if, in these cases, some sort of KASSERT() for wraparound would be a good idea. Not… | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
Not Done Inline ActionsMake comment a sentence. rwatson: Make comment a sentence.
Is this a violation of an invariant? | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. |
Remove extra comment line.