[X86] Prevent -mibt-seal to work together with -flto=thin

[joaomoreira]

Summary

As pointed out by @samitolvanen and then confirmed by @nickdesaulniers here - https://github.com/ClangBuiltLinux/linux/issues/1737 - -flto=thin does not reliably know about address-taken properties across different translation units. This breaks the assumptions behind the -mibt-seal optimization, that removes ENDBR instructions from prologues of functions which are observed as non-address-taken.

Thus, because of the above, prevent -mibt-seal from being used together with -flto=thin by only enabling it if lto is Full.

[joaomoreira]

Regarding https://github.com/ClangBuiltLinux/linux/issues/1737:

Weirdly enough, I double-tested the behavior for -flto=thin + -mibt-seal; the kernel did boot fine on my setup, but when dumped/grep'ed for ENDBRs, it had ~500 less ENDBRs throughout the binary. Considering that LTO-based optimizations applied with -flto=thin should have also been applied with -flto (which means, things like dead-code elimination should be symmetrical in both generated binaries), and also given the confirmation that -flto=thin won't properly keep address-taken details across translation units, my understanding is that -mibt-seal must be disabled when -flto=thin. Thus, submitted the patch even though I was not able to reproduce the bad behavior.

[samitolvanen]

In D140035#3995703, @joaomoreira wrote:

Regarding https://github.com/ClangBuiltLinux/linux/issues/1737:

Weirdly enough, I double-tested the behavior for -flto=thin + -mibt-seal; the kernel did boot fine on my setup, but when dumped/grep'ed for ENDBRs, it had ~500 less ENDBRs throughout the binary

Did you confirm the issue with the reproducer in the CBL bug? It would be interesting to find out why you couldn't reproduce this in the kernel.

[joaomoreira]

Weirdly enough, I double-tested the behavior for -flto=thin + -mibt-seal; the kernel did boot fine on my setup, but when dumped/grep'ed for ENDBRs, it had ~500 less ENDBRs throughout the binary

Did you confirm the issue with the reproducer in the CBL bug? It would be interesting to find out why you couldn't reproduce this in the kernel.

Yes, the reproducer from CBL highlights the issue. I tested it long ago and forgot to add the detail here, yet it should by itself suffice as a motivation for this fix. Thanks for bringing that up.

Regarding not being able to reproduce this in kernel -- never mind... I was misled by setup issues while running IBT kernels in QEMU. I managed to fix the setup and confirm that kernel won't boot. Thanks for pushing this bit too.

Also, FWIIW, objtool alerts about a bunch of relocations pointing to !endbr instructions when compiling with -flto=thin. When compiling with -flto+-mibt-seal, the only alert is for a data relocation to !non-endbr towards x86_64_start_kernel, which doesn't seem to be a concern since (already under the fixed setup) the kernel still doesn't trip.

[samitolvanen]

In D140035#3996066, @joaomoreira wrote:

Regarding not being able to reproduce this in kernel -- never mind... I was misled by setup issues while running IBT kernels in QEMU. I managed to fix the setup and confirm that kernel won't boot.

OK, great. Thanks for double checking!

The patch itself looks good to me, but I suspect ibt-seal in general has the same issue as D138337 where it can drop endbr instructions from isUsedInRegularObj symbols that are not address-taken in the bitcode (e.g. functions whose address is only taken in stand-alone assembly). I saw this issue only in the arm64 Linux kernel, but there's always a chance a similar code pattern emerges on the x86 side at some point in future too. This can obviously be worked around in the kernel, but just something to keep in mind.

clang/lib/Frontend/CompilerInvocation.cpp

1856

Please run git clang-format for the patch.

[joaomoreira]

In D140035#3998954, @samitolvanen wrote:

In D140035#3996066, @joaomoreira wrote:

Regarding not being able to reproduce this in kernel -- never mind... I was misled by setup issues while running IBT kernels in QEMU. I managed to fix the setup and confirm that kernel won't boot.

OK, great. Thanks for double checking!

The patch itself looks good to me, but I suspect ibt-seal in general has the same issue as D138337 where it can drop endbr instructions from isUsedInRegularObj symbols that are not address-taken in the bitcode (e.g. functions whose address is only taken in stand-alone assembly). I saw this issue only in the arm64 Linux kernel, but there's always a chance a similar code pattern emerges on the x86 side at some point in future too. This can obviously be worked around in the kernel, but just something to keep in mind.

Ugh, yeah. I reproduced the behavior with a .s and a .C file on a similar scheme as the repro you wrote for the thin lto problem. I'll start looking for a fix once I'm back from vacation in Jan. Tks for pointing this out.

[MaskRay]

