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
