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.
