Re: [PATCH 05/25] x86/sgx: Introduce runtime protection bits

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Andy,

On 12/4/2021 9:56 AM, Andy Lutomirski wrote:
On 12/3/21 17:14, Reinette Chatre wrote:
Hi Andy,

On 12/3/2021 4:38 PM, Andy Lutomirski wrote:
On 12/3/21 14:12, Reinette Chatre wrote:
Hi Andy,

On 12/3/2021 11:28 AM, Andy Lutomirski wrote:
On 12/1/21 11:23, Reinette Chatre wrote:
Enclave creators declare their paging permission intent at the time
the pages are added to the enclave. These paging permissions are
vetted when pages are added to the enclave and stashed off
(in sgx_encl_page->vm_max_prot_bits) for later comparison with
enclave PTEs.


I'm a bit confused here. ENCLU[EMODPE] allows the enclave to change the EPCM permission bits however it likes with no oversight from the kernel.   So we end up with a whole bunch of permission masks:

Before jumping to the permission masks I would like to step back and just confirm the context. We need to consider the following three permissions:

EPCM permissions: the enclave page permissions maintained in the SGX hardware. The OS is constrained here in that it cannot query the current EPCM permissions. Even so, the OS needs to ensure PTEs are installed appropriately (we do not want a RW PTE for a read-only enclave page)

Why not?  What's wrong with an RW PTE for a read-only enclave page?

If you convince me that this is actually important, then I'll read all the stuff below.

Perhaps it is my misunderstanding/misinterpretation of the current implementation? From what I understand the current requirement, as enforced in the current mmap(), mprotect() as well as fault() hooks, is that mappings are required to have identical or weaker permission than the enclave permission.

The current implementation does require that, but for a perhaps counterintuitive reason.  If a SELinux-restricted (or similarly restricted) process that is *not* permitted to do JIT-like things loads an enclave, it's entirely okay for it to initialize RW enclave pages however it likes and it's entirely okay for it to initialize RX (or XO if that ever becomes a thing) enclave pages from appropriately files on disk.  But it's not okay for it to create RWX enclave pages or to initialize RX enclave pages from untrusted application memory. [0]

So we have a half-baked implementation right now: the permission to execute a page is decided based on secinfo (max permissions) when the enclave is set up, and it's enforced at the PTE level.  The PTE enforcement is because, on SGX2 hardware, the enclave can do EMODPE and bypass any supposed restrictions in the EPCM.

The only coupling between EPCM and PTE here is that the max_perm is initialized together with EPCM, but it didn't have to be that way.

An SGX2 implementation needs to be more fully baked, because in a dynamic environment enclaves need to be able to use EMODPE and actually end up with permissions that exceed the initial secinfo permissions.  So

Could you please elaborate why this is a requirement? In this implementation the secinfo of a page added before enclave initialization (via SGX_IOC_ENCLAVE_ADD_PAGES) would indicate the maximum permissions it may have during its lifetime. Pages needing to be writable and executable during their lifetime can be created with RWX secinfo and during the enclave runtime the pages could obtain all combinations of permissions: RWX, R, RW, RX. A page added with RW secinfo may have R or RW permissions during its lifetime but never RX or RWX.

So far our inquiries on whether this is acceptable has been positive and is also what Dave attempted to put a spotlight on in:
https://lore.kernel.org/lkml/94d8d631-5345-66c4-52a3-941e52500f84@xxxxxxxxx/

This above is specific to pages added before enclave initialization. In this implementation pages added after enclave initialization, those needing the ENCLS[EAUG] SGX2 instruction, are added with max permissions of RW so could only have R or RW permissions during their lifetime. This is an understood limitation and it is understood that integration with user policy is required to support these pages obtaining executable permission. The plan is to handle user policy integration in a series that follows this core SGX2 enabling.

it needs to be possible to make a page that starts out R (or RW or whatever) but nonetheless has max_perm=RWX so that the enclave can use a combination of EMODPE and (ioctl-based) EMODPR to do JIT.  So I think you should make it possible to set up pages like this, but I see no reason to couple the PTE and the EPCM permissions.


Could you please elaborate how you envision PTEs should be managed in this implementation?

As above: PTE permissions may not exceed max_perm, and EPCM is entirely separate except to the extent needed for ABI compatibility with SGX1 runtimes.

ok, so if I understand correctly you, since PTE permissions may not exceed max_perm and EPCM are separate, this seems to get back to your previous question of "What's wrong with an RW PTE for a read-only enclave page?"

This is indeed something that we could allow but not doing so (that is PTEs not exceeding EPCM permissions) would better support the SGX runtime. That is why I separated out the addition of the pfn_mkwrite() callback in the previous patch (04/25). Like in your example, there is a RW mapping of a read-only enclave page that first results in a RW PTE for the read-only enclave page. That would result in a #PF with the SGX flag set (0x8007). If the PTE matches the enclave permissions the page fault would have familiar 0x7 error code.

In either case user space would encounter a #PF so technically there is nothing "wrong" with allowing this - even so, as motivated in the previous patch: accurate exception information supports the SGX runtime, which is virtually always implemented inside a shared library, by providing accurate information in support of its management of the SGX enclave.


[0] I'm not sure anyone actually has a system set up like this or that the necessary LSM support is in the kernel.  But it's supposed to be possible without changing the ABI.


Reinette



[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux