This is an archive of the discontinued LLVM Phabricator instance.

[unittest] Skip W+X MappedMemoryTests when MPROTECT is enabled
ClosedPublic

Authored by mgorny on Nov 3 2018, 2:23 PM.

Details

Summary

Skip all MappedMemoryTest variants that rely on memory pages being
mapped for MF_WRITE|MF_EXEC when MPROTECT is enabled on NetBSD. W^X
protection causes all those mmap() calls to fail, causing the tests
to fail.

Diff Detail

Repository
rL LLVM

Event Timeline

mgorny created this revision.Nov 3 2018, 2:23 PM
krytarowski added inline comments.Nov 3 2018, 2:36 PM
unittests/Support/MemoryTest.cpp
18 ↗(On Diff #172505)

sys/param.h before sys/types.h according to the NetBSD style.

Please decorate this include section with
// clang-format off
and
// clang-format on

38 ↗(On Diff #172505)

Using assert() might have a negative impact on functionality... because under some conditions this code might not be compiled at all and sysctl(3) won't be called.

I propose to go for:

if (sysctl(mib, 3, &paxflags, &len, NULL, 0) != 0)
  err(EXIT_FAILURE, "sysctl");

(and include <err.h>)

91 ↗(On Diff #172505)

I would drop : \ here. Some compilers will complain about redundant ;.

223 ↗(On Diff #172505)

Flags & Memory::MF_EXEC?

+ hfinkel for JIT API reference.

mgorny updated this revision to Diff 172507.Nov 3 2018, 2:43 PM

Implemented all comments + moved CHECK_UNSUPPORTED below faster R+W checks.

mgorny marked 4 inline comments as done.Nov 3 2018, 2:44 PM
krytarowski added inline comments.Nov 3 2018, 2:51 PM
unittests/Support/MemoryTest.cpp
90 ↗(On Diff #172507)

I would further refactor this into:

// MPROTECT prevents W+X mmaps
#define CHECK_UNSUPPORTED(f) \
	​  do { \
	​    if ((f & (Memory::MF_WRITE | Memory::MF_EXEC)) && \
	​        IsMPROTECT()) \
	​      return; \
	​  } while (0)

And use CHECK_UNSUPPORTED(Flags) or CHECK_UNSUPPORTED(Flags | Memory::MF_WRITE) for EnabledWrite.

mgorny added inline comments.Nov 3 2018, 3:00 PM
unittests/Support/MemoryTest.cpp
90 ↗(On Diff #172507)

Nah, that's too smart. Would go against KISS.

krytarowski added inline comments.Nov 3 2018, 3:07 PM
unittests/Support/MemoryTest.cpp
90 ↗(On Diff #172507)

How about?

#define CHECK_UNSUPPORTED_RAW(f) \
  ​  do { \
  ​    if ((f & (Memory::MF_WRITE | Memory::MF_EXEC)) && \
  ​        IsMPROTECT()) \
  ​      return; \
  ​  } while (0)

and

#define CHECK_UNSUPPORTED() CHECK_UNSUPPORTED_RAW(Flag)

I would like to get rid of open-coding check in EnabledWrite.

In EnableWrite it would be: CHECK_UNSUPPORTED_RAW(Flags | Memory::MF_WRITE).

mgorny added inline comments.Nov 3 2018, 3:11 PM
unittests/Support/MemoryTest.cpp
90 ↗(On Diff #172507)

Same problem — it says 'write' when it checks for 'exec'. We could get away by making an extra function with RHS specified via parameter with a default but I don't think it's worth the hassle. The purpose of the macro is to avoid repeating the most common version, not solve all the problems. Note that some functions have additional 'skip conditions' which we leave intact.

krytarowski accepted this revision.Nov 3 2018, 3:14 PM
krytarowski added inline comments.
unittests/Support/MemoryTest.cpp
90 ↗(On Diff #172507)

OK, I defer this to others in review.

This revision is now accepted and ready to land.Nov 3 2018, 3:14 PM

@hfinkel I would still like to drop W|X support from MemoryBlock for everybody, restricting this class to swapping between RW and RX in JIT code. For those who need RWX allocations it's more convenient to add a dedicated allocator (and NetBSD can support it). It's just a matter of time when new features on security hardened OSes will break and lose feature parity.

@hfinkel any comments on this? If not I recommend to land this patch as it is.

This revision was automatically updated to reflect the committed changes.