Re: [PATCH V2 16/32] x86/sgx: Support restricting of enclave page permissions

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Jarkko,

On 3/1/2022 6:05 PM, Jarkko Sakkinen wrote:
> On Tue, Mar 01, 2022 at 09:48:48AM -0800, Reinette Chatre wrote:
>> Hi Jarkko,
>>
>> On 3/1/2022 5:42 AM, Jarkko Sakkinen wrote:
>>>> With EACCEPTCOPY (kudos to Mark S. for reminding me of this version of
>>>> EACCEPT @ chat.enarx.dev) it is possible to make R and RX pages but
>>>> obviously new RX pages are now out of the picture:
>>>>
>>>>
>>>> 	/*
>>>> 	 * Adding a regular page that is architecturally allowed to only
>>>> 	 * be created with RW permissions.
>>>> 	 * TBD: Interface with user space policy to support max permissions
>>>> 	 * of RWX.
>>>> 	 */
>>>> 	prot = PROT_READ | PROT_WRITE;
>>>> 	encl_page->vm_run_prot_bits = calc_vm_prot_bits(prot, 0);
>>>> 	encl_page->vm_max_prot_bits = encl_page->vm_run_prot_bits;
>>>>
>>>> If that TBD is left out to the final version the page augmentation has a
>>>> risk of a API bottleneck, and that risk can realize then also in the page
>>>> permission ioctls.
>>>>
>>>> I.e. now any review comment is based on not fully known territory, we have
>>>> one known unknown, and some unknown unknowns from unpredictable effect to
>>>> future API changes.
>>
>> The plan to complete the "TBD" in the above snippet was to follow this work
>> with user policy integration at this location. On a high level the plan was
>> for this to look something like:
>>
>>
>>  	/*
>>  	 * Adding a regular page that is architecturally allowed to only
>>  	 * be created with RW permissions.
>>  	 * Interface with user space policy to support max permissions
>>  	 * of RWX.
>>  	 */
>>  	prot = PROT_READ | PROT_WRITE;
>>  	encl_page->vm_run_prot_bits = calc_vm_prot_bits(prot, 0);
>>
>>         if (user space policy allows RWX on dynamically added pages)
>> 	 	encl_page->vm_max_prot_bits = calc_vm_prot_bits(PROT_READ | PROT_WRITE | PROT_EXEC, 0);
>> 	else
>> 		encl_page->vm_max_prot_bits = calc_vm_prot_bits(PROT_READ | PROT_WRITE, 0);
>>
>> The work that follows this series aimed to do the integration with user
>> space policy.
> 
> What do you mean by "user space policy" anyway exactly? I'm sorry but I
> just don't fully understand this.

My apologies - I just assumed that you would need no reminder about this contentious
part of SGX history. Essentially it means that, yes, the kernel could theoretically
permit any kind of access to any file/page, but some accesses are known to generally
be a bad idea - like making memory executable as well as writable - and thus there
are additional checks based on what user space permits before the kernel allows
such accesses.

For example,
mm/mprotect.c:do_mprotect_pkey()->security_file_mprotect()

User policy and SGX has seen significant discussion. Some notable threads:
https://lore.kernel.org/linux-security-module/CALCETrXf8mSK45h7sTK5Wf+pXLVn=Bjsc_RLpgO-h-qdzBRo5Q@mail.gmail.com/
https://lore.kernel.org/linux-security-module/20190619222401.14942-1-sean.j.christopherson@intel.com/
 
> It's too big of a risk to accept this series without X taken care of. Patch
> series should neither have TODO nor TBD comments IMHO. I don't want to ack
> a series based on speculation what might happen in the future.

ok

> 
>>> I think the best way to move forward would be to do EAUG's explicitly with
>>> an ioctl that could also include secinfo for permissions. Then you can
>>> easily do the rest with EACCEPTCOPY inside the enclave.
>>
>> SGX_IOC_ENCLAVE_ADD_PAGES already exists and could possibly be used for
>> this purpose. It already includes SECINFO which may also be useful if
>> needing to later support EAUG of PT_SS* pages.
> 
> You could also simply add SGX_IOC_ENCLAVE_AUGMENT_PAGES and call it a day.

I could, yes.

> And if there is plan to extend SGX_IOC_ENCLAVE_ADD_PAGES what is this weird
> thing added to the #PF handler? Why is it added at all then?

