CVE-2024-31852: Insufficient Information (NVD-CWE-noinfo)
value: LLVM before 18.1.3 generates code in which the LR register can be overwritten without data being saved to the stack, and thus there can sometimes be an exploitable error in the flow of control. This affects the ARM backend and can be demonstrated with Clang. NOTE: the vendor perspective is "we don't have strong objections for a CVE to be created ... It does seem that the likelihood of this miscompile enabling an exploit remains very low, because the miscompile resulting in this JOP gadget is such that the function is most likely to crash on most valid inputs to the function. So, if this function is covered by any testing, the miscompile is most likely to be discovered before the binary is shipped to production."
CWE description: There is insufficient information about the issue to classify it; details are unkown or unspecified.
Published: 2024-04-05T15:15Z
Repository: https://github.com/llvmbot/llvm-project
Language: C++
Files changed: llvm/lib/Target/ARM/ARMFrameLowering.cpp, llvm/lib/Target/ARM/ARMFrameLowering.h, llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp, llvm/test/CodeGen/ARM/ldst-opt-lr-restored.ll
Commit date: 2024-03-23 15:34:54 -0700
Commit message: [ARM] Update IsRestored for LR based on all returns (#82745)

PR #75527 fixed ARMFrameLowering to set the IsRestored flag for LR based
on all of the return instructions in the function, not just one.
However, there is also code in ARMLoadStoreOptimizer which changes
return instructions, but it set IsRestored based on the one instruction
it changed, not the whole function.

The fix is to factor out the code added in #75527, and also call it from
ARMLoadStoreOptimizer if it made a change to return instructions.

Fixes #80287.

(cherry picked from commit 749384c08e042739342c88b521c8ba5dac1b9276)

Diff: diff --git a/llvm/lib/Target/ARM/ARMFrameLowering.cpp b/llvm/lib/Target/ARM/ARMFrameLowering.cpp
index eeb7f64aa..9b54dd4e4 100644
--- a/llvm/lib/Target/ARM/ARMFrameLowering.cpp
+++ b/llvm/lib/Target/ARM/ARMFrameLowering.cpp
@@ -2779,14 +2779,11 @@ void ARMFrameLowering::determineCalleeSaves(MachineFunction &MF,
   if (ForceLRSpill)
     SavedRegs.set(ARM::LR);
   AFI->setLRIsSpilled(SavedRegs.test(ARM::LR));
 }
 
-void ARMFrameLowering::processFunctionBeforeFrameFinalized(
-    MachineFunction &MF, RegScavenger *RS) const {
-  TargetFrameLowering::processFunctionBeforeFrameFinalized(MF, RS);
-
+void ARMFrameLowering::updateLRRestored(MachineFunction &MF) {
   MachineFrameInfo &MFI = MF.getFrameInfo();
   if (!MFI.isCalleeSavedInfoValid())
     return;
 
   // Check if all terminators do not implicitly use LR. Then we can 'restore' LR
@@ -2806,10 +2803,16 @@ void ARMFrameLowering::processFunctionBeforeFrameFinalized(
       break;
     }
   }
 }
 
+void ARMFrameLowering::processFunctionBeforeFrameFinalized(
+    MachineFunction &MF, RegScavenger *RS) const {
+  TargetFrameLowering::processFunctionBeforeFrameFinalized(MF, RS);
+  updateLRRestored(MF);
+}
+
 void ARMFrameLowering::getCalleeSaves(const MachineFunction &MF,
                                       BitVector &SavedRegs) const {
   TargetFrameLowering::getCalleeSaves(MF, SavedRegs);
 
   // If we have the "returned" parameter attribute which guarantees that we
diff --git a/llvm/lib/Target/ARM/ARMFrameLowering.h b/llvm/lib/Target/ARM/ARMFrameLowering.h
index 8d2b8beb9..3c7358d8c 100644
--- a/llvm/lib/Target/ARM/ARMFrameLowering.h
+++ b/llvm/lib/Target/ARM/ARMFrameLowering.h
@@ -57,10 +57,14 @@ public:
   void getCalleeSaves(const MachineFunction &MF,
                       BitVector &SavedRegs) const override;
   void determineCalleeSaves(MachineFunction &MF, BitVector &SavedRegs,
                             RegScavenger *RS) const override;
 
+  /// Update the IsRestored flag on LR if it is spilled, based on the return
+  /// instructions.
+  static void updateLRRestored(MachineFunction &MF);
+
   void processFunctionBeforeFrameFinalized(
       MachineFunction &MF, RegScavenger *RS = nullptr) const override;
 
   void adjustForSegmentedStacks(MachineFunction &MF,
                                 MachineBasicBlock &MBB) const override;
diff --git a/llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp b/llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp
index ed9d30c3c..6121055eb 100644
--- a/llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp
+++ b/llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp
@@ -2060,21 +2060,10 @@ bool ARMLoadStoreOpt::MergeReturnIntoLDM(MachineBasicBlock &MBB) {
               Opcode == ARM::LDMIA_UPD) && "Unsupported multiple load-return!");
       PrevMI.setDesc(TII->get(NewOpc));
       MO.setReg(ARM::PC);
       PrevMI.copyImplicitOps(*MBB.getParent(), *MBBI);
       MBB.erase(MBBI);
-      // We now restore LR into PC so it is not live-out of the return block
-      // anymore: Clear the CSI Restored bit.
-      MachineFrameInfo &MFI = MBB.getParent()->getFrameInfo();
-      // CSI should be fixed after PrologEpilog Insertion
-      assert(MFI.isCalleeSavedInfoValid() && "CSI should be valid");
-      for (CalleeSavedInfo &Info : MFI.getCalleeSavedInfo()) {
-        if (Info.getReg() == ARM::LR) {
-          Info.setRestored(false);
-          break;
-        }
-      }
       return true;
     }
   }
   return false;
 }
