Message-ID: <97565fed-dc67-bab1-28d4-c40201c9f055@intel.com>
Date:   Fri, 11 Mar 2022 09:53:29 -0800
From:   Reinette Chatre <reinette.chatre@...el.com>
To:     Jarkko Sakkinen <jarkko@...nel.org>,
        Haitao Huang <haitao.huang@...ux.intel.com>
CC:     "Dhanraj, Vijay" <vijay.dhanraj@...el.com>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "bp@...en8.de" <bp@...en8.de>,
        "Lutomirski, Andy" <luto@...nel.org>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "linux-sgx@...r.kernel.org" <linux-sgx@...r.kernel.org>,
        "x86@...nel.org" <x86@...nel.org>,
        "Christopherson,, Sean" <seanjc@...gle.com>,
        "Huang, Kai" <kai.huang@...el.com>,
        "Zhang, Cathy" <cathy.zhang@...el.com>,
        "Xing, Cedric" <cedric.xing@...el.com>,
        "Huang, Haitao" <haitao.huang@...el.com>,
        "Shanahan, Mark" <mark.shanahan@...el.com>,
        "hpa@...or.com" <hpa@...or.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH V2 16/32] x86/sgx: Support restricting of enclave page
 permissions

Hi Jarkko,

On 3/11/2022 4:16 AM, Jarkko Sakkinen wrote:
> On Fri, Mar 11, 2022 at 02:10:24PM +0200, Jarkko Sakkinen wrote:
>> On Thu, Mar 10, 2022 at 12:33:20PM -0600, Haitao Huang wrote:
>>> Hi Jarkko
>>>
>>> I have some trouble understanding the sequences below.
>>>
>>> On Thu, 10 Mar 2022 00:10:48 -0600, Jarkko Sakkinen <jarkko@...nel.org>
>>> wrote:
>>>
>>>> On Wed, Feb 23, 2022 at 07:21:50PM +0000, Dhanraj, Vijay wrote:
>>>>> Hi All,
>>>>>
>>>>> Regarding the recent update of splitting the page permissions change
>>>>> request into two IOCTLS (RELAX and RESTRICT), can we combine them into
>>>>> one? That is, revert to how it was done in the v1 version?
>>>>>
>>>>> Why? Currently in Gramine (a library OS for unmodified applications,
>>>>> https://gramineproject.io/) with the new proposed change, one needs to
>>>>> store the page permission for each page or range of pages. And for every
>>>>> request of `mmap` or `mprotect`, Gramine would have to do a lookup
>>>>> of the
>>>>> page permissions for the request range and then call the respective
>>>>> IOCTL
>>>>> either RESTRICT or RELAX. This seems a little overwhelming.
>>>>>
>>>>> Request: Instead, can we do `MODPE`,  call `RESTRICT` IOCTL, and then do
>>>>> an `EACCEPT` irrespective of RELAX or RESTRICT page permission request?
>>>>> With this approach, we can avoid storing  page permissions and simplify
>>>>> the implementation.
>>>>>
>>>>> I understand RESTRICT IOCTL would do a `MODPR` and trigger `ETRACK`
>>>>> flows
>>>>> to do TLB shootdowns which might not be needed for RELAX IOCTL but I am
>>>>> not sure what will be the performance impact. Is there any data point to
>>>>> see the performance impact?
>>>>>
>>>>> Thanks,
>>>>> -Vijay
>>>>
>>>> This should get better in the next versuin. "relax" is gone. And for
>>>> dynamic EAUG'd pages only VMA and EPCM permissions matter, i.e.
>>>> internal vm_max_prot_bits is set to RWX.
>>>>
>>>> I patched the existing series eno
>>>>
>>>> For Enarx I'm using the following patterns.
>>>>
>>>> Shim mmap() handler:
>>>> 1. Ask host for mmap() syscall.
>>>> 2. Construct secinfo matching the protection bits.
>>>> 3. For each page in the address range: EACCEPTCOPY with a
>>>>    zero page.
>>>
>>> For EACCEPTCOPY to work, I believe PTE.RW is required for the target page.
>>> So this only works for mmap(..., RW) or mmap(...,RWX).
>>
>> I use it only with EAUG.
>>
>>> So that gives you pages with RW/RWX.
>>>
>>> To change permissions of any of those pages from RW/RWX to R/RX , you need
>>> call ENCLAVE_RESTRICT_PERMISSIONS ioctl with R or with PROT_NONE. you can't
>>> just do EMODPE.
>>>
>>> so for RW->R, you either:
>>>
>>> 1)EMODPR(EPCM.NONE)
>>> 2)EACCEPT(EPCM.NONE)
>>> 3)EMODPE(R) -- not sure this would work as spec says EMODPE requires "Read
>>> access permitted by enclave"
>>>
>>> or:
>>>
>>> 1)EMODPR(EPCM.PROT_R)
>>> 2)EACCEPT(EPCM.PROT_R)
>>
>> I checked from SDM and you're correct.
>>
>> Then the appropriate thing is to reset to R.
>>
>>>> Shim mprotect() handler:
>>>> 1. Ask host for mprotect() syscall.
>>>> 2. For each page in the address range: EACCEPT with PROT_NONE
>>>>    secinfo and EMODPE with the secinfo having the prot bits.
>>>
>>> EACCEPT requires PTE.R. And EAUG'd pages will always initialized with
>>> EPCM.RW,
>>> so EACCEPT(EPCM.PROT_NONE) will fail with SGX_PAGE_ATTRIBUTES_MISMATCH.
>>
>> Ditto.
>>
>>>> Backend mprotect() handler:
>>>> 1. Invoke ENCLAVE_RESTRICT_PERMISSIONS ioctl for the address
>>>>    range with PROT_NONE.
>>>> 2. Invoke real mprotect() syscall.
>>>>
>>> Note #1 can only be done after EACCEPT. MODPR is not allowed for pending
>>> pages.
>>
>> Yes, and that's what I'm doing. After that shim does EACCEPT's in a loop.
>>
>> Reinette, the ioctl should already check that either R or W is set in
>> secinfo and return -EACCES.
>>
>> I.e.
>>
>> (* Check for misconfigured SECINFO flags*)
>> IF ( (SCRATCH_SECINFO reserved fields are not zero ) or
>> (SCRATCH_SECINFO.FLAGS.R is 0 and SCRATCH_SECINFO.FLAGS.W is not 0) )
>> THEN #GP(0); FI;
>>
>> I was testing this and wondering why my enclave #GP's, and then I checked
>> SDM after reading Haitao's response. So clearly check in kernel side is
>> needed.

