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

[Jarkko Sakkinen]

On Tue, Mar 01, 2022 at 02:26:48PM +0100, Jarkko Sakkinen wrote:
> On Mon, Feb 28, 2022 at 07:16:22AM -0800, Dave Hansen wrote:
> > On 2/28/22 04:24, Jarkko Sakkinen wrote:
> > >> Regarding the recent update of splitting the page permissions change
> > >> request into two IOCTLS (RELAX and RESTRICT), can we combine them into
> > >> one? That is, revert to how it was done in the v1 version?
> > > They are logically separate complex functionalities:
> > > 
> > > 1. "restrict" calls EMODPR and requires EACCEPT
> > > 2. "relax" increases permissions up to vetted ("EADD") and could be
> > >     combined with EMODPE called inside enclave.
> > 
> > It would be great to have a _slightly_ better justification than that.
> > Existing permission interfaces like chmod or mprotect() don't have this
> > asymmetry.
> > 
> > I think you're saying that the underlying hardware implementation is
> > asymmetric, so the interface should be too.  I don't find that argument
> > very convincing.  If the hardware interface is arcane and we can make it
> > look more sane in the ioctl() layer, we should that, asymmetry or not.
> 
> That is my argument, yes.
> 
> > If we can't make it any more sane, let's say why the ioctl() must or
> > should be asymmetric.
> 
> Perhaps underling this asymmetry in kdoc would be enough.
> 
> > The SGX2 page permission mechanism is horribly counter intuitive.
> > *Everybody* that looks at it thinks that it's wrong.  That means that we
> > have a lot of work ahead of us to explain the interfaces that get
> > layered on top.
> 
> I fully agree on this :-)
> 
> 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.

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.

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

BR, Jarkko