| Message ID | 20200617220844.57423-4-jarkko.sakkinen@linux.intel.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | Intel SGX foundations | expand |
On Thu, Jun 18, 2020 at 01:08:25AM +0300, Jarkko Sakkinen wrote: > From: Sean Christopherson <sean.j.christopherson@intel.com> > > Include SGX bit to the PF error codes and throw SIGSEGV with PF_SGX when > a #PF with SGX set happens. > > CPU throws a #PF with the SGX bit in the event of Enclave Page Cache Map ^ set > (EPCM) conflict. The EPCM is a CPU-internal table, which describes the > properties for a enclave page. Enclaves are measured and signed software > entities, which SGX hosts. [1] > > Although the primary purpose of the EPCM conflict checks is to prevent > malicious accesses to an enclave, an illegit access can happen also for > legit reasons. > > All SGX reserved memory, including EPCM is encrypted with a transient > key that does not survive from the power transition. Throwing a SIGSEGV > allows user space software react when this happens (e.g. rec-create the ^ to recreate > enclave, which was invalidated). > > [1] Intel SDM: 36.5.1 Enclave Page Cache Map (EPCM) > > Acked-by: Jethro Beekman <jethro@fortanix.com> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > --- > arch/x86/include/asm/traps.h | 1 + > arch/x86/mm/fault.c | 13 +++++++++++++ > 2 files changed, 14 insertions(+) > > diff --git a/arch/x86/include/asm/traps.h b/arch/x86/include/asm/traps.h > index 714b1a30e7b0..ee3617b67bf4 100644 > --- a/arch/x86/include/asm/traps.h > +++ b/arch/x86/include/asm/traps.h > @@ -58,5 +58,6 @@ enum x86_pf_error_code { > X86_PF_RSVD = 1 << 3, > X86_PF_INSTR = 1 << 4, > X86_PF_PK = 1 << 5, > + X86_PF_SGX = 1 << 15, Needs to be added to the doc above it. > #endif /* _ASM_X86_TRAPS_H */ > diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c > index 66be9bd60307..25d48aae36c1 100644 > --- a/arch/x86/mm/fault.c > +++ b/arch/x86/mm/fault.c > @@ -1055,6 +1055,19 @@ access_error(unsigned long error_code, struct vm_area_struct *vma) > if (error_code & X86_PF_PK) > return 1; > > + /* > + * Access is blocked by the Enclave Page Cache Map (EPCM), i.e. the > + * access is allowed by the PTE but not the EPCM. This usually happens > + * when the EPCM is yanked out from under us, e.g. by hardware after a > + * suspend/resume cycle. In any case, software, i.e. the kernel, can't > + * fix the source of the fault as the EPCM can't be directly modified by > + * software. Handle the fault as an access error in order to signal > + * userspace so that userspace can rebuild their enclave(s), even though > + * userspace may not have actually violated access permissions. > + */ Lemme check whether I understand this correctly: userspace must check whether the SIGSEGV is generated on an access to an enclave page? Also, do I see it correctly that when this happens, dmesg will have printk("%s%s[%d]: segfault at %lx ip %px sp %px error %lx", due to: if (likely(show_unhandled_signals)) show_signal_msg(regs, error_code, address, tsk); which does: if (!unhandled_signal(tsk, SIGSEGV)) return; or is the task expected to register a SIGSEGV handler so that the segfault doesn't land in dmesg? If so, are we documenting this? If not, then we should not issue any "segfault" messages to dmesg because that would be wrong. Or maybe I'm not seeing it right but I don't have the hardware to test this out... Thx.
On Thu, Jun 25, 2020 at 10:59:31AM +0200, Borislav Petkov wrote: > On Thu, Jun 18, 2020 at 01:08:25AM +0300, Jarkko Sakkinen wrote: > > diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c > > index 66be9bd60307..25d48aae36c1 100644 > > --- a/arch/x86/mm/fault.c > > +++ b/arch/x86/mm/fault.c > > @@ -1055,6 +1055,19 @@ access_error(unsigned long error_code, struct vm_area_struct *vma) > > if (error_code & X86_PF_PK) > > return 1; > > > > + /* > > + * Access is blocked by the Enclave Page Cache Map (EPCM), i.e. the > > + * access is allowed by the PTE but not the EPCM. This usually happens > > + * when the EPCM is yanked out from under us, e.g. by hardware after a > > + * suspend/resume cycle. In any case, software, i.e. the kernel, can't > > + * fix the source of the fault as the EPCM can't be directly modified by > > + * software. Handle the fault as an access error in order to signal > > + * userspace so that userspace can rebuild their enclave(s), even though > > + * userspace may not have actually violated access permissions. > > + */ > > Lemme check whether I understand this correctly: userspace must check > whether the SIGSEGV is generated on an access to an enclave page? Sort of. Technically it's that's an accurate statement, but practically speaking userspace can only access enclave pages when it is executing in the enclave, and exceptions in enclaves have unique behavior. Exceptions in enclaves essentially bounce through a userspace-software-defined location prior to being delivered to the kernel. The trampoline is done by the CPU so that the CPU can scrub the GPRs, XSAVE state, etc... and hide the true RIP of the exception. The pre-exception enclave state is saved into protected memory and restored when userspace resumes the enclave. Enterring or resuming an enclave can only be done through dedicted ENCLU instructions, so really it ends up being that the SIGSEGV handler needs to check the IP that "caused" the fault, which is actually the IP of the trampoline. But, that's only the first half of the story... > Also, do I see it correctly that when this happens, dmesg will have > > printk("%s%s[%d]: segfault at %lx ip %px sp %px error %lx", > > due to: > > if (likely(show_unhandled_signals)) > show_signal_msg(regs, error_code, address, tsk); > > which does: > > if (!unhandled_signal(tsk, SIGSEGV)) > return; > > or is the task expected to register a SIGSEGV handler so that the > segfault doesn't land in dmesg? Yes, without extra help, any task running an enclave is expected to register a SIGSEGV handler so that the task can restart the enclave if the EPC is "lost". However, building and running enclaves is complex, and the vast majority of SGX enabled applications are expected to leverage a library of one kind or another to hand the bulk of the gory details. But, signal handling in libraries is a mess, e.g. requires filtering/forwarding, resignaling, etc... To that end, in v14 of this patch[1], Andy Lutomirski came up with the idea of adding a vDSO function to provide the low level enclave EENTER/ERESUME and trampoline, and then teaching the kernel to do exception fixup on the relevant instructions in the vDSO. The vDSO's exception fixup then returns to normal userspace, with a (technically optional) struct holding the details of the exception. That allows for synchronous delivery of exceptions in enclaves, obviates the need for userspace to regsiter a SIGSEGV handler, and also means the SIGSEGV will never show up in dmesg so long as userspace is using the vDSO. The kernel still supports direct EENTER/ERESUME, but AFAIK everyone is moving (or has moved) to the vDSO interface. The vDSO stuff is in patches 15-18 of this series. There's a gigantic thread on all the alternatives that were considered[2]. [1] https://lkml.kernel.org/r/CALCETrXByb2UVuZ6AXUeOd8y90NAikbZuvdN3wf_TjHZ+CxNhA@mail.gmail.com [2] https://lkml.kernel.org/r/CALCETrWdpoDkbZjkucKL91GWpDPG9p=VqYrULade2pFDR7S=GQ@mail.gmail.com > > If so, are we documenting this? > > If not, then we should not issue any "segfault" messages to dmesg > because that would be wrong. > > Or maybe I'm not seeing it right but I don't have the hardware to test > this out... > > Thx. > > -- > Regards/Gruss, > Boris. > > https://people.kernel.org/tglx/notes-about-netiquette
On Thu, Jun 25, 2020 at 08:34:31AM -0700, Sean Christopherson wrote: > However, building and running enclaves is complex, and the vast majority of > SGX enabled applications are expected to leverage a library of one kind or > another to hand the bulk of the gory details. I gotta say this rings a bell: dhansen alluded on IRC to the jumping through hoops one needs to do in order to run SGX enclaves. ... > The vDSO stuff is in patches 15-18 of this series. > > There's a gigantic thread on all the alternatives that were considered[2]. > > [1] https://lkml.kernel.org/r/CALCETrXByb2UVuZ6AXUeOd8y90NAikbZuvdN3wf_TjHZ+CxNhA@mail.gmail.com > [2] https://lkml.kernel.org/r/CALCETrWdpoDkbZjkucKL91GWpDPG9p=VqYrULade2pFDR7S=GQ@mail.gmail.com Yeah, that makes it very clear. Thanks a lot for taking the time and writing it down. I've snipped it for brevity but it is very useful! Thx!
On Thu, Jun 25, 2020 at 10:59:31AM +0200, Borislav Petkov wrote: > On Thu, Jun 18, 2020 at 01:08:25AM +0300, Jarkko Sakkinen wrote: > > From: Sean Christopherson <sean.j.christopherson@intel.com> > > > > Include SGX bit to the PF error codes and throw SIGSEGV with PF_SGX when > > a #PF with SGX set happens. > > > > CPU throws a #PF with the SGX bit in the event of Enclave Page Cache Map > ^ > set > > > (EPCM) conflict. The EPCM is a CPU-internal table, which describes the > > properties for a enclave page. Enclaves are measured and signed software > > entities, which SGX hosts. [1] > > > > Although the primary purpose of the EPCM conflict checks is to prevent > > malicious accesses to an enclave, an illegit access can happen also for > > legit reasons. > > > > All SGX reserved memory, including EPCM is encrypted with a transient > > key that does not survive from the power transition. Throwing a SIGSEGV > > allows user space software react when this happens (e.g. rec-create the > ^ > to recreate > > > enclave, which was invalidated). > > > > [1] Intel SDM: 36.5.1 Enclave Page Cache Map (EPCM) > > > > Acked-by: Jethro Beekman <jethro@fortanix.com> > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > --- > > arch/x86/include/asm/traps.h | 1 + > > arch/x86/mm/fault.c | 13 +++++++++++++ > > 2 files changed, 14 insertions(+) > > > > diff --git a/arch/x86/include/asm/traps.h b/arch/x86/include/asm/traps.h > > index 714b1a30e7b0..ee3617b67bf4 100644 > > --- a/arch/x86/include/asm/traps.h > > +++ b/arch/x86/include/asm/traps.h > > @@ -58,5 +58,6 @@ enum x86_pf_error_code { > > X86_PF_RSVD = 1 << 3, > > X86_PF_INSTR = 1 << 4, > > X86_PF_PK = 1 << 5, > > + X86_PF_SGX = 1 << 15, > > Needs to be added to the doc above it. I ended up with: * bit 5 == 1: protection keys block access * bit 6 == 1: inside SGX enclave */ /Jarkko
On Thu, Jun 25, 2020 at 11:52:11PM +0300, Jarkko Sakkinen wrote: > I ended up with: > > * bit 5 == 1: protection keys block access > * bit 6 == 1: inside SGX enclave You mean bit 15.
On Thu, Jun 25, 2020 at 11:11:03PM +0200, Borislav Petkov wrote: > On Thu, Jun 25, 2020 at 11:52:11PM +0300, Jarkko Sakkinen wrote: > > I ended up with: > > > > * bit 5 == 1: protection keys block access > > * bit 6 == 1: inside SGX enclave > > You mean bit 15. Duh, did this last thing before falling into sleep last night :-/ Yes, it should be 15. I'll also rephrase the text to "inside an SGX enclave". /Jarkko
diff --git a/arch/x86/include/asm/traps.h b/arch/x86/include/asm/traps.h index 714b1a30e7b0..ee3617b67bf4 100644 --- a/arch/x86/include/asm/traps.h +++ b/arch/x86/include/asm/traps.h @@ -58,5 +58,6 @@ enum x86_pf_error_code { X86_PF_RSVD = 1 << 3, X86_PF_INSTR = 1 << 4, X86_PF_PK = 1 << 5, + X86_PF_SGX = 1 << 15, }; #endif /* _ASM_X86_TRAPS_H */ diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c index 66be9bd60307..25d48aae36c1 100644 --- a/arch/x86/mm/fault.c +++ b/arch/x86/mm/fault.c @@ -1055,6 +1055,19 @@ access_error(unsigned long error_code, struct vm_area_struct *vma) if (error_code & X86_PF_PK) return 1; + /* + * Access is blocked by the Enclave Page Cache Map (EPCM), i.e. the + * access is allowed by the PTE but not the EPCM. This usually happens + * when the EPCM is yanked out from under us, e.g. by hardware after a + * suspend/resume cycle. In any case, software, i.e. the kernel, can't + * fix the source of the fault as the EPCM can't be directly modified by + * software. Handle the fault as an access error in order to signal + * userspace so that userspace can rebuild their enclave(s), even though + * userspace may not have actually violated access permissions. + */ + if (unlikely(error_code & X86_PF_SGX)) + return 1; + /* * Make sure to check the VMA so that we do not perform * faults just to hit a X86_PF_PK as soon as we fill in a