Page MenuHomeFreeBSD
Differential D473 Diff 825 sys/kern/kern_pax_aslr.c

Changeset View

Standalone View

View Options

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$");
rwatsonUnsubmitted
Not Done
Inline Actions

Remove extra comment line.

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>
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
#include <vm/vm_extern.h>
#include <machine/elf.h>
rwatsonUnsubmitted
Not Done
Inline Actions

Could these headers be more sorted?

rwatson: Could these headers be more sorted?
#include <sys/pax.h>
FEATURE(aslr, "Address Space Layout Randomization.");
rwatsonUnsubmitted
Not Done
Inline Actions

This 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;
rwatsonUnsubmitted
Not Done
Inline Actions

Begin 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
bruefferUnsubmitted
Not Done
Inline Actions

This should be "security.pax.aslr.compat.exec", I suppose.

brueffer: This should be "security.pax.aslr.compat.exec", I suppose.
lattera-gmail.comAuthorUnsubmitted
Not Done
Inline Actions

Good 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)
{
rwatsonUnsubmitted
Not Done
Inline Actions

pax_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…
lattera-gmail.comAuthorUnsubmitted
Not Done
Inline Actions

This 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…
rwatsonUnsubmitted
Not Done
Inline Actions

There 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;
mjgUnsubmitted
Not Done
Inline Actions

All 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);
kibUnsubmitted
Not Done
Inline Actions

The 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…
lattera-gmail.comAuthorUnsubmitted
Not Done
Inline Actions

In 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:
https://github.com/HardenedBSD/hardenedBSD/blob/hardened/current/master/sys/amd64/amd64/pmap.c#L6681

Link 2:
https://github.com/HardenedBSD/hardenedBSD/blob/hardened/current/master/sys/vm/vm_map.c#L1481

lattera-gmail.com: In FreeBSD's pmap and VM code, we find comments like the one found in link 1 (links below).
mjgUnsubmitted
Not Done
Inline Actions

can 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);
val = (pr != NULL) ? pr->pr_pax_aslr_debug : pax_aslr_debug;
err = sysctl_handle_int(oidp, &val, sizeof(int), req);
if (err || !req->newptr)
mjgUnsubmitted
Not Done
Inline Actions

What'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?
return (err);
rwatsonUnsubmitted
Not Done
Inline Actions

Far too much vertical whitespace here and elsewhere in this file.

rwatson: Far too much vertical whitespace here and elsewhere in this file.
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);
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);
if ((pr == NULL) || (pr == &prison0))
pax_aslr_mmap_len = val;
if (pr != NULL) {
mtx_lock(&(pr->pr_mtx));
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
pr->pr_pax_aslr_mmap_len = val;
mtx_unlock(&(pr->pr_mtx));
}
return (0);
}
static int
sysctl_pax_aslr_stack(SYSCTL_HANDLER_ARGS)
{
struct prison *pr=NULL;
int err, val;
pr = pax_get_prison(req->td, NULL);
val = (pr != NULL) ? pr->pr_pax_aslr_stack_len : pax_aslr_stack_len;
opUnsubmitted
Not Done
Inline Actions

What 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?
err = sysctl_handle_int(oidp, &val, sizeof(int), req);
rwatsonUnsubmitted
Not Done
Inline Actions

Not 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…
opUnsubmitted
Not Done
Inline Actions

Agreed.

op: Agreed.
if (err || !req->newptr)
return (err);
if (val < PAX_ASLR_DELTA_STACK_MIN_LEN ||
val > PAX_ASLR_DELTA_STACK_MAX_LEN)
opUnsubmitted
Not Done
Inline Actions

And these. And all of the similar bit settings.

op: And these. And all of the similar bit settings.
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;
mtx_unlock(&(pr->pr_mtx));
}
return (0);
}
static int
sysctl_pax_aslr_exec(SYSCTL_HANDLER_ARGS)
{
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
struct prison *pr=NULL;
int err, val;
pr = pax_get_prison(req->td, NULL);
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
val = (pr != NULL) ? pr->pr_pax_aslr_exec_len : pax_aslr_exec_len;
err = sysctl_handle_int(oidp, &val, sizeof(int), req);
if (err || (req->newptr == NULL))
return (err);
if (val < PAX_ASLR_DELTA_EXEC_MIN_LEN ||
val > PAX_ASLR_DELTA_EXEC_MAX_LEN)
rwatsonUnsubmitted
Not Done
Inline Actions

Here, 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…
return (EINVAL);
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
if ((pr == NULL) || (pr == &prison0))
pax_aslr_exec_len = val;
if (pr != NULL) {
mtx_lock(&(pr->pr_mtx));
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
pr->pr_pax_aslr_exec_len = val;
mtx_unlock(&(pr->pr_mtx));
}
return (0);
}
/*
* COMPAT_FREEBSD32 and linuxulator..
*/
#ifdef COMPAT_FREEBSD32
rwatsonUnsubmitted
Not Done
Inline Actions

