diff mbox series

[v20,22/28] x86/traps: Attempt to fixup exceptions in vDSO before signaling

Message ID 20190417103938.7762-23-jarkko.sakkinen@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series Intel SGX1 support | expand

Commit Message

Jarkko Sakkinen April 17, 2019, 10:39 a.m. UTC 
From: Sean Christopherson <sean.j.christopherson@intel.com>

vDSO functions can now leverage an exception fixup mechanism similar to
kernel exception fixup.  For vDSO exception fixup, the initial user is
Intel's Software Guard Extensions (SGX), which will wrap the low-level
transitions to/from the enclave, i.e. EENTER and ERESUME instructions,
in a vDSO function and leverage fixup to intercept exceptions that would
otherwise generate a signal.  This allows the vDSO wrapper to return the
fault information directly to its caller, obviating the need for SGX
applications and libraries to juggle signal handlers.

Attempt to fixup vDSO exceptions immediately prior to populating and
sending signal information.  Except for the delivery mechanism, an
exception in a vDSO function should be treated like any other exception
in userspace, e.g. any fault that is successfully handled by the kernel
should not be directly visible to userspace.

Although it's debatable whether or not all exceptions are of interest to
enclaves, defer to the vDSO fixup to decide whether to do fixup or
generate a signal.  Future users of vDSO fixup, if there ever are any,
will undoubtedly have different requirements than SGX enclaves, e.g. the
fixup vs. signal logic can be made function specific if/when necessary.

Suggested-by: Andy Lutomirski <luto@amacapital.net>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Josh Triplett <josh@joshtriplett.org>
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kernel/traps.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

Comments

Jarkko Sakkinen June 25, 2019, 3:43 p.m. UTC | #1 
On Wed, Apr 17, 2019 at 01:39:32PM +0300, Jarkko Sakkinen wrote:
> From: Sean Christopherson <sean.j.christopherson@intel.com>
> 
> vDSO functions can now leverage an exception fixup mechanism similar to
> kernel exception fixup.  For vDSO exception fixup, the initial user is
> Intel's Software Guard Extensions (SGX), which will wrap the low-level
> transitions to/from the enclave, i.e. EENTER and ERESUME instructions,
> in a vDSO function and leverage fixup to intercept exceptions that would
> otherwise generate a signal.  This allows the vDSO wrapper to return the
> fault information directly to its caller, obviating the need for SGX
> applications and libraries to juggle signal handlers.
> 
> Attempt to fixup vDSO exceptions immediately prior to populating and
> sending signal information.  Except for the delivery mechanism, an
> exception in a vDSO function should be treated like any other exception
> in userspace, e.g. any fault that is successfully handled by the kernel
> should not be directly visible to userspace.
> 
> Although it's debatable whether or not all exceptions are of interest to
> enclaves, defer to the vDSO fixup to decide whether to do fixup or
> generate a signal.  Future users of vDSO fixup, if there ever are any,
> will undoubtedly have different requirements than SGX enclaves, e.g. the
> fixup vs. signal logic can be made function specific if/when necessary.
> 
> Suggested-by: Andy Lutomirski <luto@amacapital.net>
> Cc: Andy Lutomirski <luto@amacapital.net>
> Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: Josh Triplett <josh@joshtriplett.org>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>

I went through the vDSO changes just to revisit couple of details that I
had forgotten. Sean, if you don't mind I'd squash this and prepending
patch.

Is there any obvious reason why #PF fixup is in its own patch and the
rest are collected to the same patch? I would not find it confusing if
there was one patch per exception but really don't get this division.

/Jarkko
Xing, Cedric June 27, 2019, 8:32 p.m. UTC | #2 
> From: linux-sgx-owner@vger.kernel.org [mailto:linux-sgx-
> owner@vger.kernel.org] On Behalf Of Jarkko Sakkinen
> Sent: Tuesday, June 25, 2019 8:44 AM
> 
> I went through the vDSO changes just to revisit couple of details that I
> had forgotten. Sean, if you don't mind I'd squash this and prepending
> patch.

Just a reminder that #DB/#BP shall be treated differently because they are used by debuggers. So instead of branching to the fixup address, the kernel shall just signal the process. 

> 
> Is there any obvious reason why #PF fixup is in its own patch and the
> rest are collected to the same patch? I would not find it confusing if
> there was one patch per exception but really don't get this division.
> 
> /Jarkko
Sean Christopherson July 11, 2019, 3:54 p.m. UTC | #3 
On Thu, Jun 27, 2019 at 01:32:58PM -0700, Xing, Cedric wrote:
> Just a reminder that #DB/#BP shall be treated differently because they are
> used by debuggers. So instead of branching to the fixup address, the kernel
> shall just signal the process. 

More importantly, doing fixup on #DB and #BP simply doesn't work.

