Page MenuHomePhabricator

[cfi-verify] Validate there are no register clobbers between CFI-check and instruction execution.

Authored by hctim on Nov 8 2017, 3:14 PM.



This patch adds another failure mode for validateCFIProtection(..), wherein any register that affects the indirect control flow instruction is clobbered to between the CFI-check and the instruction's execution.

Also includes a modification to make MCInstrDesc::hasDefOfPhysReg public.

Diff Detail


Event Timeline

294 ↗(On Diff #122165)

I think Branch should explicitly be declared a reference to avoid copies.

306 ↗(On Diff #122165)

Reading the MCInstrDesc docs it makes it seem like this will ONLY check for implicit uses, not explicit uses. It's confusing why then your add $0, %eax unit tests below works, maybe it' s because you're implicitly defining rax by adding to eax. Could you try using an add rax instruction instead and see if that works?

304 ↗(On Diff #122165)
BranchNode.IndirectCFIsOnTargetPath = (BranchTarget == Address);
if (BranchNode.IndirectCFIsOnTargetPath) {
hctim updated this revision to Diff 122520.Nov 10 2017, 1:55 PM
hctim marked 3 inline comments as done.

Addressed Vlad's comments: Added an explicit register check and some minor changes.

Also changed instances of 'spill' to 'clobber' as it's more representative of what we're actually checking for.

294 ↗(On Diff #122165)

Thanks, nice catch!

306 ↗(On Diff #122165)

I was similarly confused by the naming/comment of hasImplicitDefOfPhysReg, I've added a test and it's all good. It's interesting as getImplicitUses is described as "Return a list of registers that are potentially read by any instance of this machine instruction", which would indicate that it also gets the explicit uses.

I'm not sure why the documentation is (apparently) wrong for hasImplicitDefOfPhysReg...

751 ↗(On Diff #122520)

Take a look at [1]. This uses 0x05 which is an encoding specifically for add %eax, if you use an instruction that explicit encodes %rax in the ModR/M encoding like 48 83 c0 00 this test will fail. This is a bit silly since from the assembly writers perspective %rax is explicit here but it's not at the MC layer.


*Also this still says %eax

751 ↗(On Diff #122520)

Oh, I forgot to mention you can witness this with llvm-mc:

$ echo "0x48 0x83 0xc0 0x00" | llvm-mc --disassemble -show-inst -show-inst-operands
	addq	$0, %rax                # <MCInst #187 ADD64ri8
                                        #  <MCOperand Reg:35>
                                        #  <MCOperand Reg:35>
                                        #  <MCOperand Imm:0>>
$ echo "0x48 0x05 0x00 0x00 0x00 0x00" | llvm-mc --disassemble -show-inst -show-inst-operands
	addq	$0, %rax                # <MCInst #181 ADD64i32
                                        #  <MCOperand Imm:0>>
hctim updated this revision to Diff 122718.EditedNov 13 2017, 1:27 PM

Updated clobber checking to use hasDefOfPhysReg rather than the implicit counterpart. This change now includes a modification to make MCInstrDesc::hasDefOfPhysReg public on pcc's reccomendation.

Also added an explicit unit test that sets rax then jumps to it, rather than relying on a REX prefix byte to target the register, as this was still considered an 'implicitly' changed register.

hctim retitled this revision from [cfi-verify] Validate there are no spills between CFI-check and instruction execution. to [cfi-verify] Validate there are no register clobbers between CFI-check and instruction execution..Nov 13 2017, 1:28 PM
hctim edited the summary of this revision. (Show Details)
vlad.tsyrklevich accepted this revision.Nov 14 2017, 1:44 PM
This revision is now accepted and ready to land.Nov 14 2017, 1:44 PM
hctim updated this revision to Diff 122919.Nov 14 2017, 2:53 PM

Merged master in preparation for submission.

hctim updated this revision to Diff 122920.Nov 14 2017, 2:54 PM


This revision was automatically updated to reflect the committed changes.