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

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

In D58102#1434470 <https://reviews.llvm.org/D58102#1434470>, @xiangzhangllvm wrote:

> In D58102#1433093 <https://reviews.llvm.org/D58102#1433093>, @peter.smith wrote:
>
> > I guess you mean "I tried", not "I tend" in 1). I think that you can do accomplish 1.) via unconditionally creating the In.GnuPropertySection and SPltSection and by implementing the empty() member function, ensure that they are removed by removeUnusedSyntheticSections() if there isn't anything that uses them. Unconditionally creating the sections means that you don't need to call mergeAggregateMetaData from Driver.cpp prior to createSyntheticSections(). For the case of ifunc where you need to turn off the feature then just inform In.GnuPropertySection and In.SPltSection as they will always be there.
>
>
> Hi peter, I have implemented your idea in my code, and it work well, I did not update here now because I am not sure about the ifunc.
>  One of my destination is to remove the Config->X86Feature1AND and do the feature collection work inside synthetic GnuPropertySection class.
>  I found I can remove the Config->X86Feature1AND except dealing with ifunc, If I remove the Config->X86Feature1AND I still need another global variable.
>  At this stage I want to disable CET when the program will use ifunc, my current code is Relocations.cpp:1072
>
>   // To Be Done.
>   // Now the IBT can't work with IFUNC feature, because they may conflict in
>   // changing the PLT. So we disable the IBT when the IFUNC enabled.
>   if (Sym.isGnuIFunc()) {
>     Config->X86Feature1AND &= ~GNU_PROPERTY_X86_FEATURE_1_IBT;
>     Config->SPltSectionEnable = false;
>   }
>   
>
> Do you have some good idea to turn off the CET feature in ifunc ?
>  Thank you very much!


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?


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D58102