Page MenuHomePhabricator

[exceptions] Upgrade exception handlers when stack protector is used
ClosedPublic

Authored by etienneb on Jun 7 2016, 1:38 PM.

Details

Summary

MSVC provide exception handlers with enhanced information to deal with security buffer feature (/GS).

To be more secure, the security cookies (GS and SEH) are validated when unwinding the stack.

The following code:

void f() {}

void foo() {
  __try {
    f();
  } __except(1) {
    f();
  }
}

Diff Detail

Event Timeline

etienneb updated this revision to Diff 59945.Jun 7 2016, 1:38 PM
etienneb retitled this revision from to [exceptions] Upgrade exception handlers when stack protector is used.
etienneb updated this object.
etienneb added a reviewer: rnk.

This is a draft on the exception handlers upgrade.
For now, only 32-bits is implemented.

It's producing this code:

	.text
	.def	 @feat.00;
	.scl	3;
	.type	0;
	.endef
	.globl	@feat.00
@feat.00 = 1
	.def	 "?example@@YAHHH@Z";
	.scl	2;
	.type	32;
	.endef
	.globl	"?example@@YAHHH@Z"
	.p2align	4, 0x90
"?example@@YAHHH@Z":                    # @"\01?example@@YAHHH@Z"
Lfunc_begin0:
# BB#0:
	pushl	%ebp
	movl	%esp, %ebp
	pushl	%ebx
	pushl	%edi
	pushl	%esi
	subl	$48, %esp
	movl	$"L__ehtable$?example@@YAHHH@Z", %edx
	movl	%esp, -36(%ebp)
	movl	$-2, -16(%ebp)
	movl	12(%ebp), %ecx
	movl	8(%ebp), %eax
"L?example@@YAHHH@Z$frame_escape_0" = -44
	xorl	___security_cookie, %edx
	movl	%edx, -20(%ebp)
	movl	$__except_handler4, -24(%ebp)
	leal	-28(%ebp), %edx
	movl	%fs:0, %esi
	movl	%esi, -28(%ebp)
	movl	%edx, %fs:0
	movl	%ecx, -40(%ebp)
	movl	%eax, -48(%ebp)
	movl	$0, -16(%ebp)
	leal	-58(%ebp), %ecx
	movl	-40(%ebp), %eax
	pushl	%eax
	pushl	$204
	pushl	%ecx
	calll	"?memset@@YAPAXPAXHH@Z"
	addl	$12, %esp
LBB0_2:
	movl	-40(%ebp), %eax
	movl	-28(%ebp), %ecx
	movsbl	-58(%ebp,%eax), %eax
	movl	%ecx, %fs:0
	addl	$48, %esp
	popl	%esi
	popl	%edi
	popl	%ebx
	popl	%ebp
	retl
LBB0_1:
	movl	-24(%ebp), %esp
	addl	$12, %ebp
	jmp	LBB0_2

With MSVC compiler, every exception handlers in the same compilation unit are upgraded (with /GS).
With this approach, only functions with EH and GS are upgraded.

rnk edited edge metadata.Jun 7 2016, 4:11 PM

Test? You should be able to add an IR-only test that uses opt -winehprepare -stack-protector like test/CodeGen/WinEH/wineh-cloning.ll.

include/llvm/Analysis/EHPersonalities.h
74 ↗(On Diff #59945)

Code in lib/Analysis isn't supposed to modify IR, but inserting a declaration into the module should be fine. Maybe a better name would be getStackGuardEHPersonality, though, since it doesn't actually modify the IR.

lib/Analysis/EHPersonalities.cpp
51 ↗(On Diff #59945)

Since we will extend this soon, here's a cute thing you can do:

StringRef NewName = llvm::StringSwitch<StringRef>(F->getName())
              .Case("_except_handler3", "_except_handler4")
              .Case("__C_specific_handler", "__GSHandlerCheck_SEH")
              .Default("");
if (NewName.empty())
  return nullptr;
return M->getOrInsertFunction(NewName, ...);
etienneb edited edge metadata.Jun 9 2016, 10:54 AM
etienneb added a subscriber: chrisha.
etienneb updated this revision to Diff 60204.Jun 9 2016, 11:01 AM
etienneb marked 2 inline comments as done.

add test

etienneb edited reviewers, added: majnemer; removed: llvm-commits.Jun 10 2016, 12:22 PM
etienneb added a subscriber: llvm-commits.
majnemer added inline comments.Jun 10 2016, 12:55 PM
include/llvm/Analysis/EHPersonalities.h
72–74 ↗(On Diff #60204)

I'd sink this into WinEHPrepare unless you expect to use it elsewhere.

lib/Analysis/EHPersonalities.cpp
44 ↗(On Diff #60204)

Please change the return type to Value *.

lib/CodeGen/WinEHPrepare.cpp
681–683 ↗(On Diff #60204)

Ditto.

683–684 ↗(On Diff #60204)

You can Fold the assignment into the if.

testCodeGenWinEHwineh-promote-eh.ll
1 ↗(On Diff #60204)

-mtriple=-mtriple= looks funky

etienneb updated this revision to Diff 60403.Jun 10 2016, 2:13 PM
etienneb marked 4 inline comments as done.

comments

majnemer accepted this revision.Jun 10 2016, 2:25 PM
majnemer edited edge metadata.

LGTM

lib/CodeGen/WinEHPrepare.cpp
107–110 ↗(On Diff #60403)

I think it would make sense to sink this call into prepareExplicitEH.

This revision is now accepted and ready to land.Jun 10 2016, 2:25 PM
etienneb updated this revision to Diff 60409.Jun 10 2016, 2:55 PM
etienneb marked an inline comment as done.
etienneb edited edge metadata.

nits

rnk accepted this revision.Jun 16 2016, 8:41 AM
rnk edited edge metadata.

lgtm

Did this ever land? If not, is it blocked on anything?

Did this ever land? If not, is it blocked on anything?

It's not blocked by anything.
It was the next thing to land on the Exception/GS issue.

We decided to land step by step the patches related to the feature to see when it will explode.

For now, this patch is a noop since we do not produce stack-guard + exception at the same time.

Should we land it now?

rnk added a comment.Jun 30 2016, 8:30 AM

Let's go ahead and land this. Even though it's not functional, it documents our future plans in code.

etienneb updated this revision to Diff 62372.Jun 30 2016, 8:43 AM
etienneb edited edge metadata.

rebase (minor fixes)

etienneb closed this revision.Jun 30 2016, 8:43 AM
This revision was automatically updated to reflect the committed changes.
etienneb reopened this revision.Jun 30 2016, 11:02 AM

temporary reverted.

SEH4 needs to be fixed first.

[Diffusion] rL274251: revert http://reviews.llvm.org/D21101

This revision is now accepted and ready to land.Jun 30 2016, 11:02 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 7 2019, 6:56 AM
Herald added a subscriber: hiraditya. · View Herald Transcript