Skip site navigation (1)Skip section navigation (2)
Date:      Mon, 29 Jul 2019 18:02:49 +0000 (UTC)
From:      Dimitry Andric <dim@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-11@freebsd.org
Subject:   svn commit: r350419 - in stable: 11/contrib/llvm/lib/CodeGen 12/contrib/llvm/lib/CodeGen
Message-ID:  <201907291802.x6TI2nJP006221@repo.freebsd.org>

next in thread | raw e-mail | index | archive | help
Author: dim
Date: Mon Jul 29 18:02:48 2019
New Revision: 350419
URL: https://svnweb.freebsd.org/changeset/base/350419

Log:
  MFC r350362:
  
  Pull in r366369 from upstream llvm trunk (by Francis Visoiu Mistrih):
  
    [CodeGen][NFC] Simplify checks for stack protector index checking
  
    Use `hasStackProtectorIndex()` instead of `getStackProtectorIndex()
    >= 0`.
  
  Pull in r366371 from upstream llvm trunk (by Francis Visoiu Mistrih):
  
    [PEI] Don't re-allocate a pre-allocated stack protector slot
  
    The LocalStackSlotPass pre-allocates a stack protector and makes sure
    that it comes before the local variables on the stack.
  
    We need to make sure that later during PEI we don't re-allocate a new
    stack protector slot. If that happens, the new stack protector slot
    will end up being **after** the local variables that it should be
    protecting.
  
    Therefore, we would have two slots assigned for two different stack
    protectors, one at the top of the stack, and one at the bottom. Since
    PEI will overwrite the assigned slot for the stack protector, the
    load that is used to compare the value of the stack protector will
    use the slot assigned by PEI, which is wrong.
  
    For this, we need to check if the object is pre-allocated, and re-use
    that pre-allocated slot.
  
    Differential Revision: https://reviews.llvm.org/D64757
  
  Pull in r367068 from upstream llvm trunk (by Francis Visoiu Mistrih):
  
    [CodeGen] Don't resolve the stack protector frame accesses until PEI
  
    Currently, stack protector loads and stores are resolved during
    LocalStackSlotAllocation (if the pass needs to run). When this is the
    case, the base register assigned to the frame access is going to be
    one of the vregs created during LocalStackSlotAllocation. This means
    that we are keeping a pointer to the stack protector slot, and we're
    using this pointer to load and store to it.
  
    In case register pressure goes up, we may end up spilling this
    pointer to the stack, which can be a security concern.
  
    Instead, leave it to PEI to resolve the frame accesses. In order to
    do that, we make all stack protector accesses go through frame index
    operands, then PEI will resolve this using an offset from sp/fp/bp.
  
    Differential Revision: https://reviews.llvm.org/D64759
  
  Together, these fix a issue where the stack protection feature in LLVM's
  ARM backend can be rendered ineffective when the stack protector slot is
  re-allocated so that it appears after the local variables that it is
  meant to protect, leaving the function potentially vulnerable to a
  stack-based buffer overflow.
  
  Reported by:	andrew
  Security:	https://kb.cert.org/vuls/id/129209/

Modified:
  stable/11/contrib/llvm/lib/CodeGen/LocalStackSlotAllocation.cpp
  stable/11/contrib/llvm/lib/CodeGen/PrologEpilogInserter.cpp
Directory Properties:
  stable/11/   (props changed)

Changes in other areas also in this revision:
Modified:
  stable/12/contrib/llvm/lib/CodeGen/LocalStackSlotAllocation.cpp
  stable/12/contrib/llvm/lib/CodeGen/PrologEpilogInserter.cpp
Directory Properties:
  stable/12/   (props changed)

