Patch that adds address-masking instructions before loads and stores, and after the instructions that change stack-pointer.

[sstankovic]

Summary

[mips] Implement NaCl sandboxing of loads, stores and SP changes:

• Add masking instructions before loads and stores (in MC layer).
• Add masking instructions after SP changes (in MC layer).
• Forbid loads, stores and SP changes in delay slots (in MI layer).

[mseaborn]

As with the previous change, can you make the commit message more informative by explaining the context about NaCl sandboxing?

lib/Target/Mips/MCTargetDesc/MipsNaClELFStreamer.cpp

13

Nit: "after instructions that change the stack pointer"

32

Could this be combined with isDangerousStoreOpcode() as isDangerousMemoryAccess()? There's not a big difference between loads and stores for sandboxing.

109

Nit: this should probably be unsigned

117

Having a separate function for that seems a bit repetitive. How about:

bool SandboxAddr = isDangerousMemAccess(Inst, &AddrIdx);
bool SandboxStack = isStackChange(Inst);
if (SandboxAddr || SandboxStack) {

EmitBundleLock(false);
if (SandboxAddr) // emit masking...
MCELFStreamer::EmitInstruction(MI, STI);
if (SandboxStack) // emit masking...
EmitBundleUnlock();

} else {

MCELFStreamer::EmitInstruction(MI, STI);

}

And omitting the EmitBundleLock() in the normal case is just on the hunch that it might be slow.

131

I don't think you want to be referring to MachineInstr from a file in the MC layer, because MachineInstr is in a higher layer.

This search currently gives no matches:
git ls-files | grep MCTargetDesc | xargs grep MachineInstr

isDangerousMemAccess() isn't very large, so I think it would be OK to duplicate it in MipsDelaySlotFiller.cpp, and export isDangerousMemAccessOpcode() from the MC layer.

lib/Target/Mips/MipsDelaySlotFiller.cpp

535

Do you mean "instructions that must be masked"?

536

I think this Triple() constructor parses the string, so, for performance, you might not want to be doing this inside a loop.

Can Subtarget->isTargetNaCl() be got in this context?

test/MC/Mips/nacl-align.ll

2 ↗

