| Message ID | 20190605194845.926-8-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/sgx: Clean up and enhance add pages ioctl | expand |
On 6/5/19 12:48 PM, Sean Christopherson wrote: > struct sgx_enclave_add_region { > __u64 addr; > @@ -51,7 +52,8 @@ struct sgx_enclave_add_region { > __u64 secinfo; > __u32 flags; > __u16 mrmask; > -} __attribute__((__packed__)); > + __u16 reserved; > +}; Without __packed__ do we have strong enough guarantees from the compiler for a stable ABI?
On Wed, Jun 5, 2019 at 12:59 PM Dave Hansen <dave.hansen@intel.com> wrote: > > On 6/5/19 12:48 PM, Sean Christopherson wrote: > > struct sgx_enclave_add_region { > > __u64 addr; > > @@ -51,7 +52,8 @@ struct sgx_enclave_add_region { > > __u64 secinfo; > > __u32 flags; > > __u16 mrmask; > > -} __attribute__((__packed__)); > > + __u16 reserved; > > +}; > > Without __packed__ do we have strong enough guarantees from the compiler > for a stable ABI? We rely on this kind of thing for uapi on a regular basis, so I sure hope so.
On Wed, Jun 05, 2019 at 12:48:45PM -0700, Sean Christopherson wrote: > A reserved field could prove useful in the future, and packed structs > can result in inefficiencies due to its impact on alignment. > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> Rather should extend mrmask just to 32 bits. /Jarkko
On Wed, Jun 12, 2019 at 06:14:39PM +0300, Jarkko Sakkinen wrote: > On Wed, Jun 05, 2019 at 12:48:45PM -0700, Sean Christopherson wrote: > > A reserved field could prove useful in the future, and packed structs > > can result in inefficiencies due to its impact on alignment. > > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > > Rather should extend mrmask just to 32 bits. I considered that option, but extending mrmask would probably confusing to userspace, e.g. we'd have to document that bits 31:15 are ignored.
On 2019-06-12 08:23, Sean Christopherson wrote: > On Wed, Jun 12, 2019 at 06:14:39PM +0300, Jarkko Sakkinen wrote: >> On Wed, Jun 05, 2019 at 12:48:45PM -0700, Sean Christopherson wrote: >>> A reserved field could prove useful in the future, and packed structs >>> can result in inefficiencies due to its impact on alignment. >>> >>> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> >> >> Rather should extend mrmask just to 32 bits. > > I considered that option, but extending mrmask would probably confusing > to userspace, e.g. we'd have to document that bits 31:15 are ignored. > Yeah this doesn't make a whole lot of sense to me. -- Jethro Beekman | Fortanix
On Wed, Jun 12, 2019 at 08:23:42AM -0700, Sean Christopherson wrote: > On Wed, Jun 12, 2019 at 06:14:39PM +0300, Jarkko Sakkinen wrote: > > On Wed, Jun 05, 2019 at 12:48:45PM -0700, Sean Christopherson wrote: > > > A reserved field could prove useful in the future, and packed structs > > > can result in inefficiencies due to its impact on alignment. > > > > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > > > > Rather should extend mrmask just to 32 bits. > > I considered that option, but extending mrmask would probably confusing > to userspace, e.g. we'd have to document that bits 31:15 are ignored. I meant u64. Not a big deal. With the reserved field we would have to document the same in different terms. /Jarkko
diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h index 18204722f238..e2d9b3d512ef 100644 --- a/arch/x86/include/uapi/asm/sgx.h +++ b/arch/x86/include/uapi/asm/sgx.h @@ -43,6 +43,7 @@ struct sgx_enclave_create { * @secinfo: address of the SECINFO data (common to the entire region) * @flags: miscellaneous flags * @mrmask: bitmask of 256 byte chunks to measure (applied per 4k page) + * @reserved: reserved for future use, must be zero */ struct sgx_enclave_add_region { __u64 addr; @@ -51,7 +52,8 @@ struct sgx_enclave_add_region { __u64 secinfo; __u32 flags; __u16 mrmask; -} __attribute__((__packed__)); + __u16 reserved; +}; /** * struct sgx_enclave_init - parameter structure for the diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c index e05a539e96fc..bae5f3155376 100644 --- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c +++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c @@ -663,6 +663,9 @@ static long sgx_ioc_enclave_add_region(struct file *filep, void __user *arg) if (!IS_ALIGNED(region.addr, 4096) || !IS_ALIGNED(region.size, 4096)) return -EINVAL; + if (region.reserved) + return -EINVAL; + if (!region.size) return 0;
A reserved field could prove useful in the future, and packed structs can result in inefficiencies due to its impact on alignment. Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- arch/x86/include/uapi/asm/sgx.h | 4 +++- arch/x86/kernel/cpu/sgx/driver/ioctl.c | 3 +++ 2 files changed, 6 insertions(+), 1 deletion(-)