[X86] Added support for nocf_check attribute for indirect Branch Tracking
AcceptedPublic

Authored by oren_ben_simhon on Jan 9 2018, 12:40 PM.

Details

Summary

Jump Oriented Programming attacks rely on tampering addresses used by indirect call / jmp, e.g. redirect control-flow to non-programmer intended bytes in binary.
X86 Supports Indirect Branch Tracking (IBT) as part of Control-Flow Enforcement Technology (CET).
IBT instruments ENDBR instructions used to specify valid targets of indirect call / jmp.

The `nocf_check` attribute has two roles in the context of X86 IBT technology:

  1. Appertains to a function - do not add ENDBR instruction at the beginning of the function.
  2. Appertains to a function pointer - do not track the target function of this pointer by adding nocf_check prefix to the indirect-call instruction.

When the CPU decodes `nocf_check` prefix, it will not update IBT state machine, hence, the target addresses of the following indirect jump will not be tracked.
So in that case there is no need for ENDBR instructions.

The patch implements `nocf_check` context for Indirect Branch Tracking.
It also auto generates `nocf_check` prefixes before indirect branchs to jump tables that are guarded by range checks.
Those cases are common in switch-case statements and it is safe to optimize them.
Meaning instead of adding many ENDBR instructions for each target address of a case statement, we add a single`nocf_check` prefix before the indirect jump of the switch statement.

Diff Detail

Repository
rL LLVM
oren_ben_simhon edited the summary of this revision. (Show Details)Jan 9 2018, 12:41 PM
oren_ben_simhon edited the summary of this revision. (Show Details)
craig.topper added inline comments.Jan 11 2018, 10:08 AM
lib/Target/X86/X86InstrSystem.td
564 ↗(On Diff #129146)

I'm not sure I like feel very safe emitting the DS_PREFIX as a separate instruction. The glue won't exist after the SelectionDAG is converted to MIR so I'm not sure we would be guaranteed to keep it together.

I think I'd rather see special flavors of CALL*r/JMP*r that have a DS_PREFIX bit set in the TSFlags for their opcode and then the code emitter would know to emit the prefix. Basically what we do with LOCK and REP today. That way the instruction is one machine IR instruction that can never be separated.

lib/Target/X86/X86InstrSystem.td
564 ↗(On Diff #129146)

I share your concern. That is why I added the glue (for SelectionDAG) and X86nocf_check_calljmp pattern (for MI), which will make sure that only DS_PREFIX that precedes indirect jmp/call will be lowered.

Your solution means that i need to double the number of JMP/CALL MIs and DAG nodes. correct?
I am not sure that the cost is fair for esoteric feature like nocf_check prefix.

If you are still worried about future passes that change the order, what do you think about creating a bundle instead (DS_PREFIX<->JMP/CALL)?

Should we be printing the DS_PREFIX as "notrack" like gcc?

lib/Target/X86/X86InstrSystem.td
564 ↗(On Diff #129146)

I don't think we've ever used a bundle in x86 before.

Is there any kind of flag we could add to the MachineInstruction that we could expand in X86AsmPrinter when we're converting to MCInst.

Should we be printing the DS_PREFIX as "notrack" like gcc?

In LLVM you can't print two different instructions (notrack and ds) for the same encoding. The only solution to that is creating separate call/jmp instructions (just like lock/rep).
You can't use flags (TSFlags or MIFlags) to change the instruction encoding, you must use different call/jmp instructions.

So we are down to 2 options:

  1. Remain as is and benefit from call/jmp optimization with the risk of MI optimization that will change the order after instruction selection (unless we try using bundle).
  2. Use same mechanism as lock/rep, multiple the number of call/jmp instructions and ISEL nodes but have the flexibility to emit notrack instead of ds.

You suggest to use the second option. are we in sync?

You can have two instructions use the same encoding if you mark one with isAsmParserOnly = 1. See XACQUIRE_PREFIX and XRELEASE_PREFIX. isAsmParserOnly will hide from the disassembler that checks encoding collisions.

For sure you can't emit two instructions based on TSFlags.

I thought maybe with MIFlags you could catch the opcodes in X86AsmPrinter.cpp just before they are emitted and emit two instructions instead of one based on the MIFlag. It would be target specific code.

But I think separate opcodes is probably the easiest solution.

@RKSimon do you have any thoughts on this?

RKSimon resigned from this revision.Jan 23 2018, 4:03 AM
oren_ben_simhon marked an inline comment as done.Jan 23 2018, 7:41 AM

Thanks Craig for your comments, i made the following changes:

  • I print notrack prefix instead of ds prefix
  • I used same mechanism as lock/rep and created unified SDNode for the prefix and the jmp/call

I think you need to add support to the assembly parser to read assembly with notrack in it as well.

lib/Target/X86/X86ISelLowering.cpp
37874

createds is mispelled.

lib/Target/X86/X86ISelLowering.h
1110

SDLoc should be passed by const reference I think.

lib/Target/X86/X86IndirectBranchTracking.cpp
146

Is this TODO implemented by this patch? I didn't see anywhere that checks for the number of destination.

test/CodeGen/X86/indirect-branch-tracking.ll
8

DS_PREFIX should be NOTRACK

oren_ben_simhon marked 3 inline comments as done.Mon, Jan 29, 7:39 AM

I will add "notrack" to X86 ASM Parser (and also add symbolic test)

lib/Target/X86/X86IndirectBranchTracking.cpp
146

I decided to follow a better approach. I don't add ENDBR to guarded switch-case. Instead adds NOTRACK prefix when the switch-case is safe.

A safe/guarded switch-case is a an indirect branch that uses a jump table and does a range check before the branch. It usually happens in large switch-case statement.

So now, I change ISD::BRIND to ISD::NT_BRIND every time the branch originates from a jump table (ISD::BR_JT).
ISD::NT_BRIND will have no track prefix before it.

In case of jump tables, the target addresses of an indirect branch are not flagged as "address taken". So no ENDBR will be instrumented.

Implemented comments posted until 01/28 (Thanks Craig)

craig.topper added inline comments.Mon, Jan 29, 10:09 AM
lib/Target/X86/X86InstrControl.td
245

Can you name these as CALL16r_NT, etc? I think then the regular expressions in the scheduler models will pick them up and group them with their counterparts without the prefix.

oren_ben_simhon marked an inline comment as done.

The new attribute needs to be added to the LangRef.

Also I think new attributes are usually sent for RFC on llvm-dev. Was that done here?

Implemented comments posted until 01/30 (Thanks Craig)

This revision is now accepted and ready to land.Tue, Feb 13, 11:16 AM