On Tue, Apr 23, 2019 at 11:59:37AM -0700, Sean Christopherson wrote:
> On Mon, Apr 22, 2019 at 06:29:06PM -0700, Andy Lutomirski wrote:
> > What's not tested here is running this code with EFLAGS.TF set and
> > making sure that it unwinds correctly.  Also, Jarkko, unless I missed
> > something, the vDSO extable code likely has a bug.  If you run the
> > instruction right before ENCLU with EFLAGS.TF set, then do_debug()
> > will eat the SIGTRAP and skip to the exception handler.  Similarly, if
> > you put an instruction breakpoint on ENCLU, it'll get skipped.  Or is
> > the code actually correct and am I just remembering wrong?
>
> The code is indeed broken, and I don't see a sane way to make it not
> broken other than to never do vDSO fixup on #DB or #BP.  But that's
> probably the right thing to do anyways since an attached debugger is
> likely the intended recipient the 99.9999999% of the time.
>
> The crux of the matter is that it's impossible to identify whether or
> not a #DB/#BP originated from within an enclave, e.g. an INT3 in an
> enclave will look identical to an INT3 at the AEP.  Even if hardware
> provided a magic flag, #DB still has scenarios where the intended
> recipient is ambiguous, e.g. data breakpoint encountered in the enclave
> but on an address outside of the enclave, breakpoint encountered in the
> enclave and a code breakpoint on the AEP, etc...
Sean Christopherson July 11, 2019, 3:56 p.m. UTC | #4 
On Tue, Jun 25, 2019 at 06:43:41PM +0300, Jarkko Sakkinen wrote:
> Is there any obvious reason why #PF fixup is in its own patch and the
> rest are collected to the same patch? I would not find it confusing if
> there was one patch per exception but really don't get this division.

I split them due to SGX's funky #PF behavior with respect to th EPCM.
I'm ok with them being squashed.
Jarkko Sakkinen July 11, 2019, 5:52 p.m. UTC | #5 
On Thu, Jul 11, 2019 at 08:56:46AM -0700, Sean Christopherson wrote:
> On Tue, Jun 25, 2019 at 06:43:41PM +0300, Jarkko Sakkinen wrote:
> > Is there any obvious reason why #PF fixup is in its own patch and the
> > rest are collected to the same patch? I would not find it confusing if
> > there was one patch per exception but really don't get this division.
> 
> I split them due to SGX's funky #PF behavior with respect to th EPCM.
> I'm ok with them being squashed.

Right, better to add a note to the commit message if anything.

/Jarkko
Xing, Cedric July 11, 2019, 10:12 p.m. UTC | #6 
On 7/11/2019 8:54 AM, Sean Christopherson wrote:
> On Thu, Jun 27, 2019 at 01:32:58PM -0700, Xing, Cedric wrote:
>> Just a reminder that #DB/#BP shall be treated differently because they are
>> used by debuggers. So instead of branching to the fixup address, the kernel
>> shall just signal the process.
> 
> More importantly, doing fixup on #DB and #BP simply doesn't work.

What's really needed is a signal, as if the fixup entry didn't exist.

You don't have to care whether a debugger is attached or not.

> On Tue, Apr 23, 2019 at 11:59:37AM -0700, Sean Christopherson wrote:
>> On Mon, Apr 22, 2019 at 06:29:06PM -0700, Andy Lutomirski wrote:
>>> What's not tested here is running this code with EFLAGS.TF set and
>>> making sure that it unwinds correctly.  Also, Jarkko, unless I missed
>>> something, the vDSO extable code likely has a bug.  If you run the
>>> instruction right before ENCLU with EFLAGS.TF set, then do_debug()
>>> will eat the SIGTRAP and skip to the exception handler.  Similarly, if
>>> you put an instruction breakpoint on ENCLU, it'll get skipped.  Or is
>>> the code actually correct and am I just remembering wrong?
>>
>> The code is indeed broken, and I don't see a sane way to make it not
>> broken other than to never do vDSO fixup on #DB or #BP.  But that's
>> probably the right thing to do anyways since an attached debugger is
>> likely the intended recipient the 99.9999999% of the time.
>>
>> The crux of the matter is that it's impossible to identify whether or
>> not a #DB/#BP originated from within an enclave, e.g. an INT3 in an
>> enclave will look identical to an INT3 at the AEP.  Even if hardware
>> provided a magic flag, #DB still has scenarios where the intended
>> recipient is ambiguous, e.g. data breakpoint encountered in the enclave
>> but on an address outside of the enclave, breakpoint encountered in the
>> enclave and a code breakpoint on the AEP, etc...
diff mbox series

Patch

diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index d26f9e9c3d83..02eda456c119 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -61,6 +61,7 @@ 
 #include <asm/mpx.h>
 #include <asm/vm86.h>
 #include <asm/umip.h>
+#include <asm/vdso.h>
 
 #ifdef CONFIG_X86_64
 #include <asm/x86_init.h>
@@ -210,6 +211,9 @@  do_trap_no_signal(struct task_struct *tsk, int trapnr, const char *str,
 		tsk->thread.error_code = error_code;
 		tsk->thread.trap_nr = trapnr;
 		die(str, regs, error_code);
+	} else {
+		if (fixup_vdso_exception(regs, trapnr, error_code, 0))
+			return 0;
 	}
 
 	/*
@@ -561,6 +565,9 @@  do_general_protection(struct pt_regs *regs, long error_code)
 		return;
 	}
 
+	if (fixup_vdso_exception(regs, X86_TRAP_GP, error_code, 0))
+		return;
+
 	tsk->thread.error_code = error_code;
 	tsk->thread.trap_nr = X86_TRAP_GP;
 
@@ -775,6 +782,10 @@  dotraplinkage void do_debug(struct pt_regs *regs, long error_code)
 							SIGTRAP) == NOTIFY_STOP)
 		goto exit;
 
+	if (user_mode(regs) &&
+	    fixup_vdso_exception(regs, X86_TRAP_DB, error_code, 0))
+		goto exit;
+
 	/*
 	 * Let others (NMI) know that the debug stack is in use
 	 * as we may switch to the interrupt stack.
@@ -855,6 +866,9 @@  static void math_error(struct pt_regs *regs, int error_code, int trapnr)
 	if (!si_code)
 		return;
 
+	if (fixup_vdso_exception(regs, trapnr, error_code, 0))
+		return;
+
 	force_sig_fault(SIGFPE, si_code,
 			(void __user *)uprobe_get_trap_addr(regs), task);
 }