This is an archive of the discontinued LLVM Phabricator instance.

Parse DWARF information to reduce false positives.
ClosedPublic

Authored by hctim on Oct 6 2017, 5:16 PM.

Details

Summary

Help differentiate code and data by parsing DWARF information. This will reduce false positive rates where data is placed in executable sections and is mistakenly parsed as code, resulting in an inflation in the number of indirect CF instructions (and hence an inflation of the number of unprotected).

Also prints the DWARF line data around the region of each indirect CF instruction.

Diff Detail

Repository
rL LLVM

Event Timeline

hctim created this revision.Oct 6 2017, 5:16 PM
hctim edited the summary of this revision. (Show Details)
hctim updated this revision to Diff 118116.Oct 6 2017, 5:52 PM

backported a clang-format

hctim updated this revision to Diff 118426.Oct 10 2017, 10:53 AM

Merged previous changes in the stack.

probinson added inline comments.
tools/llvm-cfi-verify/FileAnalysis.cpp
325 ↗(On Diff #118426)

Does this tool run only on .o files, and not on executables? Because this decides line info is valid if any CU has a non-empty line table. If you are looking at a linked executable, one might have been compiled -g and another not, and this check will still succeed.

331 ↗(On Diff #118426)

Type units can have an associated line table, but that's just to provide filespecs for DW_AT_file. There should never be any Rows in a type unit's line table.

hctim updated this revision to Diff 119706.Oct 20 2017, 1:58 PM
hctim marked 2 inline comments as done.
  • Modified tool to ensure that users are aware that all statically linked libraries are required to be compiled with '-g'.
  • Removed type unit check (thanks probison)!
  • Changed opt<uint64_t> to opt<unsigned long long> (known issue with opt<uint64_t>).
  • Lexicographically sorted library names.
tools/llvm-cfi-verify/FileAnalysis.cpp
325 ↗(On Diff #118426)

The goal for this tool is to run on both.

Thanks for reminding me :) I've had discussions with people about this exact topic offline, but forgot to update the commits.

The consensus we reached was in order to do elimination-by-dwarf (i.e. having --ignore-dwarf=false), anything that is statically linked into this binary is required to be compiled with -g. I'll update the documentation for this tool soon and add a comment around this section.

331 ↗(On Diff #118426)

Thanks for the information, will remove the redundant check.

hctim updated this revision to Diff 119937.Oct 23 2017, 2:12 PM

Patched from upstream.

pcc added inline comments.Oct 24 2017, 2:54 PM
tools/llvm-cfi-verify/lib/FileAnalysis.cpp
54 ↗(On Diff #119937)

I don't think you need cl::location or the external declaration of IgnoreDWARF, because this flag is only used in this file.

unittests/tools/llvm-cfi-verify/FileAnalysis.cpp
485 ↗(On Diff #119937)

Am I missing something, or should these new tests include DWARF information?

hctim updated this revision to Diff 120161.Oct 24 2017, 6:06 PM
hctim marked an inline comment as done.
  • Fixed diff to be against the correct changelist (prev in stack instead of master).
  • Removed external declaration of command line flag .
pcc edited edge metadata.Oct 26 2017, 1:27 PM

Should this include a test case?

hctim updated this revision to Diff 120501.Oct 26 2017, 3:38 PM
  • Moved DWARF handlers into virtual functions for mocking.
  • Added unit tests for line table based instruction exclusion.
hctim updated this revision to Diff 120688.Oct 27 2017, 1:43 PM
  • Removed dependency injection onto DWARF lookups from unit tests.
  • Added lit test suite for integration testing.
  • Added lit tests for checking line table information.
hctim updated this revision to Diff 120689.Oct 27 2017, 1:45 PM

Fixed small formatting issue from revert of dependency injection changes.

hctim updated this revision to Diff 120899.Oct 30 2017, 3:07 PM

Removed tests that require clang with plugin/LTO support. Made the tests build from the supplied assembly instead.

pcc added inline comments.Oct 31 2017, 10:55 AM
test/tools/llvm-cfi-verify/X86/protected-lineinfo.s
21 ↗(On Diff #120899)

I'd prefer if the output were smaller. Do you get a smaller output if you compile with -gmlt?

hctim updated this revision to Diff 121020.Oct 31 2017, 11:09 AM

Compiled integration tests with -gmlt instead of -g

hctim marked an inline comment as done.Oct 31 2017, 11:10 AM
pcc added a comment.Oct 31 2017, 11:31 AM

I'd expect there to be a test which makes sure that a jump with no line info in a file with line info doesn't count as an unprotected jump. Can you add one?

test/tools/llvm-cfi-verify/X86/protected-nolineinfo.s
1 ↗(On Diff #121020)

I don't think you need more than one test for the line table check. I'd drop this one and keep the other one.

hctim updated this revision to Diff 121034.Oct 31 2017, 12:15 PM
hctim marked an inline comment as done.

Added test where a file contains line table information, but an indirect CF instruction is not covered by the LT information.

pcc accepted this revision.Oct 31 2017, 3:43 PM

LGTM

This revision is now accepted and ready to land.Oct 31 2017, 3:43 PM
This revision was automatically updated to reflect the committed changes.