@@ -2118,18 +2107,26 @@ bool ARMLoadStoreOpt::runOnMachineFunction(MachineFunction &Fn) {
 
   RegClassInfoValid = false;
   isThumb2 = AFI->isThumb2Function();
   isThumb1 = AFI->isThumbFunction() && !isThumb2;
 
-  bool Modified = false;
+  bool Modified = false, ModifiedLDMReturn = false;
   for (MachineBasicBlock &MBB : Fn) {
     Modified |= LoadStoreMultipleOpti(MBB);
     if (STI->hasV5TOps() && !AFI->shouldSignReturnAddress())
-      Modified |= MergeReturnIntoLDM(MBB);
+      ModifiedLDMReturn |= MergeReturnIntoLDM(MBB);
     if (isThumb1)
       Modified |= CombineMovBx(MBB);
   }
+  Modified |= ModifiedLDMReturn;
+
+  // If we merged a BX instruction into an LDM, we need to re-calculate whether
+  // LR is restored. This check needs to consider the whole function, not just
+  // the instruction(s) we changed, because there may be other BX returns which
+  // still need LR to be restored.
+  if (ModifiedLDMReturn)
+    ARMFrameLowering::updateLRRestored(Fn);
 
   Allocator.DestroyAll();
   return Modified;
 }
 
