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
