[7/7] x86/sgx: Add a reserved field to sgx_enclave_add_region to drop 'packed'
diff mbox series

Message ID 20190605194845.926-8-sean.j.christopherson@intel.com
State New
Headers show
Series
  • x86/sgx: Clean up and enhance add pages ioctl
Related show

Commit Message

Sean Christopherson June 5, 2019, 7:48 p.m. UTC
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(-)

Comments

Dave Hansen June 5, 2019, 7:59 p.m. UTC | #1
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?
Andy Lutomirski June 5, 2019, 8 p.m. UTC | #2
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.
Jarkko Sakkinen June 12, 2019, 3:14 p.m. UTC | #3
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
Sean Christopherson June 12, 2019, 3:23 p.m. UTC | #4
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.
Jethro Beekman June 13, 2019, 12:44 a.m. UTC | #5
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
Jarkko Sakkinen June 13, 2019, 3:38 p.m. UTC | #6
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

Patch
diff mbox series

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;