Remove 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);
bruefferUnsubmitted
Not Done
Inline Actions

Excessive dots :-)

brueffer: Excessive dots :-)
lattera-gmail.comAuthorUnsubmitted
Not Done
Inline Actions

Good 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.
static int sysctl_pax_aslr_compat_stack(SYSCTL_HANDLER_ARGS);
static int sysctl_pax_aslr_compat_exec(SYSCTL_HANDLER_ARGS);
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
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,
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
CTLTYPE_INT|CTLFLAG_RWTUN|CTLFLAG_PRISON,
NULL, 0, sysctl_pax_aslr_compat_status, "I",
"Restrictions status. "
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
"0 - disabled, "
"1 - enabled, "
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
"2 - global enabled, "
"3 - force global enabled");
rwatsonUnsubmitted
Not Done
Inline Actions

Race condition per above.

rwatson: Race condition per above.
SYSCTL_PROC(_security_pax_aslr_compat, OID_AUTO, mmap_len,
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]");
SYSCTL_PROC(_security_pax_aslr_compat, OID_AUTO, stack_len,
CTLTYPE_INT|CTLFLAG_RWTUN|CTLFLAG_PRISON,
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
NULL, 0, sysctl_pax_aslr_compat_stack, "I",
"Number of bits randomized for the stack. "
"32 bit: [6,12]");
SYSCTL_PROC(_security_pax_aslr_compat, OID_AUTO, exec_len,
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
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]");
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
static int
sysctl_pax_aslr_compat_status(SYSCTL_HANDLER_ARGS)
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
{
struct prison *pr;
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
int err, val;
rwatsonUnsubmitted
Not Done
Inline Actions

Race condition.

rwatson: Race condition.
pr = pax_get_prison(req->td, NULL);
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);
switch (val) {
case PAX_ASLR_DISABLED:
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
case PAX_ASLR_OPTIN:
case PAX_ASLR_OPTOUT:
case PAX_ASLR_FORCE_ENABLED:
if ((pr == NULL) || (pr == &prison0))
pax_aslr_compat_status = val;
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
if (pr != NULL) {
mtx_lock(&(pr->pr_mtx));
pr->pr_pax_aslr_compat_status = val;
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
mtx_unlock(&(pr->pr_mtx));
}
break;
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
default:
return (EINVAL);
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
}
rwatsonUnsubmitted
Not Done
Inline Actions

Race condition.

rwatson: Race condition.
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 ||
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));
}
rwatsonUnsubmitted
Not Done
Inline Actions

Style(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…
lattera-gmail.comAuthorUnsubmitted
Not Done
Inline Actions

Will be addressed in the next patch.

lattera-gmail.com: Will be addressed in the next patch.
return (0);
}
static int
sysctl_pax_aslr_compat_stack(SYSCTL_HANDLER_ARGS)
{
struct prison *pr;
int err, val;
pr = pax_get_prison(req->td, NULL);
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 ||
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
val > PAX_ASLR_COMPAT_DELTA_STACK_MAX_LEN)
return (EINVAL);
rwatsonUnsubmitted
Not Done
Inline Actions

In 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'.
lattera-gmail.comAuthorUnsubmitted
Not Done
Inline Actions

Will be addressed in the next patch.

lattera-gmail.com: Will be addressed in the next patch.
opUnsubmitted
Not Done
Inline Actions

