[PATCH] D49383: [cfi-verify] Add an option to treat calls to a specified function as traps.

[Joel Galenson via Phabricator via llvm-commits]

jgalenson added a comment.

Thanks for the comments!

In https://reviews.llvm.org/D49383#1174223, @eugenis wrote:

> It would be really nice for llvm object library to support PLT detection/parsing.
>
> Just for cfi-verify tool, we can cheat and link with -Wl,-emit-relocs. We already rely on line tables to tell data from code, so the tool does not work on fully stripped production executables anyway.


What exactly does -Wl,-emit-relocs do?  Does it force the linker to keep the rela.plt section?  There's not much we can do without that flag, is there?



================
Comment at: tools/llvm-cfi-verify/lib/FileAnalysis.cpp:55
 
+static cl::opt<std::string> TrapFunction(
+    "trap-function",
----------------
pcc wrote:
> Since we know what all of the names of the "trap functions" are, does this really need to be a flag?
> 
> Also, `__cfi_slowpath` technically isn't a trap function so maybe there's a better name for this concept.
Good point; I added this specifically for __cfi_slowpath at plt.  If we want to hardcode that and other trap functions, that's fine by me.  What other functions should I add?  abort?

It's true that __cfi_slowpath isn't exactly a trap function, but it acts like one in the context of cfi-verify, as it will trap if and only if the call is invalid.  I guess I can try to rename this to make that clearer (e.g., willTrapOnIllegalCall or something).


================
Comment at: tools/llvm-cfi-verify/lib/FileAnalysis.cpp:563
+  // We begin by finding the base address of the PLT.
+  if (!TrapName.endswith("@plt"))
+    return make_error<StringError>("Could not find trap function",
----------------
eugenis wrote:
> TrapName should be just a function name.
> "@plt" is assembly convention, the user of the tool should not care whether the call is local or external.
True.  But the whole rest of this function only looks for functions in the PLT, which end with a "@plt" suffix (see line 598, which I added to match GNU objdump's notation).  So I just added this as an optimization to avoid this logic if it's not needed.


================
Comment at: tools/llvm-cfi-verify/lib/FileAnalysis.cpp:576
+  // This section has the same order as the PLT, so if we find it, we can
+  // compute its offset from the base address of the PLT.
+  const object::SectionRef *RelaPLT =
----------------
eugenis wrote:
> I'm not sure you can rely on the order of these sections being the same.
> 
> Generally speaking, you need to disassemble each PLT entry and find the indirect branch instruction that points to the address of the corresponding .rela.plt relocation.
> 
> This sounds complicated, but there are only a few _recommended_ PLT instruction sequences, usually found in SYSV architecture supplement documents.
> 
> Maybe relying on .rela.plt order is not such a bad idea after all, provided that all linkers behave the same way.
I'm definitely not an expert on ELF files, and I couldn't find out whether these sections always have the same order (besides a random StackOverflow answer saying they did).

Disassembling the PLT does sound complicated, especially for something like this that should be as architecture-independent as possible.

Should we just assume they have the same order, or should we look into this more?


================
Comment at: tools/llvm-cfi-verify/lib/FileAnalysis.cpp:580
+        StringRef Str;
+        return !Section.getName(Str) && Str == ".rela.plt";
+      });
----------------
eugenis wrote:
> .rela.plt can be found by DT_JMPREL entry in dynamic section.
> 
> It's very uncommon, but the section table can be stripped from the binary.
Okay, I'll look into using the ELF::DT_JMPREL flag instead of this.  There's no way to do something like that to find the PLT sections, is there?

If the section table is stripped, this code will fail to find the section and fail, so at least we don't get incorrect results.


Repository:
  rL LLVM

https://reviews.llvm.org/D49383