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

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Haitao,

On 3/3/2022 8:08 AM, Haitao Huang wrote:
> On Wed, 02 Mar 2022 16:57:45 -0600, Reinette Chatre <reinette.chatre@intel.com> wrote:
>> On 3/1/2022 6:05 PM, Jarkko Sakkinen wrote:
>>> On Tue, Mar 01, 2022 at 09:48:48AM -0800, Reinette Chatre wrote:
>>>> On 3/1/2022 5:42 AM, Jarkko Sakkinen wrote:

...

>>>>> 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.
> 
> User space can provide a hint (similar to MAP_POPULATE) to kernel that the mmap'd area will soon be needed and kernel should EAUG as soon as it sees fit based on current system usage. Or kernel implement some policy to avoid #PF triggered by EACCEPT, for example, if the system has ton of free EPC relative to the requested by mmap at the time.
> 

mmap(...,...,...,MAP_POPULATE,...,...) would be most fitting and
ideal since it would enable user space to indicate that the pages would
be needed soon and the kernel can then prefault the pages. This is already
desirable in the current implementation to avoid the first page fault on
pages added via SGX_IOC_ENCLAVE_ADD_PAGES.

Unfortunately MAP_POPULATE is not supported by SGX VMAs because of their
VM_IO and VM_PFNMAP flags. When VMAs with such flags obtain this capability
then I believe that SGX would benefit.

Reinette