op@hardenedbsd kern> git grep -A 3 "^bool"
kern_pax_aslr.c:bool
kern_pax_aslr.c-pax_aslr_active(struct proc *p)
kern_pax_aslr.c-{

kern_pax_aslr.c- u_int flags;

subr_bus.c:boolean_t
subr_bus.c-devctl_process_running(void)
subr_bus.c-{

subr_bus.c- return (devsoftc.inuse == 1);

subr_capability.c:bool
subr_capability.c-__cap_rights_is_set(const cap_rights_t *rights, ...)
subr_capability.c-{

subr_capability.c- va_list ap;

subr_capability.c:bool
subr_capability.c-cap_rights_is_valid(const cap_rights_t *rights)
subr_capability.c-{

subr_capability.c- cap_rights_t allrights;

subr_capability.c:bool
subr_capability.c-cap_rights_contains(const cap_rights_t *big, const cap_rights_t *little)
subr_capability.c-{

subr_capability.c- unsigned int i, n;

subr_sfbuf.c:boolean_t
subr_sfbuf.c-sf_buf_process_page(vm_page_t m, void (*cb)(struct sf_buf *))
subr_sfbuf.c-{
subr_sfbuf.c- struct sf_head *hash_list;

op: op@hardenedbsd kern> git grep -A 3 "^bool" kern_pax_aslr.c:bool kern_pax_aslr.c-pax_aslr_active…
rwatsonUnsubmitted
Not Done
Inline Actions

Sounds like 'In general' to me.

rwatson: Sounds like 'In general' to me.
if ((pr == NULL) || (pr == &prison0))
pax_aslr_compat_stack_len = val;
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
if (pr != NULL) {
mtx_lock(&(pr->pr_mtx));
pr->pr_pax_aslr_compat_stack_len = val;
mtx_unlock(&(pr->pr_mtx));
}
return (0);
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
}
rwatsonUnsubmitted
Not Done
Inline Actions

Add blank line (for variety).

rwatson: Add blank line (for variety).
static int
sysctl_pax_aslr_compat_exec(SYSCTL_HANDLER_ARGS)
{
struct prison *pr;
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
int err, val;
pr = pax_get_prison(req->td, NULL);
rwatsonUnsubmitted
Not Done
Inline Actions

This 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…
opUnsubmitted
Not Done
Inline Actions

Done in next patch.

op: Done in next patch.
if (pr != NULL)
val = pr->pr_pax_aslr_compat_exec_len;
else
val = pax_aslr_compat_exec_len;
err = sysctl_handle_int(oidp, &val, sizeof(int), req);
if (err || !req->newptr)
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
return (err);
if (val < PAX_ASLR_COMPAT_DELTA_EXEC_MIN_LEN ||
val > PAX_ASLR_COMPAT_DELTA_EXEC_MAX_LEN)
return (EINVAL);
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
if ((pr == NULL) || (pr == &prison0))
pax_aslr_compat_exec_len = val;
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
if (pr != NULL) {
mtx_lock(&(pr->pr_mtx));
pr->pr_pax_aslr_compat_exec_len = val;
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
mtx_unlock(&(pr->pr_mtx));
}
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
return (0);
}
#endif /* COMPAT_FREEBSD32 */
#endif /* PAX_SYSCTLS */
/*
* ASLR functions
*/
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
bool
pax_aslr_active(struct thread *td, struct proc *proc)
{
int status;
struct prison *pr;
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
uint32_t flags;
rwatsonUnsubmitted
Not Done
Inline Actions

It 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…
if ((td == NULL) && (proc == NULL))
return (true);
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
pr = pax_get_prison(td, proc);
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
flags = (td != NULL) ? td->td_proc->p_pax : proc->p_pax;
if (((flags & 0xaaaaaaaa) & ((flags & 0x55555555) << 1)) != 0) {
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
pax_log_aslr(pr, __func__, "inconsistent paxflags: %x\n", flags);
rwatsonUnsubmitted
Not Done
Inline Actions

Please 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…
lattera-gmail.comAuthorUnsubmitted
Not Done
Inline Actions

Will be address in the next patch.

lattera-gmail.com: Will be address in the next patch.
opUnsubmitted
Not Done
Inline Actions

I 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...
rwatsonUnsubmitted
Not Done
Inline Actions

Then 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…
opUnsubmitted
Not Done
Inline Actions

Okay!

op: Okay!
pax_ulog_aslr(pr, NULL, "inconsistent paxflags: %x\n", flags);
return (true);
}
if (pr != NULL)
rwatsonUnsubmitted
Not Done
Inline Actions

Please 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…
rwatsonUnsubmitted
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…
status = pr->pr_pax_aslr_status;
else
status = pax_aslr_status;
switch (status) {
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
case PAX_ASLR_DISABLED:
return (false);
case PAX_ASLR_FORCE_ENABLED:
return (true);
case PAX_ASLR_OPTIN:
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
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,
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
"ASLR is opt-in, and executable does not have ASLR enabled\n");
return (false);
}
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
break;
case PAX_ASLR_OPTOUT:
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
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,
"ASLR is opt-out, and executable explicitly disabled ASLR\n");
return (false);
}
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
break;
default:
return (true);
}
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
return (true);
}
void
rwatsonUnsubmitted
Not Done
Inline Actions

Is this KASSERT() needed?

rwatson: Is this KASSERT() needed?
_pax_aslr_init(struct vmspace *vm, struct prison *pr)
{
if (vm == NULL)
panic("[PaX ASLR] %s: vm == NULL", __func__);
rwatsonUnsubmitted
Not Done
Inline Actions

Add blank line.

rwatson: Add blank line.
vm->vm_aslr_delta_mmap = PAX_ASLR_DELTA(arc4random(),
rwatsonUnsubmitted
Not Done
Inline Actions

Is this local variable needed?

rwatson: Is this local variable needed?
lattera-gmail.comAuthorUnsubmitted
Not Done
Inline Actions

No. We'll remove it in the next patch.

lattera-gmail.com: No. We'll remove it in the next patch.
PAX_ASLR_DELTA_MMAP_LSB, (pr != NULL) ?
pr->pr_pax_aslr_mmap_len :
pax_aslr_mmap_len);
rwatsonUnsubmitted
Not Done
Inline Actions

Here'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…
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(),
PAX_ASLR_DELTA_EXEC_LSB, (pr != NULL) ?
pr->pr_pax_aslr_exec_len :
pax_aslr_exec_len);
if ((pr != NULL) && pr->pr_pax_aslr_debug) {
rwatsonUnsubmitted
Not Done
Inline Actions

len_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…
lattera-gmail.comAuthorUnsubmitted
Not Done
Inline Actions

Will be addressed in the next patch.

lattera-gmail.com: Will be addressed in the next patch.
pax_log_aslr(pr, __func__, "vm_aslr_delta_mmap=%p\n",
(void *) vm->vm_aslr_delta_mmap);
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",
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
(void *) vm->vm_aslr_delta_mmap);
rwatsonUnsubmitted
Not Done
Inline Actions

SDT probe.

rwatson: SDT probe.
pax_ulog_aslr(pr, NULL, "vm_aslr_delta_stack=%p\n",
(void *) vm->vm_aslr_delta_stack);
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
pax_ulog_aslr(pr, NULL, "vm_aslr_delta_exec=%p\n",
(void *) vm->vm_aslr_delta_exec);
}
rwatsonUnsubmitted
Not Done
Inline Actions

