lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <YhZWvFSuax2GI9cQ@iki.fi> Date: Wed, 23 Feb 2022 16:46:04 +0100 From: Jarkko Sakkinen <jarkko@...nel.org> To: Reinette Chatre <reinette.chatre@...el.com> Cc: dave.hansen@...ux.intel.com, tglx@...utronix.de, bp@...en8.de, luto@...nel.org, mingo@...hat.com, linux-sgx@...r.kernel.org, x86@...nel.org, 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 V2 16/32] x86/sgx: Support restricting of enclave page permissions 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
Powered by blists - more mailing lists