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

[Reinette Chatre]

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.

> 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()

> Other than that, looks quite good.

Thank you very much for reviewing and testing this work.

Reinette