(On Diff #7440)

If you're fixing a mistake in a previous commit, really that should be commented in the commit message. But I would prefer if you fixed this in a standalone change first.

[sstankovic]

Patch is updated.

[sstankovic]

lib/Target/Mips/MCTargetDesc/MipsNaClELFStreamer.cpp

32

There have to be separate methods for loads and stores because of the case where
load also changes SP (or they can be combined in one method with one additional
parameter that tells whether we check loads or stores).

test/MC/Mips/nacl-align.ll

2 ↗

(On Diff #7440)

I committed this as a separate change.

[mseaborn]

lib/Target/Mips/MCTargetDesc/MipsNaClELFStreamer.cpp

32

I see what you mean. However, this leads to a bug (see other comment). How about having an isMemoryAccess() which also returns whether the instruction is a load or a store?

58

This name is a bit misleading because it will return true for store instructions which only read $sp. You're relying on the caller first checking whether the instruction is a store, which fails in one case -- see other comment.

114

Since only one arg changes below, you could write:

sandboxLoadStoreStackChange(Inst, AddrIdx, STI, true, isStackChange(Inst));

without a conditional. (But see other comments.)

120

Currently this case kicks in for:

sw $sp, 123($sp)

and will convert it to:

sw $sp, 123($sp)
and $sp, $sp, $15

which is a bug. Can you fix this and add a test for it, please?

test/MC/Mips/nacl-branch-delay.ll

3

This file belongs in test/CodeGen -- it's not testing the MC layer.

I didn't notice this when you added test/MC/Mips/nacl-align.ll. Could you do a separate change to move that to test/CodeGen, please?

[sstankovic]

Patch is updated.

[sstankovic]

test/MC/Mips/nacl-branch-delay.ll

3

Done.

[mseaborn]

lib/Target/Mips/MCTargetDesc/MipsNaClELFStreamer.cpp

110

Maybe "IsLoad" for readability?

114

Now this doesn't handle this case correctly:
lw $sp, 123($sp)

It doesn't generate a mask after the load. Can you fix this and add a test, please?

[sstankovic]

Patch is updated.

[mseaborn]

LGTM, thanks

lib/Target/Mips/MCTargetDesc/MipsNaClELFStreamer.cpp

111

Nit: It would be more usual C++ style to declare these immediately before they're used...

127

Can be "bool IsMemAccess = ..."

133

I think usual LLVM style is to omit "!= NULL"

lib/Target/Mips/MipsDelaySlotFiller.cpp

19

Nit: sort #includes?

test/MC/Mips/nacl-mask.s

215

"not" -> "nor"

[sstankovic]

Patch is updated.

[sstankovic]

Closed by commit rL203484 (authored by @sstankovic).

lib/Target/Mips/MCTargetDesc/MipsMCNaCl.h

 
 #include "llvm/MC/MCELFStreamer.h"
 
 namespace llvm {
 
 // Log2 of the NaCl MIPS sandbox's instruction bundle size.
 static const unsigned MIPS_NACL_BUNDLE_ALIGN = 4u;
 
+bool isDangerousLoadOpcode(unsigned Opcode, unsigned *AddrIdx);
+bool isDangerousStoreOpcode(unsigned Opcode, unsigned *AddrIdx);
+bool registerNeedsLoadStoreMask(unsigned Reg);
+
 // This function creates an MCELFStreamer for Mips NaCl.
 MCELFStreamer *createMipsNaClELFStreamer(MCContext &Context, MCAsmBackend &TAB,
                                          raw_ostream &OS,
                                          MCCodeEmitter *Emitter,
                                          bool RelaxAll, bool NoExecStack);
 
 }
 
 #endif

lib/Target/Mips/MCTargetDesc/MipsNaClELFStreamer.cpp

 //===-- MipsNaClELFStreamer.cpp - ELF Object Output for Mips NaCl ---------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This file implements MCELFStreamer for Mips NaCl.  It emits .o object files
 // as required by NaCl's SFI sandbox.  It inserts address-masking instructions
-// before dangerous control-flow instructions.  It aligns on bundle size all
-// functions and all targets of indirect branches.
+// before dangerous control-flow and memory access instructions.  It inserts
+// address-masking instructions after instructions that change the stack
+// pointer.  It ensures that the mask and the dangerous instruction are always
+// emitted in the same bundle.
 //
 //===----------------------------------------------------------------------===//
 
 #define DEBUG_TYPE "mips-mc-nacl"
 
 #include "Mips.h"
 #include "MipsMCNaCl.h"
 #include "llvm/MC/MCELFStreamer.h"
 
 using namespace llvm;
 
 namespace {
 
 const unsigned IndirectBranchMaskReg = Mips::T6;
+const unsigned LoadStoreStackMaskReg = Mips::T7;
 
 /// Extend the generic MCELFStreamer class so that it can mask dangerous
 /// instructions.
 
 class MipsNaClELFStreamer : public MCELFStreamer {
 public:
   MipsNaClELFStreamer(MCContext &Context, MCAsmBackend &TAB, raw_ostream &OS,
                       MCCodeEmitter *Emitter)
     : MCELFStreamer(Context, TAB, OS, Emitter) {}
 
   ~MipsNaClELFStreamer() {}
 
 private:
   bool isIndirectJump(const MCInst &MI) {
     return MI.getOpcode() == Mips::JR || MI.getOpcode() == Mips::RET;
   }
 
+  bool isDangerousLoad(const MCInst &MI, unsigned *AddrIdx) {
+    return (isDangerousLoadOpcode(MI.getOpcode(), AddrIdx)
+            && registerNeedsLoadStoreMask(MI.getOperand(*AddrIdx).getReg()));
+  }
+
+  bool isDangerousStore(const MCInst &MI, unsigned *AddrIdx) {
+    return (isDangerousStoreOpcode(MI.getOpcode(), AddrIdx)
+            && registerNeedsLoadStoreMask(MI.getOperand(*AddrIdx).getReg()));
+  }
+
+  bool isStackChange(const MCInst &MI) {
+    return (MI.getNumOperands() > 0 && MI.getOperand(0).isReg()
+            && MI.getOperand(0).getReg() == Mips::SP);
+  }
+
   void emitMask(unsigned AddrReg, unsigned MaskReg,
                 const MCSubtargetInfo &STI) {
     MCInst MaskInst;
     MaskInst.setOpcode(Mips::AND);
     MaskInst.addOperand(MCOperand::CreateReg(AddrReg));
     MaskInst.addOperand(MCOperand::CreateReg(AddrReg));
     MaskInst.addOperand(MCOperand::CreateReg(MaskReg));
     MCELFStreamer::EmitInstruction(MaskInst, STI);
   }
 
   // Sandbox indirect branch or return instruction by inserting mask operation
   // before it.
   void sandboxIndirectJump(const MCInst &MI, const MCSubtargetInfo &STI) {
     unsigned AddrReg = MI.getOperand(0).getReg();
 
     EmitBundleLock(false);
     emitMask(AddrReg, IndirectBranchMaskReg, STI);
     MCELFStreamer::EmitInstruction(MI, STI);
     EmitBundleUnlock();
   }
 
+  // Sandbox memory access or SP change.  Insert mask operation before and/or
+  // after the instruction.
+  void sandboxLoadStoreStackChange(const MCInst &MI, unsigned AddrIdx,
+                                   const MCSubtargetInfo &STI, bool MaskBefore,
+                                   bool MaskAfter) {
+    EmitBundleLock(false);
+    if (MaskBefore) {
+      // Sandbox memory access.
+      unsigned BaseReg = MI.getOperand(AddrIdx).getReg();
+      emitMask(BaseReg, LoadStoreStackMaskReg, STI);
+    }
+    MCELFStreamer::EmitInstruction(MI, STI);
+    if (MaskAfter) {
+      // Sandbox SP change.
+      unsigned SPReg = MI.getOperand(0).getReg();
+      assert((Mips::SP == SPReg) && "Unexpected stack-pointer register.");
+      emitMask(SPReg, LoadStoreStackMaskReg, STI);
+    }
+    EmitBundleUnlock();
+  }
+
 public:
   /// This function is the one used to emit instruction data into the ELF
   /// streamer.  We override it to mask dangerous instructions.
   virtual void EmitInstruction(const MCInst &Inst, const MCSubtargetInfo &STI) {
+    unsigned AddrIdx;
+
     if (isIndirectJump(Inst))
       sandboxIndirectJump(Inst, STI);
+    else if (isDangerousLoad(Inst, &AddrIdx)) {
+      if (!isStackChange(Inst))
+        sandboxLoadStoreStackChange(Inst, AddrIdx, STI, true, false);
+      else
+        sandboxLoadStoreStackChange(Inst, AddrIdx, STI, true, true);
+    } else if (isDangerousStore(Inst, &AddrIdx))
+      sandboxLoadStoreStackChange(Inst, AddrIdx, STI, true, false);
+    else if (isStackChange(Inst))
+      sandboxLoadStoreStackChange(Inst, -1U/*not used*/, STI, false, true);
     else
       MCELFStreamer::EmitInstruction(Inst, STI);
   }
 };
 
 } // end anonymous namespace
 
 namespace llvm {
 
+bool isDangerousLoadOpcode(unsigned Opcode, unsigned *AddrIdx) {
+  switch (Opcode) {
+  default:
+    return false;
+
+  // Instructions with base address register in position 1.
+  case Mips::LB:
+  case Mips::LBu:
+  case Mips::LH:
+  case Mips::LHu:
+  case Mips::LW:
+  case Mips::LWC1:
+  case Mips::LDC1:
+  case Mips::LL:
+  case Mips::LWL:
+  case Mips::LWR:
+    *AddrIdx = 1;
+    return true;
+  }
+}
+
+bool isDangerousStoreOpcode(unsigned Opcode, unsigned *AddrIdx) {
+  switch (Opcode) {
+  default:
+    return false;
+
+  // Instructions with base address register in position 1.
+  case Mips::SB:
+  case Mips::SH:
+  case Mips::SW:
+  case Mips::SWC1:
+  case Mips::SDC1:
+  case Mips::SWL:
+  case Mips::SWR:
+    *AddrIdx = 1;
+    return true;
+
+  // Instructions with base address register in position 2.
+  case Mips::SC:
+    *AddrIdx = 2;
+    return true;
+  }
+}
+
+bool registerNeedsLoadStoreMask(unsigned Reg) {
+  // The contents of SP and thread pointer register do not require masking.
+  return Reg != Mips::SP && Reg != Mips::T8;
+}
+
 MCELFStreamer *createMipsNaClELFStreamer(MCContext &Context, MCAsmBackend &TAB,
                                          raw_ostream &OS,
                                          MCCodeEmitter *Emitter, bool RelaxAll,
                                          bool NoExecStack) {
   MipsNaClELFStreamer *S = new MipsNaClELFStreamer(Context, TAB, OS, Emitter);
   if (RelaxAll)
     S->getAssembler().setRelaxAll(true);
   if (NoExecStack)

lib/Target/Mips/MipsDelaySlotFiller.cpp

 //
 //===----------------------------------------------------------------------===//
 
 #define DEBUG_TYPE "delay-slot-filler"
 
 #include "Mips.h"
 #include "MipsInstrInfo.h"
 #include "MipsTargetMachine.h"
+#include "MCTargetDesc/MipsMCNaCl.h"
 #include "llvm/ADT/BitVector.h"
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/Statistic.h"
 #include "llvm/Analysis/AliasAnalysis.h"
 #include "llvm/Analysis/ValueTracking.h"
 #include "llvm/CodeGen/MachineBranchProbabilityInfo.h"
 #include "llvm/CodeGen/MachineFunctionPass.h"
 #include "llvm/CodeGen/MachineInstrBuilder.h"
if (terminateSearch(*I))
       break;
 
     assert((!I->isCall() && !I->isReturn() && !I->isBranch()) &&
            "Cannot put calls, returns or branches in delay slot.");
 
     if (delayHasHazard(*I, RegDU, IM))
       continue;
 
+    if (TM.getSubtarget<MipsSubtarget>().isTargetNaCl()) {
+      // In NaCl, instructions that must be masked are forbidden in delay slots.
+      // We only check for loads, stores and SP changes.  Calls, returns and
+      // branches are not checked because non-NaCl targets never put them in
+      // delay slots.
+      unsigned AddrIdx;
+      if ((isDangerousLoadOpcode(I->getOpcode(), &AddrIdx)
+           && registerNeedsLoadStoreMask(I->getOperand(AddrIdx).getReg()))
+          || (isDangerousStoreOpcode(I->getOpcode(), &AddrIdx)
+              && registerNeedsLoadStoreMask(I->getOperand(AddrIdx).getReg()))
+          || I->modifiesRegister(Mips::SP, TM.getRegisterInfo()))
+        continue;
+    }
+
     Filler = I;
     return true;
   }
 
   return false;
 }
 
 bool Filler::searchBackward(MachineBasicBlock &MBB, Iter Slot) const {

test/MC/Mips/nacl-branch-delay.ll

+; RUN: llc -filetype=asm -mtriple=mipsel-none-linux -relocation-model=static \
+; RUN:     -O3 < %s | FileCheck %s
+
+; RUN: llc -filetype=asm -mtriple=mipsel-none-nacl -relocation-model=static \
+; RUN:     -O3 < %s | FileCheck %s -check-prefix=CHECK-NACL
+
+@x = global i32 0, align 4
+declare void @f1(i32)
+declare void @f2()
+
+
+define void @test1() {
+  %1 = load i32* @x, align 4
+  call void @f1(i32 %1)
+  ret void
+
+
+; CHECK-LABEL:       test1
+
+; We first make sure that for non-NaCl targets branch-delay slot contains
+; dangerous instructions.
+
+; Check that branch-delay slot is used to load argument from x before function
+; call.
+
+; CHECK:             jal
+; CHECK-NEXT:        lw      $4, %lo(x)(${{[0-9]+}})
+
+; Check that branch-delay slot is used for adjusting sp before return.
+
+; CHECK:             jr      $ra
+; CHECK-NEXT:        addiu   $sp, $sp, {{[0-9]+}}
+
+
+; For NaCl, check that branch-delay slot doesn't contain dangerous instructions.
+
+; CHECK-NACL:             jal
+; CHECK-NACL-NEXT:        nop
+
+; CHECK-NACL:             jr      $ra
+; CHECK-NACL-NEXT:        nop
+}
+
+
+define void @test2() {
+  store i32 1, i32* @x, align 4
+  tail call void @f2()
+  ret void
+
+
+; CHECK-LABEL:       test2
+
+; Check that branch-delay slot is used for storing to x before function call.
+
+; CHECK:             jal
+; CHECK-NEXT:        sw      ${{[0-9]+}}, %lo(x)(${{[0-9]+}})
+
+; Check that branch-delay slot is used for adjusting sp before return.
+
+; CHECK:             jr      $ra
+; CHECK-NEXT:        addiu   $sp, $sp, {{[0-9]+}}
+
+
+; For NaCl, check that branch-delay slot doesn't contain dangerous instructions.
+
+; CHECK-NACL:             jal
+; CHECK-NACL-NEXT:        nop
+
+; CHECK-NACL:             jr      $ra
+; CHECK-NACL-NEXT:        nop
+}

test/MC/Mips/nacl-mask.s

 # RUN: llvm-mc -filetype=obj -triple=mipsel-unknown-nacl %s \
 # RUN:  | llvm-objdump -triple mipsel -disassemble -no-show-raw-insn - \
 # RUN:  | FileCheck %s
 
 # This test tests that address-masking sandboxing is added when given assembly
 # input.
 
+
+# Test that address-masking sandboxing is added before indirect branches and
+# returns.
+
 test1:
   .set  noreorder
 
         jr  $a0
         nop
         jr  $ra
         nop
 
 # CHECK-LABEL:   test1:
 
 # CHECK:         and     $4, $4, $14
 # CHECK-NEXT:    jr      $4
 
 # Check that additional nop is inserted, to align mask and jr to the next
 # bundle.
 
 # CHECK-NEXT:    nop
 # CHECK-NEXT:    nop
 
 # CHECK:         and     $ra, $ra, $14
 # CHECK-NEXT:    jr      $ra
+
+
+
+# Test that address-masking sandboxing is added before load instructions.
+
+test2:
+  .set  noreorder
+
+        lb      $4, 0($1)
+        nop
+        lbu     $4, 0($2)
+        lh      $4, 0($3)
+        lhu     $1, 0($4)
+        lw      $4, 0($5)
+        lwc1    $f0, 0($6)
+        ldc1    $f2, 0($7)
+        ll      $4, 0($8)
+        lwl     $4, 0($9)
+        lwr     $4, 0($10)
+
+        lw      $4, 0($sp)
+        lw      $4, 0($t8)
+
+# CHECK-LABEL:   test2:
+
+# CHECK:         and     $1, $1, $15
+# CHECK-NEXT:    lb      $4, 0($1)
+
+# Check that additional nop is inserted, to align mask and load to the next
+# bundle.
+
+# CHECK:         nop
+# CHECK:         nop
+
+# CHECK:         and     $2, $2, $15
+# CHECK-NEXT:    lbu     $4, 0($2)
+
+# CHECK:         and     $3, $3, $15
+# CHECK-NEXT:    lh      $4, 0($3)
+
+# CHECK:         and     $4, $4, $15
+# CHECK-NEXT:    lhu     $1, 0($4)
+
+# CHECK:         and     $5, $5, $15
+# CHECK-NEXT:    lw      $4, 0($5)
+
+# CHECK:         and     $6, $6, $15
+# CHECK-NEXT:    lwc1    $f0, 0($6)
+
+# CHECK:         and     $7, $7, $15
+# CHECK-NEXT:    ldc1    $f2, 0($7)
+
+# CHECK:         and     $8, $8, $15
+# CHECK-NEXT:    ll      $4, 0($8)
+
+# CHECK:         and     $9, $9, $15
+# CHECK-NEXT:    lwl     $4, 0($9)
+
+# CHECK:         and     $10, $10, $15
+# CHECK-NEXT:    lwr     $4, 0($10)
+
+
+# Check that loads where base register is $sp or $t8 (thread pointer register)
+# are not masked.
+
+# CHECK-NOT:     and
+# CHECK:         lw      $4, 0($sp)
+# CHECK-NOT:     and
+# CHECK:         lw      $4, 0($24)
+
+
+
+# Test that address-masking sandboxing is added before store instructions.
+
+test3:
+  .set  noreorder
+
+        sb      $4, 0($1)
+        nop
+        sh      $4, 0($2)
+        sw      $4, 0($3)
+        swc1    $f0, 0($4)
+        sdc1    $f2, 0($5)
+        swl     $4, 0($6)
+        swr     $4, 0($7)
+        sc      $4, 0($8)
+
+        sw      $4, 0($sp)
+        sw      $4, 0($t8)
+
+# CHECK-LABEL:   test3:
+
+# CHECK:         and     $1, $1, $15
+# CHECK-NEXT:    sb      $4, 0($1)
+
+# Check that additional nop is inserted, to align mask and store to the next
+# bundle.
+
+# CHECK:         nop
+# CHECK:         nop
+
+# CHECK:         and     $2, $2, $15
+# CHECK-NEXT:    sh      $4, 0($2)
+
+# CHECK:         and     $3, $3, $15
+# CHECK-NEXT:    sw      $4, 0($3)
+
+# CHECK:         and     $4, $4, $15
+# CHECK-NEXT:    swc1    $f0, 0($4)
+
+# CHECK:         and     $5, $5, $15
+# CHECK-NEXT:    sdc1    $f2, 0($5)
+
+# CHECK:         and     $6, $6, $15
+# CHECK-NEXT:    swl     $4, 0($6)
+
+# CHECK:         and     $7, $7, $15
+# CHECK-NEXT:    swr     $4, 0($7)
+
+# CHECK:         and     $8, $8, $15
+# CHECK-NEXT:    sc      $4, 0($8)
+
+
+# Check that stores where base register is $sp or $t8 (thread pointer register)
+# are not masked.
+
+# CHECK-NOT:     and
+# CHECK:         sw      $4, 0($sp)
+# CHECK-NOT:     and
+# CHECK:         sw      $4, 0($24)
+
+
+
+# Test that address-masking sandboxing is added after instructions that change
+# stack pointer.
+
+test4:
+  .set  noreorder
+
+        addiu   $sp, $sp, 24
+        nop
+        addu    $sp, $sp, $1
+        lw      $sp, 0($2)
+
+# CHECK-LABEL:   test4:
+
+# CHECK:         addiu   $sp, $sp, 24
+# CHECK-NEXT:    and     $sp, $sp, $15
+
+# Check that additional nop is inserted, to align instruction and mask to the
+# next bundle.
+
+# CHECK:         nop
+# CHECK:         nop
+
+# CHECK:         addu    $sp, $sp, $1
+# CHECK-NEXT:    and     $sp, $sp, $15
+
+# Since we next check sandboxing sequence which consists of 3 instructions,
+# check that 2 additional nops are inserted, to align it to the next bundle.
+
+# CHECK:         nop
+# CHECK:         nop
+
+
+# Check that for instructions that change stack-pointer and load from memory
+# masks are added before and after the instruction.
+
+# CHECK:         and     $2, $2, $15
+# CHECK-NEXT:    lw      $sp, 0($2)
+# CHECK-NEXT:    and     $sp, $sp, $15