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

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard 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))
----------------
labrinea wrote:
> 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.
The mitigation is performed in the backend, which is run from the linker when doing LTO.

> Also, I believe that -flto and -mcmse are incompatible options.

Fair enough


================
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");
----------------
labrinea wrote:
> 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.
I agree with Sjoerd, the ""last option wins" policy should be implemented in the driver, and only the winning option passed through to CC1. You can use `Args.hasFlag` instead of `getLastArg` here, with the default set based on the CPU option.


================
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 |\
----------------
labrinea wrote:
> 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.
This still sounds like an inconsistency which will catch out users migrating between GCC and clang. I'd prefer that we match GCC's behaviour, though I'd also be OK with leaving it like this as long as these defaults are clearly documented in ClangCommandLineReference.rst.


================
Comment at: llvm/lib/Target/ARM/ARMExpandPseudoInsts.cpp:1564
               .addReg(Reg)
+              .addReg(ARM::CPSR, RegState::ImplicitDefine)
               .add(predOps(ARMCC::AL));
----------------
labrinea wrote:
> ostannard wrote:
> > Why are these needed?
> These prevent the reordering with the mitigation sequence. It answers your next question.
Do you mean that this is modeling the effect of this instruction on the CONTROL register, to prevent it from being re-ordered with the MRS instruction? If so, then this is unrelated to my next question, and could do with a comment because I wouldn't expect any CONTROL bits to be part of ARM::CPSR (they are different registers).


================
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)
----------------
labrinea wrote:
> 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?
My concern is that these two instructions (IT and VMOV) have to be adjacent, otherwise the IT would apply to some other instruction, but I don't think there's anything here which guarantees that. I don't think a regression test is enough here, because the re-ordering could happen with a future scheduling model  not covered by the tests, or with different code to the tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109157