Re: x86/sgx: uapi change proposal

[Jethro Beekman <jethro@fortanix.com>]

[-- Attachment #1: Type: text/plain, Size: 3795 bytes --]

On 2018-12-19 13:28, Jarkko Sakkinen wrote:
> I have pretty much figured out how to change the driver implementation
> from VMA based to file based. Most of the code in the driver can be
> reused with not that enormous changes. I think it is a clue that the
> architecture is somewhat right because changing the driver this
> radically does not seem to require any changes to the core.

One weird thing is the departure from the normal mmap behavior that the 
memory mapping persists even if the original fd is closed. (See man 
mmap: "closing the file descriptor does not unmap the region.")

> Using anon inode is the right choice because it is more robust interface
> to be able to create multiple enclaves.
> 
> The only remaining open that I have when it comes to implementing this
> is the backing storage. From API perspective the most robust choice
> would be to revert to use shmem file. It would be easy then to create a
> complete construction flow without any dependencies to mm_struct.
> 
> I do recognize the issue with accounting but to which process the
> backing storage should be accounted anyway in this new paradigm.
> 
> I've attached the new uapi header to this email that I'm going forward
> with.
> 
> /Jarkko
> 
> sgx.h
> 
> /* SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) */
> /**
>  * Copyright(c) 2016-18 Intel Corporation.
>  */
> #ifndef _UAPI_ASM_X86_SGX_H
> #define _UAPI_ASM_X86_SGX_H
> 
> #include <linux/types.h>
> #include <linux/ioctl.h>
> 
> #define SGX_MAGIC 0xA4
> 
> #define SGX_IOC_ENCLAVE_CREATE \
> 	_IOW(SGX_MAGIC, 0x00, struct sgx_enclave_create)
> #define SGX_IOC_ENCLAVE_ADD_PAGE \
> 	_IOW(SGX_MAGIC, 0x01, struct sgx_enclave_add_page)
> #define SGX_IOC_ENCLAVE_INIT \
> 	_IOW(SGX_MAGIC, 0x02, struct sgx_enclave_init)
> #define SGX_IOC_ENCLAVE_SET_ATTRIBUTE \
> 	_IOW(SGX_MAGIC, 0x03, struct sgx_enclave_set_attribute)
> 
> /* IOCTL return values */
> #define SGX_POWER_LOST_ENCLAVE		0x40000000
> 
> /**
>  * struct sgx_enclave_create - parameter structure for the
>  *                             %SGX_IOC_ENCLAVE_CREATE ioctl
>  * @src:	address for the SECS page data
>  * @enclave_fd:	file handle to the enclave address space (out)
>  */
> struct sgx_enclave_create  {
> 	__u64	src;
> 	__u64	enclave_fd;
> };
> 
> /**
>  * struct sgx_enclave_add_page - parameter structure for the
>  *                               %SGX_IOC_ENCLAVE_ADD_PAGE ioctl
>  * @eclave_fd:	file handle to the enclave address space
>  * @src:	address for the page data
>  * @secinfo:	address for the SECINFO data
>  * @mrmask:	bitmask for the measured 256 byte chunks
>  */
> struct sgx_enclave_add_page {
> 	__u64	enclave_fd;
> 	__u64	src;
> 	__u64	secinfo;
> 	__u16	mrmask;
> } __attribute__((__packed__));

Wouldn't you just pass enclave_fd as the ioctl fd parameter?

How to specify the address of the page that is being added?

> 
> 
> /**
>  * struct sgx_enclave_init - parameter structure for the
>  *                           %SGX_IOC_ENCLAVE_INIT ioctl
>  * @eclave_fd:	file handle to the enclave address space
>  * @sigstruct:	address for the SIGSTRUCT data
>  */
> struct sgx_enclave_init {
> 	__u64	enclave_fd;
> 	__u64	sigstruct;
> }; >
> /**
>  * struct sgx_enclave_set_attribute - parameter structure for the
>  *				      %SGX_IOC_ENCLAVE_INIT ioctl
>  * @addr:		address within the ELRANGE

Stray parameter in docs

>  * @eclave_fd:		file handle to the enclave address space
>  * @attribute_fd:	file handle of the attribute file in the securityfs
>  */
> struct sgx_enclave_set_attribute {
> 	__u64	enclave_fd;
> 	__u64	attribute_fd;
> };

What is this for?

> 
> #endif /* _UAPI_ASM_X86_SGX_H */

--
Jethro Beekman | Fortanix


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 3990 bytes --]