Page MenuHomePhabricator

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

Authored by sstankovic on Feb 28 2014, 2:57 PM.

Details

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).

Diff Detail

Event Timeline

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 ↗(On Diff #7440)

Nit: "after instructions that change the stack pointer"

32 ↗(On Diff #7440)

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

184 ↗(On Diff #7440)

Nit: this should probably be unsigned

192 ↗(On Diff #7440)

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.

206 ↗(On Diff #7440)

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
549 ↗(On Diff #7440)

Do you mean "instructions that must be masked"?

550 ↗(On Diff #7440)

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 updated this revision to Unknown Object (????).Mar 4 2014, 9:31 AM

Patch is updated.

sstankovic added inline comments.Mar 4 2014, 9:33 AM
lib/Target/Mips/MCTargetDesc/MipsNaClELFStreamer.cpp
32 ↗(On Diff #7440)

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 added inline comments.Mar 7 2014, 11:13 AM
lib/Target/Mips/MCTargetDesc/MipsNaClELFStreamer.cpp
58 ↗(On Diff #7510)

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 ↗(On Diff #7510)

Since only one arg changes below, you could write:

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

without a conditional. (But see other comments.)

120 ↗(On Diff #7510)

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?

32 ↗(On Diff #7440)

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?

test/MC/Mips/nacl-branch-delay.ll
3 ↗(On Diff #7510)

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 updated this revision to Unknown Object (????).Mar 7 2014, 2:48 PM

Patch is updated.

sstankovic added inline comments.Mar 7 2014, 2:50 PM
test/MC/Mips/nacl-branch-delay.ll
3 ↗(On Diff #7510)

Done.

mseaborn added inline comments.Mar 7 2014, 3:03 PM
lib/Target/Mips/MCTargetDesc/MipsNaClELFStreamer.cpp
100 ↗(On Diff #7651)

Maybe "IsLoad" for readability?

105 ↗(On Diff #7651)

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 updated this revision to Unknown Object (????).Mar 8 2014, 7:46 AM

Patch is updated.

LGTM, thanks

lib/Target/Mips/MCTargetDesc/MipsNaClELFStreamer.cpp
99 ↗(On Diff #7661)

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

111 ↗(On Diff #7661)

Can be "bool IsMemAccess = ..."

137 ↗(On Diff #7661)

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

lib/Target/Mips/MipsDelaySlotFiller.cpp
19 ↗(On Diff #7661)

Nit: sort #includes?

test/MC/Mips/nacl-mask.s
215 ↗(On Diff #7661)

"not" -> "nor"

sstankovic updated this revision to Unknown Object (????).Mar 10 2014, 6:57 AM

Patch is updated.

sstankovic closed this revision.Mar 10 2014, 1:41 PM

Closed by commit rL203484 (authored by @sstankovic).