Don't use 'true' and 'false' in the kernel.
Remove blank line.

rwatson: Don't use 'true' and 'false' in the kernel. Remove blank line.
}
#ifdef COMPAT_FREEBSD32
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line; true/false.

rwatson: Remove blank line; true/false.
void
rwatsonUnsubmitted
Not Done
Inline Actions

Don't use true/false in the kernel.

rwatson: Don't use true/false in the kernel.
_pax_aslr_init32(struct vmspace *vm, struct prison *pr)
opUnsubmitted
Not Done
Inline Actions

I think there is a new convergence in the kernel:
https://github.com/freebsd/freebsd/commit/b0d837707d8b28197e3d0e7fbc48095e09e2e77a
https://github.com/freebsd/freebsd/commit/22dd25965647f50284835a47a6213410eceb38b1

op: I think there is a new convergence in the kernel: https://github.
{
if (vm == NULL)
panic("[PaX ASLR] %s: vm == NULL", __func__);
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) ?
rwatsonUnsubmitted
Not Done
Inline Actions

Remove 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) ?
pr->pr_pax_aslr_compat_exec_len :
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
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);
pax_log_aslr(pr, __func__, "vm_aslr_delta_stack=%p\n",
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
(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);
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
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);
}
}
rwatsonUnsubmitted
Not Done
Inline Actions

