On Monday, May 23, 2016 14:00 CEST, Jeremie Courreges-Anglas <j...@wxcvbn.org>
wrote:
> Stuart Henderson <s...@spacehopper.org> 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
>
I was looking what the gnustep conftest is doing, and I also found
the ffi_closure_alloc function, that comment on the function says:
/* Allocate a chunk of memory with the given size. Returns a pointer
to the writable address, and sets *CODE to the executable
corresponding virtual address. */
void *
ffi_closure_alloc (size_t size, void **code)
but both code and the return value point to the same address.
whereas I think those should differ?
Then ffi_prep_closure_loc should do the right thing?
Sebastian
> 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
>
