[ARM] Fix CMSE S->NS calls when CONTROL_S.SFPA==0 (CVE-2024-7883) #114433

ostannard · 2024-10-31T17:32:02Z

When doing a call from CMSE secure state to non-secure state for
v8-M.main, we use the VLLDM and VLSTM instructions to save, clear and
restore the FP registers around the call. These instructions both check
the CONTROL_S.SFPA bit, and if it is clear (meaning the current contents
of the FP registers are not secret) they execute as no-ops.

This causes a problem when CONTROL_S.SFPA==0 before the call, which
happens if there are no floating-point instructions executed between
entry to secure state and the call. If this is the case, then the VLSTM
instruction will do nothing, leaving the save area in the stack
uninitialised. If the called function returns a value in floating-point
registers, the call sequence includes an instruction to copy the return
value from a floating-point register to a GPR, which must be before the
VLLDM instruction. This copy sets CONTROL_S.SFPA, meaning that the VLLDM
will fully execute, and load the uninitialised stack memory into the FP
registers.

This causes two problems:

The FP register file is clobbered, including all of the callee-saved
registers, which might contain live values.
The stack region might contain secret values, which will be leaked to
non-secure state through the floating-point registers if/when we
return to non-secure state.

The fix is to insert a vmov s0, s0 instruction before the VLSTM
instruction, to ensure that CONTROL_S.SFPA is set for both the VLLDM and
VLSTM instruction.

CVE: https://www.cve.org/cverecord?id=CVE-2024-7883
Security bulletin: https://developer.arm.com/Arm%20Security%20Center/Cortex-M%20Security%20Extensions%20Vulnerability

When doing a call from CMSE secure state to non-secure state for v8-M.main, we use the VLLDM and VLSTM instructions to save, clear and restore the FP registers around the call. These instructions both check the CONTROL_S.SFPA bit, and if it is clear (meaning the current contents of the FP registers are not secret) they execute as no-ops. This causes a problem when CONTROL_S.SFPA==0 before the call, which happens if there are no floating-point instructions executed between entry to secure state and the call. If this is the case, then the VLSTM instruction will do nothing, levaing the save area in the stack uninitialised. If the called function returns a value in floating-point registers, the call sequence includes an instruction to copy the return value from a floating-point register to a GPR, which must be before the VLLDM instruction. This copy sets CONTROL_S.SFPA, meaning that the VLLDM will fully execute, and load the uninitialised stack memory into the FP registers. This causes two problems: * The FP register file is clobbered, including all of the callee-saved registers, which might contain live values. * The stack region might contain secret values, which will be leaked to non-secure state through the floating-point registers if/when we return to non-secure state. The fix is to insert a `vmov s0, s0` instruction before the VLSTM instruction, to ensure that CONTROL_S.SFPA is set for both the VLLDM and VLSTM instruction. CVE: https://www.cve.org/cverecord?id=CVE-2024-7883 Security bulletin: https://developer.arm.com/Arm%20Security%20Center/Cortex-M%20Security%20Extensions%20Vulnerability

llvmbot · 2024-10-31T17:32:24Z

@llvm/pr-subscribers-backend-arm

Author: Oliver Stannard (ostannard)

Changes

When doing a call from CMSE secure state to non-secure state for
v8-M.main, we use the VLLDM and VLSTM instructions to save, clear and
restore the FP registers around the call. These instructions both check
the CONTROL_S.SFPA bit, and if it is clear (meaning the current contents
of the FP registers are not secret) they execute as no-ops.

This causes a problem when CONTROL_S.SFPA==0 before the call, which
happens if there are no floating-point instructions executed between
entry to secure state and the call. If this is the case, then the VLSTM
instruction will do nothing, leaving the save area in the stack
uninitialised. If the called function returns a value in floating-point
registers, the call sequence includes an instruction to copy the return
value from a floating-point register to a GPR, which must be before the
VLLDM instruction. This copy sets CONTROL_S.SFPA, meaning that the VLLDM
will fully execute, and load the uninitialised stack memory into the FP
registers.

This causes two problems:

The FP register file is clobbered, including all of the callee-saved
registers, which might contain live values.
The stack region might contain secret values, which will be leaked to
non-secure state through the floating-point registers if/when we
return to non-secure state.

The fix is to insert a vmov s0, s0 instruction before the VLSTM
instruction, to ensure that CONTROL_S.SFPA is set for both the VLLDM and
VLSTM instruction.

CVE: https://www.cve.org/cverecord?id=CVE-2024-7883
Security bulletin: https://developer.arm.com/Arm%20Security%20Center/Cortex-M%20Security%20Extensions%20Vulnerability

Full diff: https://github.com/llvm/llvm-project/pull/114433.diff

3 Files Affected:

