Stuart Henderson <[email protected]> writes:
> This disables PROT_EXEC mappings in libffi (and thus python).
> I'm running with it in a bulk build with the "mandatory W^X"
> printfs that are going into snapshots and haven't triggered
> them yet, building python itself (done 2.7 and 3.4 so far)
> or in the ~200 py-* and py3-* things that have built already
> (I would have had a whole stack by now otherwise).
>
> There are a lot of test failures when this diff is used.
> Can anyone figure out if they're anything to worry about?
So I think the problem is simply that since the closure isn't allocated
with PROT_EXEC permissions, you can't... execute it.
I'm kinda discovering libffi so sorry in advance if I'm mistaken, but it
seems that the logical steps are always:
- ffi_closure_alloc
- ffi_prep_closure_loc
- ffi_closure_free
ffi_prep_closure_loc being the function that actually writes machine
code intended to be executed later. I think we could tweak
ffi_prep_closure_loc so that it adjusts the permissions after having
done its work: move from PROT_READ|PROT_WRITE (what we get now with the
closures.c diff) to PROT_READ|PROT_EXEC.
ffi_prep_closure is just a stub that calls ffi_prep_closure_loc, only
the latter can handle different addresses for writable and executable
user pages. That seems to be used for situations when ffi_closure_alloc
calls dlmmap_locked under the hood.
Here's a tentative diff, I'm not sure whether it will be the best
solution in the end, but it doesn't seem to be worse than the current
patch:
--8<--
Running ../../testsuite/libffi.call/call.exp ...
FAIL: libffi.call/nested_struct10.c -W -Wall -O2 (test for excess errors)
FAIL: libffi.call/nested_struct10.c -W -Wall -O3 (test for excess errors)
FAIL: libffi.call/nested_struct10.c -W -Wall -O2 -fomit-frame-pointer (test for
excess errors)
FAIL: libffi.call/unwindtest.cc -W -Wall -O0 execution test
FAIL: libffi.call/unwindtest.cc -W -Wall -O2 execution test
FAIL: libffi.call/unwindtest.cc -W -Wall -O3 execution test
FAIL: libffi.call/unwindtest.cc -W -Wall -Os execution test
FAIL: libffi.call/unwindtest.cc -W -Wall -O2 -fomit-frame-pointer execution test
=== libffi Summary ===
# of expected passes 1857
# of unexpected failures 8
-->8--
Diff for amd64 only, but it shouldn't be hard to patch all platforms.
Thoughts?
(yeah, the "return FFI_OK;" part doesn't look right)
Index: Makefile
===================================================================
RCS file: /cvs/ports/devel/libffi/Makefile,v
retrieving revision 1.34
diff -u -p -r1.34 Makefile
--- Makefile 23 Mar 2016 22:50:29 -0000 1.34
+++ Makefile 23 May 2016 11:46:59 -0000
@@ -3,7 +3,7 @@
COMMENT= Foreign Function Interface
DISTNAME= libffi-3.2.1
-REVISION= 1
+REVISION= 2
SHARED_LIBS += ffi 1.2 # .6.4
CATEGORIES= devel
Index: patches/patch-src_closures_c
===================================================================
RCS file: patches/patch-src_closures_c
diff -N patches/patch-src_closures_c
--- /dev/null 1 Jan 1970 00:00:00 -0000
+++ patches/patch-src_closures_c 23 May 2016 11:46:59 -0000
@@ -0,0 +1,56 @@
+$OpenBSD$
+--- src/closures.c.orig Sat Nov 8 05:47:24 2014
++++ src/closures.c Sat May 21 15:57:22 2016
+@@ -172,41 +172,6 @@ selinux_enabled_check (void)
+
+ #endif /* !FFI_MMAP_EXEC_SELINUX */
+
+-/* On PaX enable kernels that have MPROTECT enable we can't use PROT_EXEC. */
+-#ifdef FFI_MMAP_EXEC_EMUTRAMP_PAX
+-#include <stdlib.h>
+-
+-static int emutramp_enabled = -1;
+-
+-static int
+-emutramp_enabled_check (void)
+-{
+- char *buf = NULL;
+- size_t len = 0;
+- FILE *f;
+- int ret;
+- f = fopen ("/proc/self/status", "r");
+- if (f == NULL)
+- return 0;
+- ret = 0;
+-
+- while (getline (&buf, &len, f) != -1)
+- if (!strncmp (buf, "PaX:", 4))
+- {
+- char emutramp;
+- if (sscanf (buf, "%*s %*c%c", &emutramp) == 1)
+- ret = (emutramp == 'E');
+- break;
+- }
+- free (buf);
+- fclose (f);
+- return ret;
+-}
+-
+-#define is_emutramp_enabled() (emutramp_enabled >= 0 ? emutramp_enabled \
+- : (emutramp_enabled = emutramp_enabled_check
()))
+-#endif /* FFI_MMAP_EXEC_EMUTRAMP_PAX */
+-
+ #elif defined (__CYGWIN__) || defined(__INTERIX)
+
+ #include <sys/mman.h>
+@@ -216,9 +181,7 @@ emutramp_enabled_check (void)
+
+ #endif /* !defined(X86_WIN32) && !defined(X86_WIN64) */
+
+-#ifndef FFI_MMAP_EXEC_EMUTRAMP_PAX
+-#define is_emutramp_enabled() 0
+-#endif /* FFI_MMAP_EXEC_EMUTRAMP_PAX */
++#define is_emutramp_enabled() 1
+
+ /* Declare all functions defined in dlmalloc.c as static. */
+ static void *dlmalloc(size_t);
Index: patches/patch-src_x86_ffi64_c
===================================================================
RCS file: patches/patch-src_x86_ffi64_c
diff -N patches/patch-src_x86_ffi64_c
--- /dev/null 1 Jan 1970 00:00:00 -0000
+++ patches/patch-src_x86_ffi64_c 23 May 2016 11:46:59 -0000
@@ -0,0 +1,27 @@
+$OpenBSD$
+--- src/x86/ffi64.c.orig Mon May 23 13:42:49 2016
++++ src/x86/ffi64.c Mon May 23 13:42:56 2016
+@@ -27,6 +27,9 @@
+ DEALINGS IN THE SOFTWARE.
+ ----------------------------------------------------------------------- */
+
++#include <sys/types.h>
++#include <sys/mman.h>
++
+ #include <ffi.h>
+ #include <ffi_common.h>
+
+@@ -563,6 +566,13 @@ ffi_prep_closure_loc (ffi_closure* closure,
+ closure->cif = cif;
+ closure->fun = fun;
+ closure->user_data = user_data;
++
++ /* remove WRITE access on the closure */
++ if (mprotect(closure, sizeof(*closure), PROT_READ) == -1)
++ return FFI_OK;
++ /* ensure we have EXECUTE access on the closure */
++ if (mprotect(closure, sizeof(*closure), PROT_READ|PROT_EXEC) == -1)
++ return FFI_OK;
+
+ return FFI_OK;
+ }
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
