Page MenuHomeFreeBSD
Differential D473 Diff 1838 sys/kern/sys_process.c

Changeset View

Standalone View

View Options

sys/kern/sys_process.c

Show All 27 Lines
* OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
* SUCH DAMAGE. * SUCH DAMAGE.
*/ */
#include <sys/cdefs.h> #include <sys/cdefs.h>
__FBSDID("$FreeBSD$"); __FBSDID("$FreeBSD$");
#include "opt_compat.h" #include "opt_compat.h"
#include "opt_pax.h"
#include <sys/param.h> #include <sys/param.h>
#include <sys/systm.h> #include <sys/systm.h>
#include <sys/lock.h> #include <sys/lock.h>
#include <sys/mutex.h> #include <sys/mutex.h>
#include <sys/syscallsubr.h> #include <sys/syscallsubr.h>
#include <sys/sysent.h> #include <sys/sysent.h>
#include <sys/sysproto.h> #include <sys/sysproto.h>
#include <sys/pax.h>
#include <sys/priv.h> #include <sys/priv.h>
#include <sys/proc.h> #include <sys/proc.h>
#include <sys/procctl.h> #include <sys/procctl.h>
#include <sys/vnode.h> #include <sys/vnode.h>
#include <sys/ptrace.h> #include <sys/ptrace.h>
#include <sys/rwlock.h> #include <sys/rwlock.h>
#include <sys/sx.h> #include <sys/sx.h>
#include <sys/malloc.h> #include <sys/malloc.h>
▲ Show 20 Lines • Show All 595 Lines • ▼ Show 20 Lines#ifdef COMPAT_FREEBSD32
struct ptrace_lwpinfo32 *pl32 = NULL; struct ptrace_lwpinfo32 *pl32 = NULL;
struct ptrace_lwpinfo plr; struct ptrace_lwpinfo plr;
#endif #endif
curp = td->td_proc; curp = td->td_proc;
/* Lock proctree before locking the process. */ /* Lock proctree before locking the process. */
switch (req) { switch (req) {
#ifdef PAX_ASLR
case PT_PAX:
#endif
impUnsubmitted
Not Done
Inline Actions

Doesn't need to be an ifdef.

imp: Doesn't need to be an ifdef.
lattera-gmail.comAuthorUnsubmitted
Not Done
Inline Actions

I'll remove this. I'll replace with a static inline function with a body that just returns, defined in sys/sys/pax.h

lattera-gmail.com: I'll remove this. I'll replace with a static inline function with a body that just returns…
lattera-gmail.comAuthorUnsubmitted
Not Done
Inline Actions

I decided to go with a NULL macro instead of a static inline function. This is reflected in the new patch that was uploaded today.

lattera-gmail.com: I decided to go with a NULL macro instead of a static inline function. This is reflected in the…
case PT_TRACE_ME: case PT_TRACE_ME:
case PT_ATTACH: case PT_ATTACH:
case PT_STEP: case PT_STEP:
case PT_CONTINUE: case PT_CONTINUE:
case PT_TO_SCE: case PT_TO_SCE:
case PT_TO_SCX: case PT_TO_SCX:
case PT_SYSCALL: case PT_SYSCALL:
case PT_FOLLOW_FORK: case PT_FOLLOW_FORK:
case PT_DETACH: case PT_DETACH:
sx_xlock(&proctree_lock); sx_xlock(&proctree_lock);
proctree_locked = 1; proctree_locked = 1;
break; break;
default: default:
break; break;
} }
write = 0; write = 0;
if (req == PT_TRACE_ME) { if (req == PT_TRACE_ME || req == PT_PAX) {
rwatsonUnsubmitted
Not Done
Inline Actions

PT_PAX seems inconsistent with other ptrace() operations .. I wonder if this is being done right?

rwatson: PT_PAX seems inconsistent with other ptrace() operations .. I wonder if this is being done…
p = td->td_proc; p = td->td_proc;
PROC_LOCK(p); PROC_LOCK(p);
} else { } else {
if (pid <= PID_MAX) { if (pid <= PID_MAX) {
if ((p = pfind(pid)) == NULL) { if ((p = pfind(pid)) == NULL) {
if (proctree_locked) if (proctree_locked)
sx_xunlock(&proctree_lock); sx_xunlock(&proctree_lock);
return (ESRCH); return (ESRCH);
▲ Show 20 Lines • Show All 50 Lines • ▼ Show 20 Lines if (SV_PROC_FLAG(td2->td_proc, SV_ILP32))
safe = 1; safe = 1;
wrap32 = 1; wrap32 = 1;
} }
#endif #endif
/* /*
* Permissions check * Permissions check
*/ */
switch (req) { switch (req) {
case PT_PAX:
impUnsubmitted
Not Done
Inline Actions

Again, is a MAC check needed here?

imp: Again, is a MAC check needed here?
lattera-gmail.comAuthorUnsubmitted
Not Done
Inline Actions

No.

lattera-gmail.com: No.
/* securelevel should be 0 to allow this */
rwatsonUnsubmitted
Not Done
Inline Actions

Why disallow this at higher securelevels?

rwatson: Why disallow this at higher securelevels?
lattera-gmail.comAuthorUnsubmitted
Not Done
Inline Actions

Because allowing a user to disable ASLR is risky. I would like to impose some restrictions on what the user can do in regards to using an API to disable ASLR.

lattera-gmail.com: Because allowing a user to disable ASLR is risky. I would like to impose some restrictions on…
rwatsonUnsubmitted
Not Done
Inline Actions

Lots of things are risky in UNIX, but aren't controlled by securelevels. In general, securelevels limit root privileges rather than user privileges, and does so in order to protect the TCB (e.g., the kernel, raw filesystem access, etc) from manipulation that might be used to disable securelevels. It's not clear to me that this qualifies.

rwatson: Lots of things are risky in UNIX, but aren't controlled by securelevels. In general…
rwatsonUnsubmitted
Not Done
Inline Actions

Use a sentence.
"should" -> "must".

rwatson: Use a sentence. "should" -> "must".
if ((data & PAX_NOTE_NOASLR) == PAX_NOTE_NOASLR) {
if (securelevel_gt(p->p_ucred, 0) != 0) {
rwatsonUnsubmitted
Not Done
Inline Actions

p->p_ucred is almost never the right credential to use for access control -- do you mean td->td_ucred? You do hold PROC_LOCK(p) here, so if you mean p->p_ucred, you should have a PROC_LOCK_ASSERT() at the top of this 'case' to document the requirement for the lock.

rwatson: p->p_ucred is almost never the right credential to use for access control -- do you mean td…
lattera-gmail.comAuthorUnsubmitted
Not Done
Inline Actions

Switching that to use the thread should be no problem. Will address in the next patch.

lattera-gmail.com: Switching that to use the thread should be no problem. Will address in the next patch.
rwatsonUnsubmitted
Not Done
Inline Actions

This appears not to have been addressed.

rwatson: This appears not to have been addressed.
error = EPERM;
rwatsonUnsubmitted
Not Done
Inline Actions

Why not use the return value from securelevel_gt()?

rwatson: Why not use the return value from securelevel_gt()?
lattera-gmail.comAuthorUnsubmitted
Not Done
Inline Actions

Can do. Will address in the next patch.

lattera-gmail.com: Can do. Will address in the next patch.
goto fail;
}
}
break;
case PT_TRACE_ME: case PT_TRACE_ME:
/* Always legal. */ /* Always legal. */
break; break;
case PT_ATTACH: case PT_ATTACH:
/* Self */ /* Self */
if (p->p_pid == td->td_proc->p_pid) { if (p->p_pid == td->td_proc->p_pid) {
error = EINVAL; error = EINVAL;
▲ Show 20 Lines • Show All 69 Lines • ▼ Show 20 Lines#endif
/* /*
* Actually do the requests * Actually do the requests
*/ */
td->td_retval[0] = 0; td->td_retval[0] = 0;
switch (req) { switch (req) {
case PT_PAX:
p->p_pax = data;
break;
rwatsonUnsubmitted
Not Done
Inline Actions

Add blank line.

rwatson: Add blank line.
case PT_TRACE_ME: case PT_TRACE_ME:
/* set my trace flag and "owner" so it can read/write me */ /* set my trace flag and "owner" so it can read/write me */
p->p_flag |= P_TRACED; p->p_flag |= P_TRACED;
if (p->p_flag & P_PPWAIT) if (p->p_flag & P_PPWAIT)
p->p_flag |= P_PPTRACE; p->p_flag |= P_PPTRACE;
p->p_oppid = p->p_pptr->p_pid; p->p_oppid = p->p_pptr->p_pid;
break; break;
▲ Show 20 Lines • Show All 598 Lines • Show Last 20 Lines