[PATCH] D58102: Support X86 Control-flow Enforcement Technology (CET) in LLD

[Xiang Zhang via Phabricator via llvm-commits]

xiangzhangllvm added a comment.

In D58102#1434642 <https://reviews.llvm.org/D58102#1434642>, @peter.smith wrote:

> As I understand it, what you have in this review (not the patch attached as a comment).
>
> - Driver.cpp calls mergeAggregateMetaData() to set the Config->SpltSectionEnabled and Config->X86Feature1AND
>   - We know that all input objects have the feature property set.
> - Writer.cpp creates the In.SpltSection() and the In.GnuProperty() section as we know that will be needed.
> - If we encounter an ifunc then we disable IBT by setting Config->SpltSectionEnabled to false and clearing Config->X86Feature1AND
>
>   One thing I don't understand yet, apologies I've not had time to go into the details of how the splt section works, is that if you discover an ifunc mid-way through processing then you'll have inserted many entries into the In.Splt section, then disabled it, from then on you'll just use the In.Plt. Will this result in a broken PLT as it looks like we'll call get getPltVA() if Config->SpltSectionEnabled and I'm not sure if that will return the right thing if we've previously added an entry into the In.Splt section. I don't see any existing tests for ifunc handling, can you add a test case that does something like:
> - Enable SpltSection
> - Generate a In.SpltSection entry
> - encounter an Ifunc
> - Disable SpltSection
> - Generate a In.Plt entry for some other symbol
> - Check that the generated PLT makes sense.
>
>   If this bit is broken then I think you'll need to find a way of handling this case, which won't be easy. I can think of a few approaches:
> - Do a pre-scan for likely ifuncs, before calling scanRelocs. This could be simpler but more pessimistic than the existing code.
> - Have a way that the In.SpltSection decays into an In.PltSection gracefully if an ifunc is detected.
>
>   Going back to your original question, how about:
> - Writer.cpp at construction of In.GnuPropertySection calls mergeAggregateMetaData() and sets GnuPropertySection::Feature1AND and possibly SpltSection::enable
>   - As you've implemented empty() these sections can be unconditionally created, although it doesn't matter too much if you conditionally create them, in that case In.GnuPltSection == nullptr is implicity Feature1AND=0 and In.SpltSection == nullptr is implicitly enableSpltSection == false
> - If we encounter an ifunc then we disable IBT by setting In.SpltSection::Enable to false and setting In.GnuPropertySection::X86Feature1AND to 0. If you've not created the sections then you'll obviously need to test that they exist first. If I've understood correctly if the sections don't exist then there is nothing to disable anyway.
>
>   Please let me know if I've misunderstood something critical?


Hi, Peter, very appreciate for your suggestion,  for the Splt, in fact, it is just the entry of the old PLT, you can see in this way: SPLT+PLT=(old)PLT, we split the (old)Plt to Plt +SPlt, one of the reasons is for performance that we want to put them into different areas (hot code, cold code) .
And in this patch, I used getSPltVA() instead of getPltVA when face outside. For example in  getRelocTargetVA():

  case R_PLT:
    // When the CET Branch protection is enabled, the second PLT
    // should be call to.
    if (Config->SPltSectionEnable)
      return Sym.getSPltVA() + A;
    else
      return Sym.getPltVA() + A;

Maybe it's hard to use In.SpltSection::Enable to do these.
Any way I will check it.
Thank you very much!


Repository:
  rLLD LLVM Linker

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58102/new/

https://reviews.llvm.org/D58102