[PATCH] D119296: KCFI sanitizer

[Sami Tolvanen via Phabricator via cfe-commits]

samitolvanen marked an inline comment as done.
samitolvanen added inline comments.


================
Comment at: clang/lib/Driver/SanitizerArgs.cpp:63
     SanitizerKind::Unreachable | SanitizerKind::Return;
-static const SanitizerMask AlwaysRecoverable =
-    SanitizerKind::KernelAddress | SanitizerKind::KernelHWAddress;
+static const SanitizerMask AlwaysRecoverable = SanitizerKind::KernelAddress |
+                                               SanitizerKind::KernelHWAddress |
----------------
MaskRay wrote:
> samitolvanen wrote:
> > MaskRay wrote:
> > > This is incorrect.
> > > 
> > > If a violation is found, ud2 is executed. ud2 is not followed by normal control flow so I don't think recovery from the error is supported.
> > > 
> > > This seems like `Unrecoverable`
> > This variable is only used to indicate whether `-fno-sanitize-recover` command line parameter can be used with the sanitizer. It makes no sense to allow this with KCFI as we always emit a recoverable instruction sequence, hence it's included here.
> > 
> > Also, ud2 absolutely is recoverable in the kernel, and Linux specifically uses ud2 to trigger warnings in assembly code.
> ud2 being recoverable in the kernel is insufficient. The IR should consider this recoverable. In the presence of a failure, the control flow should be transferred as if no failure happens. E.g. for an asan out-of-bounds failure, the code should behave as if the failure is ignored.
> ud2 being recoverable in the kernel is insufficient. The IR should consider this recoverable. In the presence of a failure, the control flow should be transferred as if no failure happens. E.g. for an asan out-of-bounds failure, the code should behave as if the failure is ignored.

Sure, that's exactly now the code we emit here works as well. There are no traps emitted to the IR code, we only add an operand bundle to indirect calls, which doesn't affect the control flow as far as the IR is concerned.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119296