Page MenuHomePhabricator

Add test suite for the Control Flow Integrity feature.
ClosedPublic

Authored by pcc on Feb 18 2015, 2:35 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

pcc updated this revision to Diff 20224.Feb 18 2015, 2:35 PM
pcc retitled this revision from to Add test suite for the Control Flow Integrity feature..
pcc updated this object.
pcc edited the test plan for this revision. (Show Details)
pcc added reviewers: kcc, jfb.
pcc added a subscriber: Unknown Object (MLST).
jfb edited edge metadata.Feb 18 2015, 2:44 PM

I'm not familiar with how these tests usually work: is the intent just to see if they don't crash? IIUC there's no correctness check, right?

test/cfi/overwrite.cpp
18 ↗(On Diff #20224)

Could you explain this test?

pcc added a comment.Feb 18 2015, 2:54 PM

The purpose of this test suite is mostly to check that the enforcement mechanism works, i.e. that certain things (e.g. calls through classes of the wrong type, or calls through things that look like vtables but aren't) cause the program to crash. I'll add a few comments to each test case to explain what it is testing.

test/cfi/overwrite.cpp
18 ↗(On Diff #20224)

The test overwrites a's virtual function pointer with a pointer to fake_vtable and attempts to make a call through it. If the enforcement mechanism is working, the program will crash (checked via not --crash above).

Lovely!

+samsonov for cmake code.
Questions:

can we make these tests run with -O0 and -O2 w/o adding extra RUN lines? 
what will happen with the tests if LTO is not supported?
test/cfi/anon-namespace.cpp
4 ↗(On Diff #20224)

Hm... I would actually like to have a way to validate that we have an expected crash, not some unrelated crash (e.g. due to miscompile).
The simplest way is to add two printfs, one before and one after the vcall, and check that only the first one is there

6 ↗(On Diff #20224)

Why does it require that now? Is that an LTO limitation?

test/cfi/overwrite.cpp
2 ↗(On Diff #20224)

I would like to run this test with and w/o CFI, so that w/o CFI we actually see foo() called.

test/cfi/simple-fail.cpp
2 ↗(On Diff #20224)

Same here and for all other --crash tests: I would like to also run these tests w/o CFI to see that they "work".
I't ok that the test has undefined behavior according to the standard. We are testing the expected behavior of the particular compiler.

test/cfi/simple-pass.cpp
50 ↗(On Diff #20224)

we need to ensure that the calls are not devirtualized.
e.g. check break_optimization in test/asan/TestCases/Linux/sized_delete_test.cc

pcc updated this revision to Diff 20230.Feb 18 2015, 3:48 PM
pcc edited edge metadata.
  • Add comments and a test for virtual inheritance
  • Address Kostya's comments
test/cfi/anon-namespace.cpp
4 ↗(On Diff #20224)

Done

6 ↗(On Diff #20224)

Added a comment explaining.

test/cfi/overwrite.cpp
2 ↗(On Diff #20224)

Done

test/cfi/simple-fail.cpp
2 ↗(On Diff #20224)

Done

test/cfi/simple-pass.cpp
50 ↗(On Diff #20224)

Done

pcc added a comment.Feb 18 2015, 3:50 PM

can we make these tests run with -O0 and -O2 w/o adding extra RUN lines?

Probably not, but I don't think it would make a difference at the moment. As far as I know, the optimization level chosen at compile time does not affect the level used at link time.

what will happen with the tests if LTO is not supported?

There is logic in lit.cfg to disable the tests if LTO is not supported.

kcc added a comment.Feb 18 2015, 4:29 PM

Please also add tests with multiple inheritance

pcc updated this revision to Diff 20246.Feb 18 2015, 6:00 PM
  • Add multiple inheritance test
jfb added a comment.Feb 19 2015, 10:32 AM

The NCFI tests are checking that UB doesn't cause a crash. It would be nice for the test to point out exactly where the UB is so that if the test starts failing it's easy to figure out why.

kcc added a comment.Feb 19 2015, 10:56 AM

Looks great.
Once Alexey LGTMs the cmake code you have my LGTM too.
Please, whenever you get any kind of question about the CFI add a test here that answers the question.
Of course, this patch depends on the other two.

The question about -O0 vs -O2 is more to Alexey.
I think we do have similar cmake magic somewhere, don't we?
It would be great to run all these tests with both -O0 and -O2

test/cfi/anon-namespace.cpp
43 ↗(On Diff #20246)

#endif // TU1
here and below

test/cfi/multiple-inheritance.cpp
35 ↗(On Diff #20246)

If you use stderr you will not need fflush

pcc updated this revision to Diff 20358.Feb 19 2015, 4:35 PM
  • Mark UB
  • Use stderr
  • endif comments
test/cfi/anon-namespace.cpp
43 ↗(On Diff #20246)

Done

test/cfi/multiple-inheritance.cpp
35 ↗(On Diff #20246)

Done

jfb added a comment.Feb 19 2015, 5:29 PM

lgtm too.

samsonov added inline comments.Feb 19 2015, 5:57 PM
test/cfi/CMakeLists.txt
11 ↗(On Diff #20358)

Would this work if LLVM_BINUTILS_INCDIR is unavailable?

test/cfi/lit.cfg
8 ↗(On Diff #20358)

This should be set in test/lit.common.cfg

9 ↗(On Diff #20358)
os.path.dirname(__file__)

?

10 ↗(On Diff #20358)

You probably don't need test_exec_root here.

11 ↗(On Diff #20358)

There is no such directory at the moment

29 ↗(On Diff #20358)

We have config.clang for that

32 ↗(On Diff #20358)

ditto

38 ↗(On Diff #20358)

This should be handled by lit.common.cfg as well: we add llvm_tools_dir to PATH

pcc updated this revision to Diff 20421.Feb 20 2015, 11:21 AM
  • Address review comments
test/cfi/CMakeLists.txt
11 ↗(On Diff #20358)

Probably not, fixed.

test/cfi/lit.cfg
8 ↗(On Diff #20358)

Done

9 ↗(On Diff #20358)

Done

10 ↗(On Diff #20358)

Done

11 ↗(On Diff #20358)

Done

29 ↗(On Diff #20358)

Done

32 ↗(On Diff #20358)

Done

38 ↗(On Diff #20358)

Done

samsonov accepted this revision.Feb 20 2015, 12:17 PM
samsonov edited edge metadata.

LGTM

This revision is now accepted and ready to land.Feb 20 2015, 12:17 PM
This revision was automatically updated to reflect the committed changes.
kcc added a comment.Feb 20 2015, 3:01 PM

None of these tests have 32+ or 64+ vfunctions.
In a separate commit please add some larger tests to cover the case with more vfunctions.