Date:   Fri, 3 Dec 2021 10:18:33 -0800
From:   Reinette Chatre <reinette.chatre@...el.com>
To:     Dave Hansen <dave.hansen@...el.com>, <dave.hansen@...ux.intel.com>,
        <jarkko@...nel.org>, <tglx@...utronix.de>, <bp@...en8.de>,
        <luto@...nel.org>, <mingo@...hat.com>, <linux-sgx@...r.kernel.org>,
        <x86@...nel.org>
CC:     <seanjc@...gle.com>, <kai.huang@...el.com>,
        <cathy.zhang@...el.com>, <cedric.xing@...el.com>,
        <haitao.huang@...el.com>, <mark.shanahan@...el.com>,
        <hpa@...or.com>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 10/25] x86/sgx: Support enclave page permission changes

Hi Dave,

On 12/2/2021 4:32 PM, Dave Hansen wrote:
> On 12/1/21 11:23 AM, Reinette Chatre wrote:
>> Whether enclave page permissions are restricted or extended it
>> is necessary to ensure that the page table entries and enclave page
>> permissions are in sync. Introduce a new ioctl,
> 
> These should be "ioctl()".

Will fix.

> 
>> SGX_IOC_PAGE_MODP, to support enclave page permission changes. Since
>> the OS has no insight in how permissions may have been extended from
>> within the enclave all page permission requests are treated as
>> permission restrictions.
> I'm trying to wrap my head around this a bit.  If this is only for
> restricting permissions, should we be reflecting that in the naming?
> SGX_IOC_PAGE_RESTRICT_PERM, perhaps?  Wouldn't that be more direct than
> saying, "here's a permission change ioctl(), but it doesn't arbitrarily
> change things, it treats all changes as restrictions"?

The ioctl is named from the user space perspective as opposed to the OS 
perspective. While the OS treats all permission changes as permission 
restrictions, user space needs to call this ioctl() to support all 
enclave page permission changes:

* If the enclave page permissions are being restricted then this ioctl() 
would clear the page table entries and call ENCLS[EMODPR] that would 
have work to do to change the enclave page permissions.
* If the enclave page permissions are relaxed (should have been preceded 
by ENCLU[EMODPE] from within the enclave) then this ioctl() would do the 
same as in previous bullet (most importantly clear the page tables) but 
in this case ENCLS[EMODPR] would be a no-op as you indicate below.

Since user space needs OS support for both relaxing and restriction of 
permissions "SGX_IOC_PAGE_MODP" seemed appropriate.


> The pseudocode for EMODPR looks like this:
> 
>> (* Update EPCM permissions *)
>> EPCM(DS:RCX).R := EPCM(DS:RCX).R & SCRATCH_SECINFO.FLAGS.R;
>> EPCM(DS:RCX).W := EPCM(DS:RCX).W & SCRATCH_SECINFO.FLAGS.W;
>> EPCM(DS:RCX).X := EPCM(DS:RCX).X & SCRATCH_SECINFO.FLAGS.X;
> 
> so it makes total sense that it can only restrict permissions since it's
> effectively:
> 
> 	new_hw_perm = old_hw_perm & secinfo_perm;
> 
> ...
>> +/**
>> + * struct sgx_page_modp - parameter structure for the %SGX_IOC_PAGE_MODP ioctl
>> + * @offset:	starting page offset (page aligned relative to enclave base
>> + *		address defined in SECS)
>> + * @length:	length of memory (multiple of the page size)
>> + * @prot:	new protection bits of pages in range described by @offset
>> + *		and @length
>> + * @result:	SGX result code of ENCLS[EMODPR] function
>> + * @count:	bytes successfully changed (multiple of page size)
>> + */
>> +struct sgx_page_modp {
>> +	__u64 offset;
>> +	__u64 length;
>> +	__u64 prot;
>> +	__u64 result;
>> +	__u64 count;
>> +};
> 
> Could we make it more explicit that offset/length/prot are inputs and
> result/count are output?

This follows the pattern of existing struct sgx_enclave_add_pages. Could 
you please provide guidance or a reference of what you would like to 
see? I scanned all the files in arch/x86/include/uapi/asm/* defining RW 
ioctls and a few files in include/uapi/linux/* and I was not able to 
notice such a custom.

Would you like to see something like a "in_"/"out_" prefix? If so, would 
you like to see a preparatory patch that changes struct 
sgx_enclave_add_pages also? If needed, I am not sure how to handle the 
latter due to the possible user space impact.

> 
> ..
>> +	if (!params.length || params.length & (PAGE_SIZE - 1))
>> +		return -EINVAL;
> 
> I find these a bit easier to read if they're:
> 
> 	if (!params.length || !IS_ALIGNED(params.length, PAGE_SIZE))
> 		...
> 

I am not sure about this. First, (I understand this is not a reason to 
do things a particular way), this is re-using the vetted code from 
sgx_ioc_enclave_add_pages(). Second, my understanding of IS_ALIGNED is 
its use to indicate that a provided address/offset is on some boundary, 
in this case it is the length field being verified (not an address or 
offset) and it is required to be a multiple of the page size.

I understand that the code ends up being the same but I think that it 
may be hard to parse that a length field is required to be aligned.

No objection to changing this if you prefer using IS_ALIGNED and I will 
then also include a preparatory patch to change 
sgx_ioc_enclave_add_pages() and make the same change in the following 
patches.

Could you please let me know what you prefer?

>> +	if (params.offset + params.length - PAGE_SIZE >= encl->size)
>> +		return -EINVAL;
> 
> I hate boundary conditions. :)  But, I think this would be simpler
> written as:
> 
> 	if (params.offset + params.length > encl->size)
> 
> Please double-check me on that, though.  I've gotten these kinds of
> checks wrong more times than I care to admit.
> 

I am very cautious about boundary conditions and thus preferred to 
re-use the existing checks from sgx_ioc_enclave_add_pages(). Your 
suggestion is much simpler though and I will use it. Would you also like 
to see a preparatory patch that changes the existing check in 
sgx_ioc_enclave_add_pages()?

Reinette

Powered by blists - more mailing lists