[PATCH] D76158: Add inline assembly load hardening mitigation for Load Value Injection (LVI) on X86 [6/6]

[Matthew Riley via Phabricator via llvm-commits]

mattdr accepted this revision.
mattdr added a comment.

In D76158#2004182 <https://reviews.llvm.org/D76158#2004182>, @sconstab wrote:

> The coverage of the LVI mitigations for LLVM should match the coverage of the mitigations implemented for binutils. Both should adhere to the guidance outlined in this document: https://software.intel.com/security-software-guidance/insights/deep-dive-load-value-injection.


I mean, yes, sure, I want that too, but that doc just says the Intel patches to GNU binutils work by "by inserting an LFENCE instruction after each instruction that performs a load". And the binutils implementation //does seem// to be trying to make that true?

> Mitigations have only been recommended for SGX enclave code. Since SGX enclaves do not support far procedure calls and branches (and therefore far returns), these features do not need to be covered by either mitigation tool.

That reasoning also tracks, but then here is the change where binutils explicitly adds support for LRET: https://github.com/bminor/binutils-gdb/commit/a09f656b267b9a684f038fba7cadfe98e2f18892#diff-c3c1bdcf15ebcd70b899275b3486272bR4594

Anyway, if the controlling factor is "we are only mitigating code that works in SGX and that excludes instructions like <blah>" that at least merits a comment for posterity. Otherwise -- because these are just magic lists of instructions without an explanation of how they were arrived at -- there is no way to tell the difference between an oversight and a purposeful omission, and others will probably independently come to the same conclusions @craig.topper did.

My "accept" remains with previously-described provisos.


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

https://reviews.llvm.org/D76158