(modified) llvm/lib/Target/ARM/ARMExpandPseudoInsts.cpp (+41-3)
(modified) llvm/test/CodeGen/ARM/cmse-clear-float-hard.ll (+33-30)
(modified) llvm/test/CodeGen/ARM/cmse-vlldm-no-reorder.mir (+1)

diff --git a/llvm/lib/Target/ARM/ARMExpandPseudoInsts.cpp b/llvm/lib/Target/ARM/ARMExpandPseudoInsts.cpp
index 5be9d73022a6ee..8e79a0a344067f 100644
--- a/llvm/lib/Target/ARM/ARMExpandPseudoInsts.cpp
+++ b/llvm/lib/Target/ARM/ARMExpandPseudoInsts.cpp
@@ -1426,6 +1426,7 @@ void ARMExpandPseudo::CMSESaveClearFPRegsV8(
   // Use ScratchRegs to store the fp regs
   std::vector<std::tuple<unsigned, unsigned, unsigned>> ClearedFPRegs;
   std::vector<unsigned> NonclearedFPRegs;
+  bool ReturnsFPReg = false;
   for (const MachineOperand &Op : MBBI->operands()) {
     if (Op.isReg() && Op.isUse()) {
       Register Reg = Op.getReg();
@@ -1460,14 +1461,51 @@ void ARMExpandPseudo::CMSESaveClearFPRegsV8(
           NonclearedFPRegs.push_back(Reg);
         }
       }
+    } else if (Op.isReg() && Op.isDef()) {
+      Register Reg = Op.getReg();
+      if (ARM::SPRRegClass.contains(Reg) || ARM::DPRRegClass.contains(Reg) ||
+          ARM::QPRRegClass.contains(Reg))
+        ReturnsFPReg = true;
     }
   }
 
-  bool passesFPReg = (!NonclearedFPRegs.empty() || !ClearedFPRegs.empty());
+  bool PassesFPReg = (!NonclearedFPRegs.empty() || !ClearedFPRegs.empty());
 
-  if (passesFPReg)
+  if (PassesFPReg || ReturnsFPReg)
     assert(STI->hasFPRegs() && "Subtarget needs fpregs");
 
+  // CVE-2024-7883
+  //
+  // The VLLDM/VLSTM instructions set up lazy state preservation, but they
+  // execute as NOPs if the FP register file is not considered to contain
+  // secure data, represented by the CONTROL_S.SFPA bit. This means that the
+  // state of CONTROL_S.SFPA must be the same when these two instructions are
+  // executed. That might not be the case if we haven't used any FP
+  // instructions before the VLSTM, so CONTROL_S.SFPA is clear, but do have one
+  // before the VLLDM, which sets it..
+  //
+  // If we can't prove that SFPA will be the same for the VLSTM and VLLDM, we
+  // execute a "vmov s0, s0" instruction before the VLSTM to ensure that
+  // CONTROL_S.SFPA is set for both.
+  //
+  // That can only happen for callees which take no FP arguments (or we'd have
+  // inserted a VMOV above) and which return values in FP regs (so that we need
+  // to use a VMOV to back-up the return value before the VLLDM). It also can't
+  // happen if the call is dominated by other existing floating-point
+  // instructions, but we don't currently check for that case.
+  //
+  // These conditions mean that we only emit this instruction when using the
+  // hard-float ABI, which means we can assume that FP instructions are
+  // available, and don't need to make it conditional like we do for the
+  // CVE-2021-35465 workaround.
+  if (ReturnsFPReg && !PassesFPReg) {
+    bool S0Dead = !LiveRegs.contains(ARM::S0);
+    BuildMI(MBB, MBBI, DL, TII->get(ARM::VMOVS))
+        .addReg(ARM::S0, RegState::Define | getDeadRegState(S0Dead))
+        .addReg(ARM::S0, getUndefRegState(S0Dead))
+        .add(predOps(ARMCC::AL));
+  }
+
   // Lazy store all fp registers to the stack.
   // This executes as NOP in the absence of floating-point support.
   MachineInstrBuilder VLSTM =
@@ -1528,7 +1566,7 @@ void ARMExpandPseudo::CMSESaveClearFPRegsV8(
   }
   // restore FPSCR from stack and clear bits 0-4, 7, 28-31
   // The other bits are program global according to the AAPCS
