Stefan Kempf <[email protected]> writes: > Jeremie Courreges-Anglas wrote: >> 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: > >> [...] > >> 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; > > The second PROT_READ|PROT_EXEC mprotect should be enough.
Enough for us, but I wrote the diff with the intent that it could be fed upstream, even if that may sound premature at this time. > Is ffi_closure_alloc() guaranteed to allocate a separate page, i.e. does > every closure get its own page? Because if a page can contain multiple > ffi_closures, they would all get mapped to PROT_READ|PROT_EXEC > at once => any later call of calling ffi_prep_closure_loc for those > other closures would cause a segfault. I thought about that but did not investigate. Anyway, Theo's last mail and my reply make me think that this is not the direction we're taking for the time being. In the long run, I'm interested in trying to move this to true W^X support. >> + return FFI_OK; >> + } >> >> >> -- >> jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE >> > -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
