Re: W^X mmap

[Pierre Pronchery <khorben%defora.org@localhost>]

			Hi,

On 10/12/2016 14:02, Michael van Elst wrote:
> coypu%SDF.ORG@localhost writes:
>
> > Why doesn't the following code get rejected by pax mprotect?
>
> > 	a = mmap(NULL, BUFSIZ, PROT_READ | PROT_WRITE | PROT_EXEC, MAP_ANON, -1, 2);
>
> It gets 'rejected' by silently dropping the PROT_EXEC flag.

I find this awful: programs trying to use e.g JIT will fail to detect 
that it really is not supported, and crash later instead.

I am attaching here a patch returning errors instead.

Thanks to this patch, www/firefox works without having to set the "m: 
mprotect(2) restrictions, explicit disable" flag on its executable 
binaries (tested on netbsd-7/amd64).

> POSIX would require mmap to fail with errno = EACCES.

In the patch attached I have used ENOTSUP, because this is what OpenBSD 
seems to be using:
http://man.openbsd.org/mmap.2

I also think EACCES (or EPERM?) would be better though, so I will be 
happy to replace it if considered more appropriate.

I have changed the logic deciding which flags to drop. It used to be, 
independently of whether PROT_READ is set:
- if PROT_WRITE, or PROT_WRITE and PROT_EXECUTE are set, then execution
  is silently denied;
- otherwise, writing is silently denied.
(which doesn't make much sense to me)

Now there would be only one case instead:
- if PROT_WRITE and PROT_EXECUTE are set, execution is denied and an
  error is returned.

Another thing I will really need to know before committing this, is 
whether the changes should really be applied to sys_mmap() only. 
Finally, I left a XXX where there might be a side-effect, if applied in 
sys/kern/exec_subr.c, after calling vn_rdwr().

Cheers,
--
khorben

diff --git a/sys/kern/exec_subr.c b/sys/kern/exec_subr.c
index efd0b5e..b523288 100644
--- a/sys/kern/exec_subr.c
+++ b/sys/kern/exec_subr.c
@@ -185,7 +185,9 @@ vmcmd_map_pagedvn(struct lwp *l, struct exec_vmcmd *cmd)
 	prot = cmd->ev_prot;
 	maxprot = UVM_PROT_ALL;
 #ifdef PAX_MPROTECT
-	pax_mprotect(l, &prot, &maxprot);
+	error = pax_mprotect(l, &prot, &maxprot);
+	if (error)
+		return error;
 #endif /* PAX_MPROTECT */
 
 	/*
@@ -267,7 +269,10 @@ vmcmd_readvn(struct lwp *l, struct exec_vmcmd *cmd)
 	prot = cmd->ev_prot;
 	maxprot = VM_PROT_ALL;
 #ifdef PAX_MPROTECT
-	pax_mprotect(l, &prot, &maxprot);
+	/* XXX side-effects with vn_rdwr()? */
+	error = pax_mprotect(l, &prot, &maxprot);
+	if (error)
+		return error;
 #endif /* PAX_MPROTECT */
 
 #ifdef PMAP_NEED_PROCWR
@@ -327,7 +332,9 @@ vmcmd_map_zero(struct lwp *l, struct exec_vmcmd *cmd)
 	prot = cmd->ev_prot;
 	maxprot = UVM_PROT_ALL;
 #ifdef PAX_MPROTECT
-	pax_mprotect(l, &prot, &maxprot);
+	error = pax_mprotect(l, &prot, &maxprot);
+	if (error)
+		return error;
 #endif /* PAX_MPROTECT */
 
 	error = uvm_map(&p->p_vmspace->vm_map, &cmd->ev_addr,
diff --git a/sys/kern/kern_pax.c b/sys/kern/kern_pax.c
index 2b1bc12..e84838a 100644
--- a/sys/kern/kern_pax.c
+++ b/sys/kern/kern_pax.c
@@ -263,26 +263,28 @@ pax_init(void)
 }
 
 #ifdef PAX_MPROTECT
-void
+int
 pax_mprotect(struct lwp *l, vm_prot_t *prot, vm_prot_t *maxprot)
 {
 	uint32_t f;
 
 	if (!pax_mprotect_enabled)
-		return;
+		return 0;
 
 	f = l->l_proc->p_pax;
 	if ((pax_mprotect_global && (f & ELF_NOTE_PAX_NOMPROTECT) != 0) ||
 	    (!pax_mprotect_global && (f & ELF_NOTE_PAX_MPROTECT) == 0))
-		return;
+		return 0;
 
-	if ((*prot & (VM_PROT_WRITE|VM_PROT_EXECUTE)) != VM_PROT_EXECUTE) {
+	if ((*prot & (VM_PROT_WRITE|VM_PROT_EXECUTE))
+			== (VM_PROT_WRITE|VM_PROT_EXECUTE)) {
 		*prot &= ~VM_PROT_EXECUTE;
 		*maxprot &= ~VM_PROT_EXECUTE;
-	} else {
-		*prot &= ~VM_PROT_WRITE;
-		*maxprot &= ~VM_PROT_WRITE;
+		log(LOG_ALERT, "PaX MPROTECT: [%d] Enforcing W^X\n",
+			    l->l_proc->p_pid);
+		return ENOTSUP;
 	}
+	return 0;
 }
 #endif /* PAX_MPROTECT */
 
diff --git a/sys/sys/pax.h b/sys/sys/pax.h
index 0c28b46..e8b1d13 100644
--- a/sys/sys/pax.h
+++ b/sys/sys/pax.h
@@ -47,7 +47,7 @@ struct vmspace;
 
 void pax_init(void);
 
-void pax_mprotect(struct lwp *, vm_prot_t *, vm_prot_t *);
+int pax_mprotect(struct lwp *, vm_prot_t *, vm_prot_t *);
 int pax_segvguard(struct lwp *, struct vnode *, const char *, bool);
 
 #define	PAX_ASLR_DELTA(delta, lsb, len)	\
diff --git a/sys/uvm/uvm_mmap.c b/sys/uvm/uvm_mmap.c
index 8043f0f..5291ab5 100644
--- a/sys/uvm/uvm_mmap.c
+++ b/sys/uvm/uvm_mmap.c
@@ -416,7 +416,10 @@ sys_mmap(struct lwp *l, const struct sys_mmap_args *uap, register_t *retval)
 	}
 
 #ifdef PAX_MPROTECT
-	pax_mprotect(l, &prot, &maxprot);
+	error = pax_mprotect(l, &prot, &maxprot);
+	if (error) {
+		goto out;
+	}
 #endif /* PAX_MPROTECT */
 
 #ifdef PAX_ASLR
diff --git a/sys/uvm/uvm_unix.c b/sys/uvm/uvm_unix.c
index 7ac5579..b6d1c7e 100644
--- a/sys/uvm/uvm_unix.c
+++ b/sys/uvm/uvm_unix.c
@@ -103,7 +103,11 @@ sys_obreak(struct lwp *l, const struct sys_obreak_args *uap, register_t *retval)
 		vm_prot_t maxprot = UVM_PROT_ALL;
 
 #ifdef PAX_MPROTECT
-		pax_mprotect(l, &prot, &maxprot);
+		error = pax_mprotect(l, &prot, &maxprot);
+		if (error) {
+			mutex_exit(&p->p_auxlock);
+			return (error);
+		}
 #endif /* PAX_MPROTECT */
 
 		error = uvm_map(&vm->vm_map, &old, new - old, NULL,