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

[Jarkko Sakkinen <jarkko@kernel.org>]

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

> > Other than that, looks quite good.
> 
> Thank you very much for reviewing and testing this work.

NP
 
> Reinette

BR, Jarkko