Use SDT probes.

rwatson: Use SDT probes.
#endif
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__);
rwatsonUnsubmitted
Not Done
Inline Actions

Add blank line here.

rwatson: Add blank line here.
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)
{
struct prison *pr;
if (!pax_aslr_active(td, NULL))
return;
pr = pax_get_prison(td, NULL);
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;
rwatsonUnsubmitted
Not Done
Inline Actions

Remove 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);
}
}
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
void
pax_aslr_stack(struct thread *td, uintptr_t *addr)
{
struct prison *pr;
uintptr_t orig_addr;
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
if (!pax_aslr_active(td, NULL))
return;
pr = pax_get_prison(td, NULL);
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
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",
(void *)orig_addr, (void *)*addr);
pax_ulog_aslr(pr, NULL, "orig_addr=%p, new_addr=%p\n",
rwatsonUnsubmitted
Not Done
Inline Actions

Use SDT probes.

rwatson: Use SDT probes.
(void *)orig_addr, (void *)*addr);
}
kibUnsubmitted
Not Done
Inline Actions

You 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…
rwatsonUnsubmitted
Not Done
Inline Actions

Was this comment addressed?

rwatson: Was this comment addressed?
opUnsubmitted
Not Done
Inline Actions

Not yet, but after the latest sys_mmap cleanup from jhb@ I changed and cleaned up the MAP_32BIT part.
I can solve this, if I add 3 new fields to vmspace, which could contain the randoms for compat / MAP_32BIT cases. I not really like to truncate the original randoms to fit to MAP_32BIT mappings.

op: Not yet, but after the latest sys_mmap cleanup from jhb@ I changed and cleaned up the MAP_32BIT…
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
rwatsonUnsubmitted
Not Done
Inline Actions

Use SDT probe.

rwatson: Use SDT probe.
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
rwatsonUnsubmitted
Not Done
Inline Actions

Comment 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…
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
rwatsonUnsubmitted
Not Done
Inline Actions

Use SDT probe.

rwatson: Use SDT probe.
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
rwatsonUnsubmitted
Not Done
Inline Actions

Comment should be a complete sentence.
As above, would be preferable to just use prison0.pr_hardening... elsewhere rather than globals, avoiding code replication and the opportunity for maintenance errors.

rwatson: Comment should be a complete sentence. As above, would be preferable to just use prison0.
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
rwatsonUnsubmitted
Not Done
Inline Actions

Use SDT probe.

rwatson: Use SDT probe.
rwatsonUnsubmitted
Not Done
Inline Actions

style(9) does not like variable declarations in blocks.

rwatson: style(9) does not like variable declarations in blocks.
rwatsonUnsubmitted
Not Done
Inline Actions

Use SDT probe.

rwatson: Use SDT probe.
rwatsonUnsubmitted
Not Done
Inline Actions

Use SDT probe.

rwatson: Use SDT probe.
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
rwatsonUnsubmitted
Not Done
Inline Actions

Call variable 'addrp' indicating that it is a pointer to an address.

rwatson: Call variable 'addrp' indicating that it is a pointer to an address.
rwatsonUnsubmitted
Not Done
Inline Actions

Use SDT probe.

rwatson: Use SDT probe.
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
rwatsonUnsubmitted
Not Done
Inline Actions

Name 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.
rwatsonUnsubmitted
Not Done
Inline Actions

I 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…
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.
rwatsonUnsubmitted
Not Done
Inline Actions

Make comment a sentence.
Is this a violation of an invariant?

rwatson: Make comment a sentence. Is this a violation of an invariant?
rwatsonUnsubmitted
Not Done
Inline Actions

Remove blank line.

rwatson: Remove blank line.