I was just speculating in my response, there is no plan to extend
SGX_IOC_ENCLAVE_ADD_PAGES (that I am aware of).

>> How this could work is user space calls SGX_IOC_ENCLAVE_ADD_PAGES
>> after enclave initialization on any memory region within the enclave where
>> pages are planned to be added dynamically. This ioctl() calls EAUG to add the
>> new pages with RW permissions and their vm_max_prot_bits can be set to the
>> permissions found in the included SECINFO. This will support later EACCEPTCOPY
>> as well as SGX_IOC_ENCLAVE_RELAX_PERMISSIONS
> 
> I don't like this type of re-use of the existing API.

I could proceed with SGX_IOC_ENCLAVE_AUGMENT_PAGES if there is consensus after
considering the user policy question (above) and performance trade-off (more below).

> 
>> The big question is whether communicating user policy after enclave initialization
>> via the SECINFO within SGX_IOC_ENCLAVE_ADD_PAGES is acceptable to all? I would
>> appreciate a confirmation on this direction considering the significant history
>> behind this topic.
> 
> I have no idea because I don't know what is user space policy.

This discussion is about some enclave usages needing RWX permissions
on dynamically added enclave pages. RWX permissions on dynamically added pages is
not something that should blindly be allowed for all SGX enclaves but instead the user
needs to explicitly allow specific enclaves to have such ability. This is equivalent
to (but not the same as) what exists in Linux today with LSM. As seen in
mm/mprotect.c:do_mprotect_pkey()->security_file_mprotect() Linux is able to make
files and memory be both writable and executable, but it would only do so for those
files and memory that the LSM (which is how user policy is communicated, like SELinux)
indicates it is allowed, not blindly do so for all files and all memory.

>>> Putting EAUG to the #PF handler and implicitly call it just too flakky and
>>> hard to make deterministic for e.g. JIT compiler in our use case (not to
>>> mention that JIT is not possible at all because inability to do RX pages).

I understand how SGX_IOC_ENCLAVE_AUGMENT_PAGES can be more deterministic but from
what I understand it would have a performance impact since it would require all memory
that may be needed by the enclave be pre-allocated from outside the enclave and not
just dynamically allocated from within the enclave at the time it is needed.

Would such a performance impact be acceptable?

>> In this series this is indeed not possible because it lacks the user policy
>> integration. JIT will be possible after user policy integration.
> 
> Like this I don't what this series can be used in practice.
> 
> Majority of practical use cases for EDMM boil down to having a way to add
> new executable code (not just Enarx).
> 

Understood.

On 3/1/2022 8:03 PM, Jarkko Sakkinen wrote:
> I'd actually to leave out permission change madness completely out of this
> patch set, as we all know it is a grazy beast of microarchitecture. For
> user space having that is less critical than having executable pages.
> 
> Simply with EAUG/EACCEPTCOPY you can already populate enclave with any
> permissions you had in mind. Augmenting alone would be logically consistent
> patch set that is actually usable for many workloads.

Support for permission changes is required in order to support dynamically added
pages (EAUG pages) to be made executable. Yes, you could make
a dynamically added page have executable EPCM permissions using EACCEPTCOPY
but the kernel is still required to make the PTE executable.

> Now there is half-broken augmenting (this is even writtend down to the TBD
> comment) and complex code for EMODPR and EMODT that is usable only for
> kselftests and not much else before there is fully working augmenting.
> 
> This way we get actually sound patch set that is easy to review and apply
> to the mainline. It is also factors easier for you to iterate a smaller
> set of patches.
> 
> After this it is so much easier to start to look at remaining functionality,
> and at the same time augmenting part can be stress tested with real-world
> code and it will mature quickly.
> 
> This whole thing *really* needs a serious U-turn on how it is delivered to
> the upstream. Sometimes it is better just to admit that this didn't start
> with the right foot.

As mentioned above, from what I understand the support for (as you state) the
"majority of practical use cases" on dynamically added pages do require
supporting permission changes also. It thus seems to me that it would help
consuming this feature if dynamic addition of pages and permission changes
are presented together. The SGX2 functionality that remains after that is the
changing of page type, which forms part of the page removal flow. In this
regard I also find that presenting the page addition flow at the same time
as the page removal flow would make these features easier to consume. I
think supporting the addition of pages and leaving page removal to
"future work" would be similarly frustrating to consume.

Reinette