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.
>
> 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.
A quick check shows this in dlmmap:
assert (start == NULL && length % malloc_getpagesize == 0
&& prot == (PROT_READ | PROT_WRITE)
&& flags == (MAP_PRIVATE | MAP_ANONYMOUS)
&& fd == -1 && offset == 0);
Note the length % malloc_getpagesize == 0 bits. It *appears* that yes,
each closure gets exclusive pages.
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