Modified: stable/11/contrib/llvm/lib/CodeGen/LocalStackSlotAllocation.cpp
==============================================================================
--- stable/11/contrib/llvm/lib/CodeGen/LocalStackSlotAllocation.cpp	Mon Jul 29 16:32:23 2019	(r350418)
+++ stable/11/contrib/llvm/lib/CodeGen/LocalStackSlotAllocation.cpp	Mon Jul 29 18:02:48 2019	(r350419)
@@ -200,19 +200,27 @@ void LocalStackSlotPass::calculateFrameObjectOffsets(M
   // Make sure that the stack protector comes before the local variables on the
   // stack.
   SmallSet<int, 16> ProtectedObjs;
-  if (MFI.getStackProtectorIndex() >= 0) {
+  if (MFI.hasStackProtectorIndex()) {
+    int StackProtectorFI = MFI.getStackProtectorIndex();
+
+    // We need to make sure we didn't pre-allocate the stack protector when
+    // doing this.
+    // If we already have a stack protector, this will re-assign it to a slot
+    // that is **not** covering the protected objects.
+    assert(!MFI.isObjectPreAllocated(StackProtectorFI) &&
+           "Stack protector pre-allocated in LocalStackSlotAllocation");
+
     StackObjSet LargeArrayObjs;
     StackObjSet SmallArrayObjs;
     StackObjSet AddrOfObjs;
 
-    AdjustStackOffset(MFI, MFI.getStackProtectorIndex(), Offset,
-                      StackGrowsDown, MaxAlign);
+    AdjustStackOffset(MFI, StackProtectorFI, Offset, StackGrowsDown, MaxAlign);
 
     // Assign large stack objects first.
     for (unsigned i = 0, e = MFI.getObjectIndexEnd(); i != e; ++i) {
       if (MFI.isDeadObjectIndex(i))
         continue;
-      if (MFI.getStackProtectorIndex() == (int)i)
+      if (StackProtectorFI == (int)i)
         continue;
 
       switch (MFI.getObjectSSPLayout(i)) {
@@ -343,6 +351,14 @@ bool LocalStackSlotPass::insertFrameReferenceRegisters
     int FrameIdx = FR.getFrameIndex();
     assert(MFI.isObjectPreAllocated(FrameIdx) &&
            "Only pre-allocated locals expected!");
+
+    // We need to keep the references to the stack protector slot through frame
+    // index operands so that it gets resolved by PEI rather than this pass.
+    // This avoids accesses to the stack protector though virtual base
+    // registers, and forces PEI to address it using fp/sp/bp.
+    if (MFI.hasStackProtectorIndex() &&
+        FrameIdx == MFI.getStackProtectorIndex())
+      continue;
 
     LLVM_DEBUG(dbgs() << "Considering: " << MI);
 

Modified: stable/11/contrib/llvm/lib/CodeGen/PrologEpilogInserter.cpp
==============================================================================
--- stable/11/contrib/llvm/lib/CodeGen/PrologEpilogInserter.cpp	Mon Jul 29 16:32:23 2019	(r350418)
+++ stable/11/contrib/llvm/lib/CodeGen/PrologEpilogInserter.cpp	Mon Jul 29 18:02:48 2019	(r350419)
@@ -845,18 +845,26 @@ void PEI::calculateFrameObjectOffsets(MachineFunction 
   // Make sure that the stack protector comes before the local variables on the
   // stack.
   SmallSet<int, 16> ProtectedObjs;
-  if (MFI.getStackProtectorIndex() >= 0) {
+  if (MFI.hasStackProtectorIndex()) {
+    int StackProtectorFI = MFI.getStackProtectorIndex();
     StackObjSet LargeArrayObjs;
     StackObjSet SmallArrayObjs;
     StackObjSet AddrOfObjs;
 
-    AdjustStackOffset(MFI, MFI.getStackProtectorIndex(), StackGrowsDown,
-                      Offset, MaxAlign, Skew);
+    // If we need a stack protector, we need to make sure that
+    // LocalStackSlotPass didn't already allocate a slot for it.
+    // If we are told to use the LocalStackAllocationBlock, the stack protector
+    // is expected to be already pre-allocated.
+    if (!MFI.getUseLocalStackAllocationBlock())
+      AdjustStackOffset(MFI, StackProtectorFI, StackGrowsDown, Offset, MaxAlign,
+                        Skew);
+    else if (!MFI.isObjectPreAllocated(MFI.getStackProtectorIndex()))
+      llvm_unreachable(
+          "Stack protector not pre-allocated by LocalStackSlotPass.");
 
     // Assign large stack objects first.
     for (unsigned i = 0, e = MFI.getObjectIndexEnd(); i != e; ++i) {
-      if (MFI.isObjectPreAllocated(i) &&
-          MFI.getUseLocalStackAllocationBlock())
+      if (MFI.isObjectPreAllocated(i) && MFI.getUseLocalStackAllocationBlock())
         continue;
       if (i >= MinCSFrameIndex && i <= MaxCSFrameIndex)
         continue;
@@ -864,8 +872,7 @@ void PEI::calculateFrameObjectOffsets(MachineFunction 
         continue;
       if (MFI.isDeadObjectIndex(i))
         continue;
-      if (MFI.getStackProtectorIndex() == (int)i ||
-          EHRegNodeFrameIndex == (int)i)
+      if (StackProtectorFI == (int)i || EHRegNodeFrameIndex == (int)i)
         continue;
 
       switch (MFI.getObjectSSPLayout(i)) {
@@ -884,6 +891,15 @@ void PEI::calculateFrameObjectOffsets(MachineFunction 
       llvm_unreachable("Unexpected SSPLayoutKind.");
     }
 
+    // We expect **all** the protected stack objects to be pre-allocated by
+    // LocalStackSlotPass. If it turns out that PEI still has to allocate some
+    // of them, we may end up messing up the expected order of the objects.
+    if (MFI.getUseLocalStackAllocationBlock() &&
+        !(LargeArrayObjs.empty() && SmallArrayObjs.empty() &&
+          AddrOfObjs.empty()))
+      llvm_unreachable("Found protected stack objects not pre-allocated by "
+                       "LocalStackSlotPass.");
+
     AssignProtectedObjSet(LargeArrayObjs, ProtectedObjs, MFI, StackGrowsDown,
                           Offset, MaxAlign, Skew);
     AssignProtectedObjSet(SmallArrayObjs, ProtectedObjs, MFI, StackGrowsDown,
@@ -905,8 +921,7 @@ void PEI::calculateFrameObjectOffsets(MachineFunction 
       continue;
     if (MFI.isDeadObjectIndex(i))
       continue;
-    if (MFI.getStackProtectorIndex() == (int)i ||
-        EHRegNodeFrameIndex == (int)i)
+    if (MFI.getStackProtectorIndex() == (int)i || EHRegNodeFrameIndex == (int)i)
       continue;
     if (ProtectedObjs.count(i))
       continue;



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201907291802.x6TI2nJP006221>