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. 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. > + return FFI_OK; > + } > > > -- > jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE >
