On Tue, Sep 22, 2020 at 08:11:14AM -0700, Dave Hansen wrote: > > Enclave permissions can be dynamically modified by using ENCLS[EMODPE] > > I'm not sure this sentence matters. I'm not sure why I care what the > instruction is named that does this. But, it _sounds_ here like an > enclave can adjust its own permissions directly with ENCLS[EMODPE]. If there was no EMODPE, I would drop this patch from the patch set. It is the only reason I keep it. There are no other hard reasons to have this. > Now I'm confused. I actually don't think I have a strong understanding > of how an enclave actually gets loaded, how mmap() and mprotect() are > normally used and what this hook is intended to thwart. Enclave gets loaded with the ioctl API so ATM we rely only on DAC permissions. In future you might want to have LSM hooks to improve granularity in two points: 1. When pages are added using SGX_IOC_ENCLAVE ADD_PAGES. 2. When enclave is initialized using SGX_IOC_ENCLAVE_INIT In both you might want to make a policy decision based on origin and page permissions. If we do not limit mmap(), enclave could later on upgrade its permissions with EMODPE and do mmap(). No matter what kind of LSM hooks or whatever possible improvements are done later on to access control, they won't work unless we have this because the permissions could be different than the original. With this change you can still do EMODPE inside an enclave, but you don't gain anything with it because your max permissions are capped during the build time. I'm not sure what exactly from this is missing from the commit message but if you get this explanation maybe you can help me out with that. Thank you for the feedback. /Jarkko