diff --git a/llvm/test/CodeGen/ARM/ldst-opt-lr-restored.ll b/llvm/test/CodeGen/ARM/ldst-opt-lr-restored.ll
index 883a81213..9494880f9 100644
--- a/llvm/test/CodeGen/ARM/ldst-opt-lr-restored.ll
+++ b/llvm/test/CodeGen/ARM/ldst-opt-lr-restored.ll
@@ -8,20 +8,21 @@
 define i32 @foo(ptr %ctx) {
 ; CHECK-LABEL: foo:
 ; CHECK:       @ %bb.0: @ %entry
 ; CHECK-NEXT:    cbz r0, .LBB0_2
 ; CHECK-NEXT:  @ %bb.1: @ %if.end
+; CHECK-NEXT:    movw r12, :lower16:val2
+; CHECK-NEXT:    movw r3, :lower16:val1
 ; CHECK-NEXT:    movw r2, :lower16:val0
 ; CHECK-NEXT:    mov r1, r0
 ; CHECK-NEXT:    movs r0, #0
-; CHECK-NEXT:    movw r12, :lower16:val2
-; CHECK-NEXT:    movw r3, :lower16:val1
-; CHECK-NEXT:    movt r2, :upper16:val0
-; CHECK-NEXT:    add.w lr, r1, #4
 ; CHECK-NEXT:    movt r12, :upper16:val2
 ; CHECK-NEXT:    movt r3, :upper16:val1
-; CHECK-NEXT:    stm.w lr, {r2, r3, r12}
+; CHECK-NEXT:    movt r2, :upper16:val0
+; CHECK-NEXT:    str r2, [r1, #4]
+; CHECK-NEXT:    str r3, [r1, #8]
+; CHECK-NEXT:    str.w r12, [r1, #12]
 ; CHECK-NEXT:    str r0, [r1, #16]
 ; CHECK-NEXT:    bx lr
 ; CHECK-NEXT:  .LBB0_2: @ %if.then
 ; CHECK-NEXT:    .save {r7, lr}
 ; CHECK-NEXT:    push {r7, lr}

Vulnerable code: void ARMFrameLowering::processFunctionBeforeFrameFinalized(
    MachineFunction &MF, RegScavenger *RS) const {
  TargetFrameLowering::processFunctionBeforeFrameFinalized(MF, RS);

      // We now restore LR into PC so it is not live-out of the return block
      // anymore: Clear the CSI Restored bit.
      MachineFrameInfo &MFI = MBB.getParent()->getFrameInfo();
      // CSI should be fixed after PrologEpilog Insertion
      assert(MFI.isCalleeSavedInfoValid() && "CSI should be valid");
      for (CalleeSavedInfo &Info : MFI.getCalleeSavedInfo()) {
        if (Info.getReg() == ARM::LR) {
          Info.setRestored(false);
          break;
        }
      }
  bool Modified = false;
      Modified |= MergeReturnIntoLDM(MBB);
; CHECK-NEXT:    movw r12, :lower16:val2
; CHECK-NEXT:    movw r3, :lower16:val1
; CHECK-NEXT:    movt r2, :upper16:val0
; CHECK-NEXT:    add.w lr, r1, #4
; CHECK-NEXT:    stm.w lr, {r2, r3, r12}
Fixed code: void ARMFrameLowering::updateLRRestored(MachineFunction &MF) {
void ARMFrameLowering::processFunctionBeforeFrameFinalized(
    MachineFunction &MF, RegScavenger *RS) const {
  TargetFrameLowering::processFunctionBeforeFrameFinalized(MF, RS);
  updateLRRestored(MF);
}

  /// Update the IsRestored flag on LR if it is spilled, based on the return
  /// instructions.
  static void updateLRRestored(MachineFunction &MF);

  bool Modified = false, ModifiedLDMReturn = false;
      ModifiedLDMReturn |= MergeReturnIntoLDM(MBB);
  Modified |= ModifiedLDMReturn;

  // If we merged a BX instruction into an LDM, we need to re-calculate whether
  // LR is restored. This check needs to consider the whole function, not just
  // the instruction(s) we changed, because there may be other BX returns which
  // still need LR to be restored.
  if (ModifiedLDMReturn)
    ARMFrameLowering::updateLRRestored(Fn);
; CHECK-NEXT:    movw r12, :lower16:val2
; CHECK-NEXT:    movw r3, :lower16:val1
; CHECK-NEXT:    movt r2, :upper16:val0
; CHECK-NEXT:    str r2, [r1, #4]
; CHECK-NEXT:    str r3, [r1, #8]
; CHECK-NEXT:    str.w r12, [r1, #12]