On Thu, Feb 20, 2020 at 10:33:36AM -0800, Jordan Hand wrote: > On 2/20/20 10:13 AM, Sean Christopherson wrote: > > There are essentially two paths we can take: > > > > 1) Exempt EPC pages from RIE during mmap()/mprotect(), i.e. don't add > > PROT_EXEC for enclaves. > > > > 2) Punt the issue to userspace. > > > > Option (1) is desirable in some ways: > > > > - Enclaves will get an executable stack if and only if the loader/creator > > intentionally configures it to have an executable stack. > > > > - Separates enclaves from the personality of the loader. > > > > - Userspace doesn't have to do anything for the common case of not > > wanting an executable stack for its enclaves. > > > > The big down side to (1) is that it'd require an ugly hook in architecture > > agnostic code. And arguably, it reduces the overall security of the > > platform (more below). > > > > For (2), userspace has a few options: > > > > a) Tell the linker the enclave loader doesn't need RIE, either via a .note > > in assembly files or via the global "-z noexecstack" flag. > > > > b) Spawn a separate process to run/map the enclave if the enclave loader > > needs RIE. > > > > c) Require enclaves to allow PROT_EXEC on all pages. Note, this is an > > absolutely terrible idea and only included for completeness. > > > > As shown by the lack of a mmap()/mprotect() hook in this series to squash > > RIE, we chose option (2). Given that enclave loaders are not legacy code > > and hopefully following decent coding practices, option (2a) should suffice > > for all loaders. The security benefit mentioned above is that forcing > > enclave loaders to squash RIE eliminates an exectuable stack as an attack > > vector on the loader. > > I see your point and I do agree that there are security benefits to (2a) > and I think we could do that for our loader. That said, it does concern > me that this breaks perfectly valid userspace behavior. If a userspace > process decides to use RIE, I don't know that the SGX driver should > disobey that decision. > > So option (3) would be to just honor RIE for enclave pages and when page > permissions are set to PROT_READ in sgx_encl_page_alloc and RIE is set, > also add PROT_EXEC. Ah, right, IIRC that idea also came up in our internal discussions. Note, SGX would need to implement this option by checking for RIE in sgx_encl_may_map(), as the process that built the enclave may not be the same process that is running the enclave. > I understand your concerns that this using RIE is bad security practice > and I'm not convinced that (3) is the way to go, but from a philosophy > perspective I don't know that the kernel should be in the business of > stopping userspace from doing valid things. > > If option (3) can't/shouldn't be done for some reason, option (1) at > least keeps from breaking expected userspace behavior. But I do agree > that (1) is ugly to implement. My biggest concern for allowing PROT_EXEC if RIE is that it would result in #PF(SGX) (#GP on Skylake) due to an EPCM violation if the enclave actually tried to execute from such a page. This isn't a problem for the kernel as the fault will be reported cleanly through the vDSO (or get delivered as a SIGSEGV if the enclave isn't entered through the vDSO), but it's a bit weird for userspace as userspace will see the #PF(SGX) and likely assume the EPC was lost, e.g. silently restart the enclave instead of logging an error that the enclave is broken.
