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

[Haitao Huang]

On Fri, 04 Mar 2022 19:02:28 -0600, Jarkko Sakkinen <jarkko@xxxxxxxxxx>  
wrote:

> On Fri, Mar 04, 2022 at 09:51:22AM -0600, Haitao Huang wrote:
> > Hi Jarkko
> >
> > On Fri, 04 Mar 2022 02:30:22 -0600, Jarkko Sakkinen <jarkko@xxxxxxxxxx>
> > wrote:
> >
> > > On Thu, Mar 03, 2022 at 10:03:30PM -0600, Haitao Huang wrote:
> > > >
> > > > On Thu, 03 Mar 2022 17:18:33 -0600, Jarkko Sakkinen  
> > <jarkko@xxxxxxxxxx>
> > > > wrote:
> > > >
> > > > > On Thu, Mar 03, 2022 at 10:08:14AM -0600, Haitao Huang wrote:
> > > > > > Hi all,
> > > > > >
> > > > > > On Wed, 02 Mar 2022 16:57:45 -0600, Reinette Chatre
> > > > > > <reinette.chatre@xxxxxxxxx> wrote:
> > > > > >
> > > > > > > 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@xxxxxxxxxxxxxx/
> > > > > > >  
> > https://lore.kernel.org/linux-security-module/20190619222401.14942-1-sean.j.christopherson@xxxxxxxxx/
> > > > > > >
> > > > > > > > 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?
> > > > > > >
> > > > > >
> > > > > > User space won't always have enough info to decide whether the  
> > pages
> > > > > > to be
> > > > > > EAUG'd immediately. In some cases (shared libraries, JVM for
> > > > > > example) lots
> > > > > > of code/data pages can be mapped but never actually touched. One
> > > > > > enclave/process does not know if any other more important
> > > > > > enclave/process
> > > > > > would need the EPC.
> > > > > >
> > > > > > It should be for kernel to make the final decision as it has  
> > overall
> > > > > > picture
> > > > > > of the system EPC usage and availability.
> > > > >
> > > > > EAUG ioctl does not give better capabilities for user space to  
> > waste
> > > > > EPC given that EADD ioctl already exists, i.e. your argument is
> > > > logically
> > > > > incorrect.
> > > >
> > > > The point of adding EAUG is to allow more efficient use of EPC  
> > pages.
> > > > Without EAUG, enclaves have to EADD everything upfront into EPC,
> > > > consuming
> > > > predetermined number of EPC pages, some of which may not be used at  
> > all.
> > > > With EAUG, enclaves should be able to load minimal pages to get  
> > started,
> > > > pages added on #PF as they are actually accessed.
> > > >
> > > > Obviously as you pointed out, some usages make more sense to
> > > > pre-EAUG (EAUG
> > > > before #PF). But your proposal of supporting only pre-EAUG here
> > > > essentially
> > > > makes EAUG behave almost the same as EADD.  If the current
> > > > implementation
> > > > with EAUG on #PF can also use MAP_POPULATE for pre-EAUG (seems  
> > possible
> > > > based on Dave's comments), then it is flxible to cover all cases and
> > > > allow
> > > > kernel to optimize allocation of EPC pages.
> > >
> > > There is no even a working #PF based implementation in existance, and
> > > your
> > > argument has too many if's for my taste.
> >
> > 1) if you mean no user space is implementing this kind of solution, read
> > this section, otherwise, skip to 2) below which is only couple of  
> > sentences.
> >
> > If you are willing to look, there is already implementation in our SDK  
> > to do
> > heap and stack expansion on demand on #PF. Enclaves may not know  
> > heap/stack
> > size up front, we have implemented these features to make EPC usage more
> > efficient. I don't know why normal processes can add RAM on #PF, but
> > enclaves adding EPC on #PF becomes so unacceptable concept to you. And  
> > the
> > kernel does that for EPC swapping already when #PF happens on a swapped  
> > out
> > EPC page.
>
> In adds O(n) round-trips for a mmap() emulation, which can be done in  
> O(1)
> round-trips with a ioctl.
>
> > Our implementation has gone through several rounds, the latest is
> > here:https://github.com/intel/linux-sgx/tree/edmm_v2/sdk/emm. It was  
> > also
> > implemented in original OOT driver based SDK implementation. Customers  
> > are
> > using it and found them useful. I think this is a critical feature that  
> > many
> > other runtimes will also need.
>
> I'm not sure what the common sense argument here is.
My (wrong) assumption was that you are disabling EAUG on #PF totally, and  
all I was saying EAUG on #PF is critical for many usages and disabling it  
requires good justification.

But you are expecting an ioctl call for each #PF for those usages:  
https://lore.kernel.org/linux-sgx/YiK8NEnvgPerEdFB@xxxxxx/#t. IIUC, that's  
better than total disabling but less optimal. (I have not checked all call  
sequences in detail to be sure it would work for all our cases)


> > 2)
> > It's OK for you to request additional support for your usage and I  
> > agree it
> > is needed. But IMHO, totally getting rid of EAUG on #PF is bad and
> > unnecessary. Current implementation can be extended to support your  
> > usage.
> > What's the reason  you think MAP_POPULATE won't work for you?
>
> I do not recall taking stand on MAP_POPULATE.

Thanks for looking into that. Like I said, that should cover all usages.