I do not believe that you encountered the #GP documented above because that
check is already present in the current implementation of
SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS:

sgx_ioc_enclave_restrict_permissions()->sgx_perm_from_user_secinfo():
	if ((perm & SGX_SECINFO_W) && !(perm & SGX_SECINFO_R))
		return -EINVAL;

It does return EINVAL which is the catch-all error code used to represent
invalid input from user space. I am not convinced that EACCES should be used
instead though, EACCES means "Permission denied", which is not the case here.
The case here is just an invalid request.

It currently does not prevent the user from setting PROT_NONE though, which
EMODPR does seem to allow.

I saw Haitao's note that EMODPE requires "Read access permitted by enclave".
This motivates that EMODPR->PROT_NONE should not be allowed since it would
not be possible to relax permissions (run EMODPE) after that. Even so, I
also found in the SDM that EACCEPT has the note "Read access permitted
by enclave". That seems to indicate that EMODPR->PROT_NONE is not practical
from that perspective either since the enclave will not be able to
EACCEPT the change. Does that match your understanding?

I will add the check for R in SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS at least.

> I would consider also adding such check "add pages". It's our least common
> denominator.
> 
> If we can assume that at least R is there for every enclave page, then it
> gives invariant that enables EMODPR with R all the time.

Adding pages without permissions to an enclave does not seem practical. I
do not know if there are such usages. I can add this as a separate change for
consideration.

Reinette

Powered by blists - more mailing lists