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

[Jarkko Sakkinen]

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.

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.

> > 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.

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?

> 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.

> 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.

> > 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).
> 
> 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).

> Reinette

BR, Jarkko