[PATCH] D109157: [ARM] Mitigate the cve-2021-35465 security vulnurability.

[Alexandros Lamprineas via Phabricator via cfe-commits]

labrinea added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:1665
+
+    CmdArgs.push_back("-mllvm");
+    if (A->getOption().matches(options::OPT_mfix_cmse_cve_2021_35465))
----------------
ostannard wrote:
> Are these optional also being passed through to the linker when doing LTO?
No, the mitigation is only performed in the compiler. Also, I believe that `-flto` and `-mcmse` are incompatible options.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:1666
+    CmdArgs.push_back("-mllvm");
+    if (A->getOption().matches(options::OPT_mfix_cmse_cve_2021_35465))
+      CmdArgs.push_back("-arm-fix-cmse-cve-2021-35465=1");
----------------
SjoerdMeijer wrote:
> If `-mcpu=cortex-[m33|m35|m55]` was provided, then `-arm-fix-cmse-cve-2021-35465=1` is already set and we are adding another option here? For example, for
> 
>   -mcpu=cortex-m33 -mcmse -mfix-cmse-cve-2021-35465
> 
> I am expecting:
> 
>   "-mllvm" "-arm-fix-cmse-cve-2021-35465=1"  "-mllvm" "-arm-fix-cmse-cve-2021-35465=1" 
> 
> and with `-mno-fix-cmse-cve-2021-35465`:
> 
>    "-mllvm" "-arm-fix-cmse-cve-2021-35465=1"  "-mllvm" "-arm-fix-cmse-cve-2021-35465=0" 
> 
> Probably it's nicer to just pass this once.
> 
> Also, in the tests, I think cases are missing for `-mcpu=...` and `-m[no-]fix-cmse-cve-2021-35465`.
Your interpretetion is correct. The established policy is "last option wins", but I could make the Driver pass only one option if that's preferable.


================
Comment at: clang/test/Driver/arm-cmse-cve-2021-35465.c:3
+//
+// RUN: %clang --target=arm-arm-none-eabi -march=armv8-m.main %s -### \
+// RUN:   -mcmse -mno-fix-cmse-cve-2021-35465 2>&1 |\
----------------
ostannard wrote:
> The last few paragraphs of https://developer.arm.com/support/arm-security-updates/vlldm-instruction-security-vulnerability claim that this is enabled by default for -march=armv8-m.main in AC6 and GCC, is there a reason we're not matching that?
Yes, the inconsistency lies on the fact that GCC implements the mitigation in library code, therefore it is always present for `-march=armv8-m.main`, whereas in llvm there's no such limitation. We've contacted the authors of this page to rectify the documentation.


================
Comment at: llvm/lib/Target/ARM/ARMExpandPseudoInsts.cpp:1564
               .addReg(Reg)
+              .addReg(ARM::CPSR, RegState::ImplicitDefine)
               .add(predOps(ARMCC::AL));
----------------
ostannard wrote:
> Why are these needed?
These prevent the reordering with the mitigation sequence. It answers your next question.


================
Comment at: llvm/lib/Target/ARM/ARMExpandPseudoInsts.cpp:1626
+      // Thumb2ITBlockPass will not recognise the instruction as conditional.
+      BuildMI(MBB, MBBI, DL, TII->get(ARM::t2IT))
+          .addImm(ARMCC::NE)
----------------
ostannard wrote:
> This pass runs before the final scheduler pass, so is there a risk that the IT and VMOV instructions could be moved apart? I think it would be safer to either put the IT instruction inside the inline asm block, or make this a new pseudo-instruction expanded in the asm printer.
The use of `.addReg(ARM::CPSR, RegState::ImplicitDefine)` prevents the reordering. There are regression tests in place that check the mitigation sequence ordering.

Is this what you meant? Where you refering specifically to the case where `!STI->hasFPRegs()`, when we generate inline asm, or to both scenarios?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109157