[PATCH] D59780: Support Intel Control-flow Enforcement Technology

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

In D59780#1780753 <https://reviews.llvm.org/D59780#1780753>, @xiangzhangllvm wrote:

> In D59780#1778988 <https://reviews.llvm.org/D59780#1778988>, @MaskRay wrote:
>
> > Apply two patches
> >
> >   -  uint64_t Plt = In.Plt->getVA();
> >   +  uint64_t Plt = In.IBTPlt ? In.IBTPlt->getVA() : In.Plt->getVA();
> >
> >
> >
> >
> >   --- i/lld/ELF/Driver.cpp
> >   +++ w/lld/ELF/Driver.cpp
> >   @@ -1705,2 +1705,4 @@ template <class ELFT> static uint32_t getAndFeatures() {
> >   -    } else if (!features && config->requireCET)
> >   -      error(toString(f) + ": --require-cet: file is not compatible with CET");
> >   +    } else if (config->requireCET && !(features & GNU_PROPERTY_X86_FEATURE_1_IBT)) {
> >   +      warn(toString(f) + ": --require-cet: file is not compatible with CET");
> >   +      features |= GNU_PROPERTY_X86_FEATURE_1_IBT;
> >   +    }
> >
> >
> > @xiangzhangllvm The previous version I uploaded definitely did not work. I just applied a fix. Both i386 and x86_64 seem to work now, but there are still problems with the tests.
> >
> > You need either `arc patch D59780` or `curl -L 'https://reviews.llvm.org/D59780?download=true' | patch -p1` to apply the latest diff in your local repository.
> >
> > I have a separate patch to rename lld `--force-bti` to `-z force-bti`. I think `--force-ibt` or `-z force-ibt` may make more sense than `--require-cet`.
>
>
> Thank you very much!
>  I checked the "X86-64 fail tests", the reason is just that you changed "error" to "warn".
>
>   `--force-ibt` maybe use, (but to change `--force-ibt` to `--force-cet`, because 'cet' contains 'ibt' and 'shstk')
>
> `--require-cet` is also needed, (strictly check the input files, make sure run ok in CET machines).
>  And I think it better to add `--try-cet` too, try to generate CET output if all inputs is CETed, this must be useful in bootstrap compiling program. (To let the whole program CETed step by step).
>
> Any way, we can add the other "refined options" late, this time, just let '-force-cet' in.


x86 IBT is similar to AArch64 BTI. Whether we should upgrade from `-z force-ibt` to `-z force-cet` depends on whether shstk requires support in the object files for correctness.. Here is my understanding.

- `GNU_PROPERTY_X86_FEATURE_1_IBT` represents the capability of IBT in an object file. The output is IBT capable only if all object files have the bit. `GNU_PROPERTY_AARCH64_FEATURE_1_BTI` belongs to this category. So naming the relevant option `-z force-ibt` makes sense.
- `GNU_PROPERTY_X86_FEATURE_1_SHSTK` represents that an object file desires SHSTK. It does not require support in the object files for correctness. `GNU_PROPERTY_AARCH64_FEATURE_1_PAC_PLT` belongs to this category.

I would name the two options `-z force-ibt` and `-z shstk`. I can imagine that shstk may have non-negligible overhead, and probably not everyone wants to enable it. So having separate options makes sense.

(GNU ld supports `-z ibt` and `-z cet-report=[none|warning|error]`. If shstk, as I think, does not require object files' support, I will consider it not so ideal. The name `-z ibt` does not make it clear that this forces ibt when some object files are not IBT capable. `-z cet-report` mixes intentions of two features.)

If @ruiu still accepts the patch in the first place, I may go with `-z force-ibt` and `-z shstk`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59780