I did not notice this patch (and https://github.com/ClangBuiltLinux/linux/issues/1737#issuecomment-1310741237) but make a comment yesterday while studying CFI schemes in llvm-project: https://reviews.llvm.org/D116070#4004060

As mentioned, the isUsedInRegularObj issue makes -mibt-seal not working with regular LTO as well. So it works with neither LTO mode.
I suggest that we remove the option until the issues are all sorted out.

[joaomoreira]

Given https://reviews.llvm.org/D140363, this patch is being abandoned.

clang/lib/Driver/ToolChains/Clang.cpp

   if (Args.hasArg(options::OPT_nogpulib))
     CmdArgs.push_back("-nogpulib");
 
   if (Arg *A = Args.getLastArg(options::OPT_fcf_protection_EQ)) {
     CmdArgs.push_back(
         Args.MakeArgString(Twine("-fcf-protection=") + A->getValue()));
   }
 
-  if (IsUsingLTO)
+  if (LTOMode == LTOK_Full)
     Args.AddLastArg(CmdArgs, options::OPT_mibt_seal);
 
   if (Arg *A = Args.getLastArg(options::OPT_mfunction_return_EQ))
     CmdArgs.push_back(
         Args.MakeArgString(Twine("-mfunction-return=") + A->getValue()));
 
   Args.AddLastArg(CmdArgs, options::OPT_mindirect_branch_cs_prefix);
 

clang/lib/Frontend/CompilerInvocation.cpp

else if (Val == llvm::FunctionReturnThunksKind::Extern &&
              Args.getLastArgValue(OPT_mcmodel_EQ).equals("large"))
       Diags.Report(diag::err_drv_argument_not_allowed_with)
           << A->getAsString(Args)
           << Args.getLastArg(OPT_mcmodel_EQ)->getAsString(Args);
     else
       Opts.FunctionReturnThunks = static_cast<unsigned>(Val);
   }
 
-  if (Opts.PrepareForLTO && Args.hasArg(OPT_mibt_seal))
+  if (Opts.PrepareForLTO &&
+      !Opts.PrepareForThinLTO &&
+      Args.hasArg(OPT_mibt_seal)) {
     Opts.IBTSeal = 1;
+  }
 
   for (auto *A :
        Args.filtered(OPT_mlink_bitcode_file, OPT_mlink_builtin_bitcode)) {
     CodeGenOptions::BitcodeFileToLink F;
     F.Filename = A->getValue();
     if (A->getOption().matches(OPT_mlink_builtin_bitcode)) {
       F.LinkFlags = llvm::Linker::Flags::LinkOnlyNeeded;
       // When linking CUDA bitcode, propagate function attributes so that

clang/test/CodeGen/X86/x86-cf-protection.c

 // RUN: %clang_cc1 -E -triple i386 -dM -o - -fcf-protection=return %s | FileCheck %s --check-prefix=RETURN
 // RUN: %clang_cc1 -E -triple i386 -dM -o - -fcf-protection=branch %s | FileCheck %s --check-prefix=BRANCH
 // RUN: %clang_cc1 -E -triple i386 -dM -o - -fcf-protection=full %s   | FileCheck %s --check-prefix=FULL
 // RUN: %clang_cc1 -emit-llvm -triple i386 -o - -fcf-protection=branch -mibt-seal -flto %s | FileCheck %s --check-prefixes=CFPROT,IBTSEAL
+// RUN: %clang_cc1 -emit-llvm -triple i386 -o - -fcf-protection=branch -mibt-seal -flto=thin %s | FileCheck %s --check-prefixes=CFPROT,NOIBTSEAL
 // RUN: %clang_cc1 -emit-llvm -triple i386 -o - -fcf-protection=branch -flto %s | FileCheck %s --check-prefixes=CFPROT,NOIBTSEAL
 // RUN: %clang_cc1 -emit-llvm -triple i386 -o - -fcf-protection=branch -mibt-seal %s | FileCheck %s --check-prefixes=CFPROT,NOIBTSEAL
 // RUN: not %clang_cc1 -emit-llvm-only -triple i386 -target-cpu pentium-mmx -fcf-protection=branch %s 2>&1 | FileCheck %s --check-prefix=NOCFPROT
 
 // RETURN: #define __CET__ 2
 // BRANCH: #define __CET__ 1
 // FULL: #define __CET__ 3
 // CFPROT: !{i32 8, !"cf-protection-branch", i32 1}
 // IBTSEAL: !{i32 8, !"ibt-seal", i32 1}
 // NOIBTSEAL-NOT: "ibt-seal", i32 1
 
 // NOCFPROT: error: option 'cf-protection=branch' cannot be specified on this target
 
 void foo() {}