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

[Reinette Chatre]

Hi Jarkko,

On 2/23/2022 7:46 AM, Jarkko Sakkinen wrote:
> On Tue, Feb 22, 2022 at 10:35:04AM -0800, Reinette Chatre wrote:
>> Hi Jarkko,
>>
>> On 2/20/2022 4:49 PM, Jarkko Sakkinen wrote:
>>> On Mon, Feb 07, 2022 at 04:45:38PM -0800, Reinette Chatre wrote:
>>
>> ...
>>
>>>> diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
>>>> index 5c678b27bb72..b0ffb80bc67f 100644
>>>> --- a/arch/x86/include/uapi/asm/sgx.h
>>>> +++ b/arch/x86/include/uapi/asm/sgx.h
>>>> @@ -31,6 +31,8 @@ enum sgx_page_flags {
>>>>  	_IO(SGX_MAGIC, 0x04)
>>>>  #define SGX_IOC_ENCLAVE_RELAX_PERMISSIONS \
>>>>  	_IOWR(SGX_MAGIC, 0x05, struct sgx_enclave_relax_perm)
>>>> +#define SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS \
>>>> +	_IOWR(SGX_MAGIC, 0x06, struct sgx_enclave_restrict_perm)
>>>>  
>>>>  /**
>>>>   * struct sgx_enclave_create - parameter structure for the
>>>> @@ -95,6 +97,25 @@ struct sgx_enclave_relax_perm {
>>>>  	__u64 count;
>>>>  };
>>>>  
>>>> +/**
>>>> + * struct sgx_enclave_restrict_perm - parameters for ioctl
>>>> + *                                    %SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS
>>>> + * @offset:	starting page offset (page aligned relative to enclave base
>>>> + *		address defined in SECS)
>>>> + * @length:	length of memory (multiple of the page size)
>>>> + * @secinfo:	address for the SECINFO data containing the new permission bits
>>>> + *		for pages in range described by @offset and @length
>>>> + * @result:	(output) SGX result code of ENCLS[EMODPR] function
>>>> + * @count:	(output) bytes successfully changed (multiple of page size)
>>>> + */
>>>> +struct sgx_enclave_restrict_perm {
>>>> +	__u64 offset;
>>>> +	__u64 length;
>>>> +	__u64 secinfo;
>>>> +	__u64 result;
>>>> +	__u64 count;
>>>> +};
>>>> +
>>>>  struct sgx_enclave_run;
>>>>  
>>>>  /**
>>
>> ...
>>
>>>
>>> Just a suggestion but these might be a bit less cluttered explanations of
>>> the fields:
>>>
>>> /// SGX_IOC_ENCLAVE_RELAX_PERMISSIONS parameter structure
>>> #[repr(C)]
>>> pub struct RelaxPermissions {
>>>     /// In: starting page offset
>>>     offset: u64,
>>>     /// In: length of the address range (multiple of the page size)
>>>     length: u64,
>>>     /// In: SECINFO containing the relaxed permissions
>>>     secinfo: u64,
>>>     /// Out: length of the address range successfully changed
>>>     count: u64,
>>> };
>>>
>>> /// SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS parameter structure
>>> #[repr(C)]
>>> pub struct RestrictPermissions {
>>>     /// In: starting page offset
>>>     offset: u64,
>>>     /// In: length of the address range (multiple of the page size)
>>>     length: u64,
>>>     /// In: SECINFO containing the restricted permissions
>>>     secinfo: u64,
>>>     /// In: ENCLU[EMODPR] return value
>>>     result: u64,
>>>     /// Out: length of the address range successfully changed
>>>     count: u64,
>>> };
>>
>> In your proposal you shorten the descriptions from the current implementation.
>> I do consider the removed information valuable since I believe that it helps
>> users understand the kernel interface requirements without needing to be
>> familiar with or dig into the kernel code to understand how the provided data
>> is used.
>>
>> For example, you shorten offset to "starting page offset", but what was removed
>> was the requirement that this offset has to be page aligned and what the offset
>> is relative to. I do believe summarizing these requirements upfront helps
>> a user space developer by not needing to dig through kernel code later
>> in order to understand why an -EINVAL was received.
>>
>>  
>>> I can live with the current ones too but I rewrote them so that I can
>>> quickly make sense of the fields later. It's Rust code but the point is
>>> the documentation...
>>
>> Since you do seem to be ok with the current descriptions I would prefer
>> to keep them.
> 
> Yeah, they are fine to me.
> 
>>> Also, it should not be too much trouble to use the struct in user space
>>> code even if the struct names are struct sgx_enclave_relax_permissions and
>>> struct sgx_enclave_restrict_permissions, given that you most likely have
>>> exactly single call-site in the run-time.
>>
>> Are you requesting that I make the following name changes?
>> struct sgx_enclave_relax_perm -> struct sgx_enclave_relax_permissions
>> struct sgx_enclave_restrict_perm -> struct sgx_enclave_restrict_permissions
>>
>> If so, do you want the function names also written out in this way?
>> sgx_enclave_relax_perm()        -> sgx_enclave_relax_permissions()
>> sgx_ioc_enclave_relax_perm()    -> sgx_ioc_enclave_relax_permissions()
>> sgx_enclave_restrict_perm()     -> sgx_enclave_restrict_permissions()
>> sgx_ioc_enclave_restrict_perm() -> sgx_ioc_enclave_restrict_permissions()
> 
> Yes, unless you have a specific reason to shorten them :-)

Just aesthetic reasons ... having a long function name can look unbalanced
if it has many parameters and if the parameters themselves are long it
becomes hard to keep to the required line length.

Even so, it does look as though the longest ones can be made to work within 80
characters:
sgx_enclave_restrict_permissions(...
                                 struct sgx_enclave_restrict_permissions *modp,
                                 ...)

Other (aesthetic) consequence would be, for example, the core sgx_ioctl() would
now have some branches span more lines than other so it would not look as neat as
now (this is subjective I know).

Apart from the aesthetic reasons I do not have another reason not to make the
change and I will do so in the next version.

Reinette