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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Yh7Q5fbOtr+6YWaS@iki.fi>
Date:   Wed, 2 Mar 2022 03:05:25 +0100
From:   Jarkko Sakkinen <jarkko@...nel.org>
To:     Reinette Chatre <reinette.chatre@...el.com>
Cc:     Dave Hansen <dave.hansen@...el.com>,
        "Dhanraj, Vijay" <vijay.dhanraj@...el.com>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "bp@...en8.de" <bp@...en8.de>,
        "Lutomirski, Andy" <luto@...nel.org>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "linux-sgx@...r.kernel.org" <linux-sgx@...r.kernel.org>,
        "x86@...nel.org" <x86@...nel.org>,
        "Christopherson,, Sean" <seanjc@...gle.com>,
        "Huang, Kai" <kai.huang@...el.com>,
        "Zhang, Cathy" <cathy.zhang@...el.com>,
        "Xing, Cedric" <cedric.xing@...el.com>,
        "Huang, Haitao" <haitao.huang@...el.com>,
        "Shanahan, Mark" <mark.shanahan@...el.com>,
        "hpa@...or.com" <hpa@...or.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH V2 16/32] x86/sgx: Support restricting of enclave page
 permissions

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