-  if (passesFPReg) {
+  if (PassesFPReg) {
     BuildMI(MBB, MBBI, DL, TII->get(ARM::tLDRspi), SpareReg)
         .addReg(ARM::SP)
         .addImm(0x10)
diff --git a/llvm/test/CodeGen/ARM/cmse-clear-float-hard.ll b/llvm/test/CodeGen/ARM/cmse-clear-float-hard.ll
index 606859db0a0ea8..94cf38804dae8d 100644
--- a/llvm/test/CodeGen/ARM/cmse-clear-float-hard.ll
+++ b/llvm/test/CodeGen/ARM/cmse-clear-float-hard.ll
@@ -187,7 +187,7 @@ define float @f2(ptr nocapture %fptr) #2 {
 ; CHECK-8M-NEXT:    bic r0, r0, #1
 ; CHECK-8M-NEXT:    sub sp, #136
 ; CHECK-8M-NEXT:    vmov r12, s0
-; CHECK-8M-NEXT:    vlstm sp
+; CHECK-8M-NEXT:    vlstm sp, {d0 - d15}
 ; CHECK-8M-NEXT:    vmov s0, r12
 ; CHECK-8M-NEXT:    ldr r1, [sp, #64]
 ; CHECK-8M-NEXT:    bic r1, r1, #159
@@ -207,7 +207,7 @@ define float @f2(ptr nocapture %fptr) #2 {
 ; CHECK-8M-NEXT:    msr apsr_nzcvqg, r0
 ; CHECK-8M-NEXT:    blxns r0
 ; CHECK-8M-NEXT:    vmov r12, s0
-; CHECK-8M-NEXT:    vlldm sp
+; CHECK-8M-NEXT:    vlldm sp, {d0 - d15}
 ; CHECK-8M-NEXT:    vmov s0, r12
 ; CHECK-8M-NEXT:    add sp, #136
 ; CHECK-8M-NEXT:    pop.w {r4, r5, r6, r7, r8, r9, r10, r11}
@@ -245,7 +245,7 @@ define double @d2(ptr nocapture %fptr) #2 {
 ; CHECK-8M-LE-NEXT:    bic r0, r0, #1
 ; CHECK-8M-LE-NEXT:    sub sp, #136
 ; CHECK-8M-LE-NEXT:    vmov r11, r12, d0
-; CHECK-8M-LE-NEXT:    vlstm sp
+; CHECK-8M-LE-NEXT:    vlstm sp, {d0 - d15}
 ; CHECK-8M-LE-NEXT:    vmov d0, r11, r12
 ; CHECK-8M-LE-NEXT:    ldr r1, [sp, #64]
 ; CHECK-8M-LE-NEXT:    bic r1, r1, #159
@@ -264,7 +264,7 @@ define double @d2(ptr nocapture %fptr) #2 {
 ; CHECK-8M-LE-NEXT:    msr apsr_nzcvqg, r0
 ; CHECK-8M-LE-NEXT:    blxns r0
 ; CHECK-8M-LE-NEXT:    vmov r11, r12, d0
-; CHECK-8M-LE-NEXT:    vlldm sp
+; CHECK-8M-LE-NEXT:    vlldm sp, {d0 - d15}
 ; CHECK-8M-LE-NEXT:    vmov d0, r11, r12
 ; CHECK-8M-LE-NEXT:    add sp, #136
 ; CHECK-8M-LE-NEXT:    pop.w {r4, r5, r6, r7, r8, r9, r10, r11}
@@ -283,7 +283,7 @@ define double @d2(ptr nocapture %fptr) #2 {
 ; CHECK-8M-BE-NEXT:    bic r0, r0, #1
 ; CHECK-8M-BE-NEXT:    sub sp, #136
 ; CHECK-8M-BE-NEXT:    vmov r11, r12, d0
-; CHECK-8M-BE-NEXT:    vlstm sp
+; CHECK-8M-BE-NEXT:    vlstm sp, {d0 - d15}
 ; CHECK-8M-BE-NEXT:    vmov d0, r11, r12
 ; CHECK-8M-BE-NEXT:    ldr r1, [sp, #64]
 ; CHECK-8M-BE-NEXT:    bic r1, r1, #159
@@ -302,7 +302,7 @@ define double @d2(ptr nocapture %fptr) #2 {
 ; CHECK-8M-BE-NEXT:    msr apsr_nzcvqg, r0
 ; CHECK-8M-BE-NEXT:    blxns r0
 ; CHECK-8M-BE-NEXT:    vmov r11, r12, d0
-; CHECK-8M-BE-NEXT:    vlldm sp
+; CHECK-8M-BE-NEXT:    vlldm sp, {d0 - d15}
 ; CHECK-8M-BE-NEXT:    vmov d0, r11, r12
 ; CHECK-8M-BE-NEXT:    add sp, #136
 ; CHECK-8M-BE-NEXT:    pop.w {r4, r5, r6, r7, r8, r9, r10, r11}
@@ -368,7 +368,7 @@ define float @f3(ptr nocapture %fptr) #4 {
 ; CHECK-8M-NEXT:    bic r0, r0, #1
 ; CHECK-8M-NEXT:    sub sp, #136
 ; CHECK-8M-NEXT:    vmov r12, s0
-; CHECK-8M-NEXT:    vlstm sp
+; CHECK-8M-NEXT:    vlstm sp, {d0 - d15}
 ; CHECK-8M-NEXT:    vmov s0, r12
 ; CHECK-8M-NEXT:    ldr r1, [sp, #64]
 ; CHECK-8M-NEXT:    bic r1, r1, #159
@@ -388,7 +388,7 @@ define float @f3(ptr nocapture %fptr) #4 {
 ; CHECK-8M-NEXT:    msr apsr_nzcvqg, r0
 ; CHECK-8M-NEXT:    blxns r0
 ; CHECK-8M-NEXT:    vmov r12, s0
-; CHECK-8M-NEXT:    vlldm sp
+; CHECK-8M-NEXT:    vlldm sp, {d0 - d15}
 ; CHECK-8M-NEXT:    vmov s0, r12
 ; CHECK-8M-NEXT:    add sp, #136
 ; CHECK-8M-NEXT:    pop.w {r4, r5, r6, r7, r8, r9, r10, r11}
@@ -426,7 +426,7 @@ define double @d3(ptr nocapture %fptr) #4 {
 ; CHECK-8M-LE-NEXT:    bic r0, r0, #1
 ; CHECK-8M-LE-NEXT:    sub sp, #136
 ; CHECK-8M-LE-NEXT:    vmov r11, r12, d0
-; CHECK-8M-LE-NEXT:    vlstm sp
+; CHECK-8M-LE-NEXT:    vlstm sp, {d0 - d15}
 ; CHECK-8M-LE-NEXT:    vmov d0, r11, r12
 ; CHECK-8M-LE-NEXT:    ldr r1, [sp, #64]
 ; CHECK-8M-LE-NEXT:    bic r1, r1, #159
@@ -445,7 +445,7 @@ define double @d3(ptr nocapture %fptr) #4 {
 ; CHECK-8M-LE-NEXT:    msr apsr_nzcvqg, r0
 ; CHECK-8M-LE-NEXT:    blxns r0
 ; CHECK-8M-LE-NEXT:    vmov r11, r12, d0
-; CHECK-8M-LE-NEXT:    vlldm sp
+; CHECK-8M-LE-NEXT:    vlldm sp, {d0 - d15}
 ; CHECK-8M-LE-NEXT:    vmov d0, r11, r12
 ; CHECK-8M-LE-NEXT:    add sp, #136
 ; CHECK-8M-LE-NEXT:    pop.w {r4, r5, r6, r7, r8, r9, r10, r11}
@@ -464,7 +464,7 @@ define double @d3(ptr nocapture %fptr) #4 {
 ; CHECK-8M-BE-NEXT:    bic r0, r0, #1
 ; CHECK-8M-BE-NEXT:    sub sp, #136
 ; CHECK-8M-BE-NEXT:    vmov r11, r12, d0
-; CHECK-8M-BE-NEXT:    vlstm sp
+; CHECK-8M-BE-NEXT:    vlstm sp, {d0 - d15}
 ; CHECK-8M-BE-NEXT:    vmov d0, r11, r12
 ; CHECK-8M-BE-NEXT:    ldr r1, [sp, #64]
 ; CHECK-8M-BE-NEXT:    bic r1, r1, #159
@@ -483,7 +483,7 @@ define double @d3(ptr nocapture %fptr) #4 {
 ; CHECK-8M-BE-NEXT:    msr apsr_nzcvqg, r0
 ; CHECK-8M-BE-NEXT:    blxns r0
 ; CHECK-8M-BE-NEXT:    vmov r11, r12, d0
-; CHECK-8M-BE-NEXT:    vlldm sp
+; CHECK-8M-BE-NEXT:    vlldm sp, {d0 - d15}
 ; CHECK-8M-BE-NEXT:    vmov d0, r11, r12
 ; CHECK-8M-BE-NEXT:    add sp, #136
 ; CHECK-8M-BE-NEXT:    pop.w {r4, r5, r6, r7, r8, r9, r10, r11}
@@ -547,8 +547,9 @@ define float @f4(ptr nocapture %fptr) #6 {
 ; CHECK-8M-NEXT:    push.w {r4, r5, r6, r7, r8, r9, r10, r11}
 ; CHECK-8M-NEXT:    bic r0, r0, #1
 ; CHECK-8M-NEXT:    sub sp, #136
-; CHECK-8M-NEXT:    vlstm sp
+; CHECK-8M-NEXT:    vmov.f32 s0, s0
 ; CHECK-8M-NEXT:    mov r1, r0
+; CHECK-8M-NEXT:    vlstm sp, {d0 - d15}
 ; CHECK-8M-NEXT:    mov r2, r0
 ; CHECK-8M-NEXT:    mov r3, r0
 ; CHECK-8M-NEXT:    mov r4, r0
@@ -563,7 +564,7 @@ define float @f4(ptr nocapture %fptr) #6 {
 ; CHECK-8M-NEXT:    msr apsr_nzcvqg, r0
 ; CHECK-8M-NEXT:    blxns r0
 ; CHECK-8M-NEXT:    vmov r12, s0
-; CHECK-8M-NEXT:    vlldm sp
+; CHECK-8M-NEXT:    vlldm sp, {d0 - d15}
 ; CHECK-8M-NEXT:    vmov s0, r12
 ; CHECK-8M-NEXT:    add sp, #136
 ; CHECK-8M-NEXT:    pop.w {r4, r5, r6, r7, r8, r9, r10, r11}
@@ -598,8 +599,9 @@ define double @d4(ptr nocapture %fptr) #6 {
 ; CHECK-8M-NEXT:    push.w {r4, r5, r6, r7, r8, r9, r10, r11}
 ; CHECK-8M-NEXT:    bic r0, r0, #1
 ; CHECK-8M-NEXT:    sub sp, #136
-; CHECK-8M-NEXT:    vlstm sp
+; CHECK-8M-NEXT:    vmov.f32 s0, s0
 ; CHECK-8M-NEXT:    mov r1, r0
+; CHECK-8M-NEXT:    vlstm sp, {d0 - d15}
 ; CHECK-8M-NEXT:    mov r2, r0
 ; CHECK-8M-NEXT:    mov r3, r0
 ; CHECK-8M-NEXT:    mov r4, r0
@@ -614,7 +616,7 @@ define double @d4(ptr nocapture %fptr) #6 {
 ; CHECK-8M-NEXT:    msr apsr_nzcvqg, r0
 ; CHECK-8M-NEXT:    blxns r0
 ; CHECK-8M-NEXT:    vmov r11, r12, d0
-; CHECK-8M-NEXT:    vlldm sp
+; CHECK-8M-NEXT:    vlldm sp, {d0 - d15}
 ; CHECK-8M-NEXT:    vmov d0, r11, r12
 ; CHECK-8M-NEXT:    add sp, #136
 ; CHECK-8M-NEXT:    pop.w {r4, r5, r6, r7, r8, r9, r10, r11}
@@ -649,7 +651,7 @@ define void @fd(ptr %f, float %a, double %b) #8 {
 ; CHECK-8M-NEXT:    vmov r12, s0
 ; CHECK-8M-NEXT:    mov r2, r0
 ; CHECK-8M-NEXT:    vmov r10, r11, d1
-; CHECK-8M-NEXT:    vlstm sp
+; CHECK-8M-NEXT:    vlstm sp, {d0 - d15}
 ; CHECK-8M-NEXT:    vmov s0, r12
 ; CHECK-8M-NEXT:    vmov d1, r10, r11
 ; CHECK-8M-NEXT:    ldr r1, [sp, #64]
@@ -666,7 +668,7 @@ define void @fd(ptr %f, float %a, double %b) #8 {
 ; CHECK-8M-NEXT:    mov r9, r0
 ; CHECK-8M-NEXT:    msr apsr_nzcvqg, r0
 ; CHECK-8M-NEXT:    blxns r0
-; CHECK-8M-NEXT:    vlldm sp
+; CHECK-8M-NEXT:    vlldm sp, {d0 - d15}
 ; CHECK-8M-NEXT:    add sp, #136
 ; CHECK-8M-NEXT:    pop.w {r4, r5, r6, r7, r8, r9, r10, r11}
 ; CHECK-8M-NEXT:    pop {r7, pc}
@@ -708,7 +710,7 @@ define void @fdff(ptr %f, float %a, double %b, float %c, float %d) #8 {
 ; CHECK-8M-NEXT:    vmov r9, s1
 ; CHECK-8M-NEXT:    mov r4, r0
 ; CHECK-8M-NEXT:    vmov r8, s4
-; CHECK-8M-NEXT:    vlstm sp
+; CHECK-8M-NEXT:    vlstm sp, {d0 - d15}
 ; CHECK-8M-NEXT:    vmov s0, r12
 ; CHECK-8M-NEXT:    vmov d1, r10, r11
 ; CHECK-8M-NEXT:    vmov s1, r9
@@ -723,7 +725,7 @@ define void @fdff(ptr %f, float %a, double %b, float %c, float %d) #8 {
 ; CHECK-8M-NEXT:    mov r7, r0
 ; CHECK-8M-NEXT:    msr apsr_nzcvqg, r0
 ; CHECK-8M-NEXT:    blxns r0
-; CHECK-8M-NEXT:    vlldm sp
+; CHECK-8M-NEXT:    vlldm sp, {d0 - d15}
 ; CHECK-8M-NEXT:    add sp, #136
 ; CHECK-8M-NEXT:    pop.w {r4, r5, r6, r7, r8, r9, r10, r11}
 ; CHECK-8M-NEXT:    pop {r7, pc}
@@ -765,7 +767,7 @@ define void @fidififid(ptr %fu, float %a, i32 %b, double %c, i32 %d, float %e, i
 ; CHECK-8M-NEXT:    vmov r8, s1
 ; CHECK-8M-NEXT:    vmov r7, s4
 ; CHECK-8M-NEXT:    vmov r5, r6, d3
-; CHECK-8M-NEXT:    vlstm sp
+; CHECK-8M-NEXT:    vlstm sp, {d0 - d15}
 ; CHECK-8M-NEXT:    vmov s0, r11
 ; CHECK-8M-NEXT:    vmov d1, r9, r10
 ; CHECK-8M-NEXT:    vmov s1, r8
@@ -778,7 +780,7 @@ define void @fidififid(ptr %fu, float %a, i32 %b, double %c, i32 %d, float %e, i
 ; CHECK-8M-NEXT:    mov r4, r12
 ; CHECK-8M-NEXT:    msr apsr_nzcvqg, r12
 ; CHECK-8M-NEXT:    blxns r12
-; CHECK-8M-NEXT:    vlldm sp
+; CHECK-8M-NEXT:    vlldm sp, {d0 - d15}
 ; CHECK-8M-NEXT:    add sp, #136
 ; CHECK-8M-NEXT:    pop.w {r4, r5, r6, r7, r8, r9, r10, r11}
 ; CHECK-8M-NEXT:    pop {r7, pc}
@@ -897,7 +899,7 @@ define half @h2(ptr nocapture %hptr) nounwind {
 ; CHECK-8M-NEXT:    bic r0, r0, #1
 ; CHECK-8M-NEXT:    sub sp, #136
 ; CHECK-8M-NEXT:    vmov r12, s0
-; CHECK-8M-NEXT:    vlstm sp
+; CHECK-8M-NEXT:    vlstm sp, {d0 - d15}
 ; CHECK-8M-NEXT:    vmov s0, r12
 ; CHECK-8M-NEXT:    ldr r1, [sp, #64]
 ; CHECK-8M-NEXT:    bic r1, r1, #159
@@ -917,7 +919,7 @@ define half @h2(ptr nocapture %hptr) nounwind {
 ; CHECK-8M-NEXT:    msr apsr_nzcvqg, r0
 ; CHECK-8M-NEXT:    blxns r0
 ; CHECK-8M-NEXT:    vmov r12, s0
-; CHECK-8M-NEXT:    vlldm sp
+; CHECK-8M-NEXT:    vlldm sp, {d0 - d15}
 ; CHECK-8M-NEXT:    vmov s0, r12
 ; CHECK-8M-NEXT:    add sp, #136
 ; CHECK-8M-NEXT:    pop.w {r4, r5, r6, r7, r8, r9, r10, r11}
@@ -976,7 +978,7 @@ define half @h3(ptr nocapture %hptr) nounwind {
 ; CHECK-8M-NEXT:    bic r0, r0, #1
 ; CHECK-8M-NEXT:    sub sp, #136
 ; CHECK-8M-NEXT:    vmov r12, s0
-; CHECK-8M-NEXT:    vlstm sp
+; CHECK-8M-NEXT:    vlstm sp, {d0 - d15}
 ; CHECK-8M-NEXT:    vmov s0, r12
 ; CHECK-8M-NEXT:    ldr r1, [sp, #64]
 ; CHECK-8M-NEXT:    bic r1, r1, #159
@@ -996,7 +998,7 @@ define half @h3(ptr nocapture %hptr) nounwind {
 ; CHECK-8M-NEXT:    msr apsr_nzcvqg, r0
 ; CHECK-8M-NEXT:    blxns r0
 ; CHECK-8M-NEXT:    vmov r12, s0
-; CHECK-8M-NEXT:    vlldm sp
+; CHECK-8M-NEXT:    vlldm sp, {d0 - d15}
 ; CHECK-8M-NEXT:    vmov s0, r12
 ; CHECK-8M-NEXT:    add sp, #136
 ; CHECK-8M-NEXT:    pop.w {r4, r5, r6, r7, r8, r9, r10, r11}
@@ -1053,8 +1055,9 @@ define half @h4(ptr nocapture %hptr) nounwind {
 ; CHECK-8M-NEXT:    push.w {r4, r5, r6, r7, r8, r9, r10, r11}
 ; CHECK-8M-NEXT:    bic r0, r0, #1
 ; CHECK-8M-NEXT:    sub sp, #136
-; CHECK-8M-NEXT:    vlstm sp
+; CHECK-8M-NEXT:    vmov.f32 s0, s0
 ; CHECK-8M-NEXT:    mov r1, r0
+; CHECK-8M-NEXT:    vlstm sp, {d0 - d15}
 ; CHECK-8M-NEXT:    mov r2, r0
 ; CHECK-8M-NEXT:    mov r3, r0
 ; CHECK-8M-NEXT:    mov r4, r0
@@ -1069,7 +1072,7 @@ define half @h4(ptr nocapture %hptr) nounwind {
 ; CHECK-8M-NEXT:    msr apsr_nzcvqg, r0
 ; CHECK-8M-NEXT:    blxns r0
 ; CHECK-8M-NEXT:    vmov r12, s0
-; CHECK-8M-NEXT:    vlldm sp
+; CHECK-8M-NEXT:    vlldm sp, {d0 - d15}
 ; CHECK-8M-NEXT:    vmov s0, r12
 ; CHECK-8M-NEXT:    add sp, #136
 ; CHECK-8M-NEXT:    pop.w {r4, r5, r6, r7, r8, r9, r10, r11}
@@ -1176,7 +1179,7 @@ define half @h1_arg(ptr nocapture %hptr, half %harg) nounwind {
 ; CHECK-8M-NEXT:    bic r0, r0, #1
 ; CHECK-8M-NEXT:    sub sp, #136
 ; CHECK-8M-NEXT:    vmov r12, s0
-; CHECK-8M-NEXT:    vlstm sp
+; CHECK-8M-NEXT:    vlstm sp, {d0 - d15}
 ; CHECK-8M-NEXT:    vmov s0, r12
 ; CHECK-8M-NEXT:    ldr r1, [sp, #64]
 ; CHECK-8M-NEXT:    bic r1, r1, #159
@@ -1196,7 +1199,7 @@ define half @h1_arg(ptr nocapture %hptr, half %harg) nounwind {
 ; CHECK-8M-NEXT:    msr apsr_nzcvqg, r0
 ; CHECK-8M-NEXT:    blxns r0
 ; CHECK-8M-NEXT:    vmov r12, s0
-; CHECK-8M-NEXT:    vlldm sp
+; CHECK-8M-NEXT:    vlldm sp, {d0 - d15}
 ; CHECK-8M-NEXT:    vmov s0, r12
 ; CHECK-8M-NEXT:    add sp, #136
 ; CHECK-8M-NEXT:    pop.w {r4, r5, r6, r7, r8, r9, r10, r11}
diff --git a/llvm/test/CodeGen/ARM/cmse-vlldm-no-reorder.mir b/llvm/test/CodeGen/ARM/cmse-vlldm-no-reorder.mir
index 3d49fee8fdaf43..29429fd5a23eb1 100644
--- a/llvm/test/CodeGen/ARM/cmse-vlldm-no-reorder.mir
+++ b/llvm/test/CodeGen/ARM/cmse-vlldm-no-reorder.mir
@@ -89,6 +89,7 @@ body:             |
 # CHECK: $sp = t2STMDB_UPD $sp, 14 /* CC::al */, $noreg, $r4, $r5, $r6, undef $r7, $r8, $r9, $r10, $r11
 # CHECK-NEXT:  $r0 = t2BICri $r0, 1, 14 /* CC::al */, $noreg, $noreg
 # CHECK-NEXT:  $sp = tSUBspi $sp, 34, 14 /* CC::al */, $noreg
+# CHECK-NEXT:  dead $s0 = VMOVS undef $s0, 14 /* CC::al */, $noreg
 # CHECK-NEXT:  VLSTM $sp, 14 /* CC::al */, $noreg, 0, implicit-def $vpr, implicit-def $fpscr, implicit-def $fpscr_nzcv, implicit undef $vpr, implicit undef $fpscr, implicit undef $fpscr_nzcv, implicit undef $d0, implicit undef $d1, implicit undef $d2, implicit undef $d3, implicit undef $d4, implicit undef $d5, implicit undef $d6, implicit undef $d7, implicit $d8, implicit $d9, implicit $d10, implicit $d11, implicit $d12, implicit $d13, implicit $d14, implicit $d15
 # CHECK-NEXT:  $r1 = tMOVr $r0, 14 /* CC::al */, $noreg
 # CHECK-NEXT:  $r2 = tMOVr $r0, 14 /* CC::al */, $noreg

efriedma-quic

The logic here seems like it's very dependent on the assumption that nothing will try to optimize the expanded instructions after pseudo-expansion. Which is... probably okay, I guess. Probably we'd notice if that sort of optimization started happening.

It might be worth adding a testcase where an function argument is undef/poison, to make sure we don't optimize out the "no-op" there?

Otherwise LGTM.

ostannard · 2024-11-01T09:33:12Z

The pre-commit failures are an unrelated problem on main, they went away locally after merging again.

It might be worth adding a testcase where an function argument is undef/poison, to make sure we don't optimize out the "no-op" there?

The extra vmov instruction is only emitted when there are no floating-point arguments, so we don't need to worry about s0 being poison/undef, but I'll add some tests with poison and undef integer arguments to be sure.

…vm#114433) When doing a call from CMSE secure state to non-secure state for v8-M.main, we use the VLLDM and VLSTM instructions to save, clear and restore the FP registers around the call. These instructions both check the CONTROL_S.SFPA bit, and if it is clear (meaning the current contents of the FP registers are not secret) they execute as no-ops. This causes a problem when CONTROL_S.SFPA==0 before the call, which happens if there are no floating-point instructions executed between entry to secure state and the call. If this is the case, then the VLSTM instruction will do nothing, leaving the save area in the stack uninitialised. If the called function returns a value in floating-point registers, the call sequence includes an instruction to copy the return value from a floating-point register to a GPR, which must be before the VLLDM instruction. This copy sets CONTROL_S.SFPA, meaning that the VLLDM will fully execute, and load the uninitialised stack memory into the FP registers. This causes two problems: * The FP register file is clobbered, including all of the callee-saved registers, which might contain live values. * The stack region might contain secret values, which will be leaked to non-secure state through the floating-point registers if/when we return to non-secure state. The fix is to insert a `vmov s0, s0` instruction before the VLSTM instruction, to ensure that CONTROL_S.SFPA is set for both the VLLDM and VLSTM instruction. CVE: https://www.cve.org/cverecord?id=CVE-2024-7883 Security bulletin: https://developer.arm.com/Arm%20Security%20Center/Cortex-M%20Security%20Extensions%20Vulnerability

efriedma-quic · 2024-11-05T21:45:18Z

I was thinking of a case where you had a undef floating-point argument, we skip this code, but there isn't any code to actually set the FP argument.

ostannard · 2024-11-06T09:31:26Z

Ah, I see. In that case we still emit the code to save/restore the floating-point registers around the vlstm at the start of the call sequence, even if they contain poison or undef values, so the extra vmov still isn't needed. I've added those tests as ed9dab6.

ostannard · 2024-11-06T10:39:34Z

That test failed on an expensive-checks buildbot, so I've reverted it for now: https://lab.llvm.org/buildbot/#/builders/16/builds/8346

The error message points to the argument saving instructions at the start of the call sequence, not the new vmov added by this change, so it's probably a pre-existing bug.

ostannard added 2 commits October 31, 2024 17:23

Re-generate test for changed assembly syntax

9b6e64c

ostannard added the backend:ARM label Oct 31, 2024

ostannard requested review from momchil-velikov, kbeyls and efriedma-quic October 31, 2024 17:32

efriedma-quic approved these changes Oct 31, 2024

View reviewed changes

ostannard added 2 commits November 1, 2024 09:22

Merge remote-tracking branch 'origin/main' into cmse-sfpa

573ca88

Add tests with undef/poison arguments

54150ed

ostannard merged commit 33411d5 into llvm:main Nov 1, 2024
4 of 6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[ARM] Fix CMSE S->NS calls when CONTROL_S.SFPA==0 (CVE-2024-7883) #114433

[ARM] Fix CMSE S->NS calls when CONTROL_S.SFPA==0 (CVE-2024-7883) #114433

Uh oh!

ostannard commented Oct 31, 2024

Uh oh!

llvmbot commented Oct 31, 2024

Uh oh!

efriedma-quic left a comment

Uh oh!

ostannard commented Nov 1, 2024

Uh oh!

Uh oh!

efriedma-quic commented Nov 5, 2024

Uh oh!

ostannard commented Nov 6, 2024

Uh oh!

ostannard commented Nov 6, 2024

Uh oh!

Uh oh!

[ARM] Fix CMSE S->NS calls when CONTROL_S.SFPA==0 (CVE-2024-7883) #114433

[ARM] Fix CMSE S->NS calls when CONTROL_S.SFPA==0 (CVE-2024-7883) #114433

Uh oh!

Conversation

ostannard commented Oct 31, 2024

Uh oh!

llvmbot commented Oct 31, 2024

Uh oh!

efriedma-quic left a comment

Choose a reason for hiding this comment

Uh oh!

ostannard commented Nov 1, 2024

Uh oh!

Uh oh!

efriedma-quic commented Nov 5, 2024

Uh oh!

ostannard commented Nov 6, 2024

Uh oh!

ostannard commented Nov 6, 2024

Uh oh!

Uh oh!