RE: [PATCH v21 16/28] x86/sgx: Add the Linux SGX Enclave Driver

[Ayoun, Serge]

> From: Jarkko Sakkinen 
> Sent: Saturday, July 13, 2019 20:08
> Subject: [PATCH v21 16/28] x86/sgx: Add the Linux SGX Enclave Driver

> +static long sgx_ioc_enclave_add_page(struct file *filep, void __user
> +*arg) {
> + struct sgx_encl *encl = filep->private_data;
> + struct sgx_enclave_add_page addp;
> + struct sgx_secinfo secinfo;
> + struct page *data_page;
> + unsigned long prot;
> + void *data;
> + int ret;
> +
> + if (copy_from_user(, arg, sizeof(addp)))
> + return -EFAULT;
> +
> + if (copy_from_user(, (void __user *)addp.secinfo,
> +sizeof(secinfo)))
> + return -EFAULT;
> +
> + data_page = alloc_page(GFP_HIGHUSER);
> + if (!data_page)
> + return -ENOMEM;
> +
> + data = kmap(data_page);
> +
> + prot = _calc_vm_trans(secinfo.flags, SGX_SECINFO_R, PROT_READ)
> |
> +_calc_vm_trans(secinfo.flags, SGX_SECINFO_W, PROT_WRITE) |
> +_calc_vm_trans(secinfo.flags, SGX_SECINFO_X, PROT_EXEC);
> +
> + /* TCS pages need to be RW in the PTEs, but can be 0 in the EPCM. */
> + if ((secinfo.flags & SGX_SECINFO_PAGE_TYPE_MASK) ==
> SGX_SECINFO_TCS)
> + prot |= PROT_READ | PROT_WRITE;

For TCS pages you add both RD and WR maximum protection bits.
For the enclave to be able to run, user mode will have to change the 
"vma->vm_flags" from PROT_NONE to PROT_READ | PROT_WRITE (otherwise eenter 
fails). 
This is exactly what your selftest  does.
But when mmap (or mprotect) is called with PROT_READ bit, it automatically adds 
the PROT_EXEC bit unless the host application has been compiled with '-z 
noexecstack' option; pasting below the mmap() code which does it:

if ((prot & PROT_READ) && (current->personality & READ_IMPLIES_EXEC))
if (!(file && path_noexec(>f_path)))
prot |= PROT_EXEC;

The problem is that if PROT_EXEC bit is added then sgx_mmap callback will fail 
since PROT_EXEC will get blocked by your code and not allowed for TCS pages.
This restriction is not necessary at all, i.e. I wouldn't block PROT_EXEC on 
tcs area because anyway, the hardware will never let those areas to execute: 
the SGX protection flags are fixed by the cpu and can not be changed by any 
mean.
So in order to facilitate user's interface I would let prot |= PROT_READ | 
PROT_WRITE | PROT_EXEC; we do not give up to any security criteria and make 
user interaction easier.

> +
> + ret = sgx_encl_page_import_user(data, addp.src, prot);
> + if (ret)
> + goto out;
> +
> + ret = sgx_encl_add_page(encl, addp.addr, data, ,
> addp.mrmask,
> + prot);
> + if (ret)
> + goto out;
> +
> +out:
> + kunmap(data_page);
> + __free_page(data_page);
> + return ret;
> +}
-
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

RE: [RFC PATCH 6/9] x86/sgx: Require userspace to provide allowed prots to ADD_PAGES

[Ayoun, Serge]

> From: Christopherson, Sean J
> Sent: Saturday, June 01, 2019 02:32
> 
>  /**
>   * struct sgx_enclave_add_pages - parameter structure for the
>   *%SGX_IOC_ENCLAVE_ADD_PAGES ioctl
> @@ -39,6 +44,7 @@ struct sgx_enclave_create  {
>   * @secinfo: address for the SECINFO data (common to all pages)
>   * @nr_pages:number of pages (must be virtually contiguous)
>   * @mrmask:  bitmask for the measured 256 byte chunks (common to all
> pages)
> + * @flags:   flags, e.g. SGX_ALLOW_{READ,WRITE,EXEC} (common to all
> pages)
>   */
>  struct sgx_enclave_add_pages {
>   __u64   addr;
> @@ -46,7 +52,8 @@ struct sgx_enclave_add_pages {
>   __u64   secinfo;
>   __u32   nr_pages;
>   __u16   mrmask;
> -} __attribute__((__packed__));
> + __u16   flags;
> +};

You are adding a flags member. The secinfo structure has already a flags member 
in it.
Why do you need both - they are both coming from user mode. What kind of 
scenario would
require having different values. Seems confusing.

-
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

RE: [intel-sgx-kernel-dev] [PATCH v7 4/8] intel_sgx: driver for Intel Software Guard Extensions

[Ayoun, Serge]

> Subject: [intel-sgx-kernel-dev] [PATCH v7 4/8] intel_sgx: driver for Intel
> Software Guard Extensions
> 
> Intel SGX is a set of CPU instructions that can be used by applications
> to set aside private regions of code and data.  The code outside the
> enclave is disallowed to access the memory inside the enclave by the CPU
> access control.
> 
> SGX driver provides a ioctl API for loading and initializing enclaves.
> Address range for enclaves is reserved with mmap() and they are
> destroyed with munmap(). Enclave construction, measurement and
> initialization is done with the provided the ioctl API.
> 
> The driver implements also a swapper thread ksgxswapd for EPC pages
> backed by a private shmem file. Currently it has a limitation of not
> swapping VA pages but there is nothing preventing to implement it later
> on. Now it was scoped out in order to keep the implementation simple.
> 
> The parameter struct for SGX_IOC_ENCLAVE_INIT does not contain a
> parameter to supply a launch token. Generating and using tokens is best
> to be kept in the control of the kernel because it has direct binding to
> the IA32_SGXPUBKEYHASHx MSRs (a core must have MSRs set to the same
> value as the signer of token).
> 
> By giving user space any role in the launch process is a risk for
> introducing bottlenecks as kernel must exhibit behavior that user space
> launch daemon depends on, properietary risks (closed launch daemons on
> closed platforms) and stability risks as there would be division of
> semantics between user space and kernel.
> 
> Signed-off-by: Jarkko Sakkinen 
> ---
>  arch/x86/include/asm/sgx.h  | 233 ++
>  arch/x86/include/asm/sgx_arch.h | 270 +++
>  arch/x86/include/uapi/asm/sgx.h | 138 
>  drivers/platform/x86/Kconfig|   2 +
>  drivers/platform/x86/Makefile   |   1 +
>  drivers/platform/x86/intel_sgx/Kconfig  |  19 +
>  drivers/platform/x86/intel_sgx/Makefile |  13 +
>  drivers/platform/x86/intel_sgx/sgx.h| 251 ++
>  drivers/platform/x86/intel_sgx/sgx_encl.c   | 974
> 
>  drivers/platform/x86/intel_sgx/sgx_ioctl.c  | 281 +++
>  drivers/platform/x86/intel_sgx/sgx_main.c   | 413 ++
>  drivers/platform/x86/intel_sgx/sgx_page_cache.c | 647 
>  drivers/platform/x86/intel_sgx/sgx_util.c   | 346 +
>  drivers/platform/x86/intel_sgx/sgx_vma.c| 117 +++
>  14 files changed, 3705 insertions(+)
>  create mode 100644 arch/x86/include/asm/sgx.h
>  create mode 100644 arch/x86/include/asm/sgx_arch.h
>  create mode 100644 arch/x86/include/uapi/asm/sgx.h
>  create mode 100644 drivers/platform/x86/intel_sgx/Kconfig
>  create mode 100644 drivers/platform/x86/intel_sgx/Makefile
>  create mode 100644 drivers/platform/x86/intel_sgx/sgx.h
>  create mode 100644 drivers/platform/x86/intel_sgx/sgx_encl.c
>  create mode 100644 drivers/platform/x86/intel_sgx/sgx_ioctl.c
>  create mode 100644 drivers/platform/x86/intel_sgx/sgx_main.c
>  create mode 100644 drivers/platform/x86/intel_sgx/sgx_page_cache.c
>  create mode 100644 drivers/platform/x86/intel_sgx/sgx_util.c
>  create mode 100644 drivers/platform/x86/intel_sgx/sgx_vma.c
> 
> diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h
> new file mode 100644
> index ..2c2575100d0d
> --- /dev/null
> +++ b/arch/x86/include/asm/sgx.h
> @@ -0,0 +1,233 @@
> +/*
> + * This file is provided under a dual BSD/GPLv2 license.  When using or
> + * redistributing this file, you may do so under either license.
> + *
> + * GPL LICENSE SUMMARY
> + *
> + * Copyright(c) 2016-2017 Intel Corporation.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of version 2 of the GNU General Public License as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> GNU
> + * General Public License for more details.
> + *
> + * Contact Information:
> + * Jarkko Sakkinen 
> + * Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
> + *
> + * BSD LICENSE
> + *
> + * Copyright(c) 2016-2017 Intel Corporation.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + *
> + *   * Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + *   * Redistributions in binary form must reproduce the above copyright
> + * notice, this list of conditions and the following disclaimer in
> + * the documentation and/or other materials provided with the
> + *  

RE: [intel-sgx-kernel-dev] [PATCH v7 4/8] intel_sgx: driver for Intel Software Guard Extensions

[Ayoun, Serge]

> Subject: [intel-sgx-kernel-dev] [PATCH v7 4/8] intel_sgx: driver for Intel
> Software Guard Extensions
> 
> Intel SGX is a set of CPU instructions that can be used by applications
> to set aside private regions of code and data.  The code outside the
> enclave is disallowed to access the memory inside the enclave by the CPU
> access control.
> 
> SGX driver provides a ioctl API for loading and initializing enclaves.
> Address range for enclaves is reserved with mmap() and they are
> destroyed with munmap(). Enclave construction, measurement and
> initialization is done with the provided the ioctl API.
> 
> The driver implements also a swapper thread ksgxswapd for EPC pages
> backed by a private shmem file. Currently it has a limitation of not
> swapping VA pages but there is nothing preventing to implement it later
> on. Now it was scoped out in order to keep the implementation simple.
> 
> The parameter struct for SGX_IOC_ENCLAVE_INIT does not contain a
> parameter to supply a launch token. Generating and using tokens is best
> to be kept in the control of the kernel because it has direct binding to
> the IA32_SGXPUBKEYHASHx MSRs (a core must have MSRs set to the same
> value as the signer of token).
> 
> By giving user space any role in the launch process is a risk for
> introducing bottlenecks as kernel must exhibit behavior that user space
> launch daemon depends on, properietary risks (closed launch daemons on
> closed platforms) and stability risks as there would be division of
> semantics between user space and kernel.
> 
> Signed-off-by: Jarkko Sakkinen 
> ---
>  arch/x86/include/asm/sgx.h  | 233 ++
>  arch/x86/include/asm/sgx_arch.h | 270 +++
>  arch/x86/include/uapi/asm/sgx.h | 138 
>  drivers/platform/x86/Kconfig|   2 +
>  drivers/platform/x86/Makefile   |   1 +
>  drivers/platform/x86/intel_sgx/Kconfig  |  19 +
>  drivers/platform/x86/intel_sgx/Makefile |  13 +
>  drivers/platform/x86/intel_sgx/sgx.h| 251 ++
>  drivers/platform/x86/intel_sgx/sgx_encl.c   | 974
> 
>  drivers/platform/x86/intel_sgx/sgx_ioctl.c  | 281 +++
>  drivers/platform/x86/intel_sgx/sgx_main.c   | 413 ++
>  drivers/platform/x86/intel_sgx/sgx_page_cache.c | 647 
>  drivers/platform/x86/intel_sgx/sgx_util.c   | 346 +
>  drivers/platform/x86/intel_sgx/sgx_vma.c| 117 +++
>  14 files changed, 3705 insertions(+)
>  create mode 100644 arch/x86/include/asm/sgx.h
>  create mode 100644 arch/x86/include/asm/sgx_arch.h
>  create mode 100644 arch/x86/include/uapi/asm/sgx.h
>  create mode 100644 drivers/platform/x86/intel_sgx/Kconfig
>  create mode 100644 drivers/platform/x86/intel_sgx/Makefile
>  create mode 100644 drivers/platform/x86/intel_sgx/sgx.h
>  create mode 100644 drivers/platform/x86/intel_sgx/sgx_encl.c
>  create mode 100644 drivers/platform/x86/intel_sgx/sgx_ioctl.c
>  create mode 100644 drivers/platform/x86/intel_sgx/sgx_main.c
>  create mode 100644 drivers/platform/x86/intel_sgx/sgx_page_cache.c
>  create mode 100644 drivers/platform/x86/intel_sgx/sgx_util.c
>  create mode 100644 drivers/platform/x86/intel_sgx/sgx_vma.c
> 
> diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h
> new file mode 100644
> index ..2c2575100d0d
> --- /dev/null
> +++ b/arch/x86/include/asm/sgx.h
> @@ -0,0 +1,233 @@
> +/*
> + * This file is provided under a dual BSD/GPLv2 license.  When using or
> + * redistributing this file, you may do so under either license.
> + *
> + * GPL LICENSE SUMMARY
> + *
> + * Copyright(c) 2016-2017 Intel Corporation.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of version 2 of the GNU General Public License as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> GNU
> + * General Public License for more details.
> + *
> + * Contact Information:
> + * Jarkko Sakkinen 
> + * Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
> + *
> + * BSD LICENSE
> + *
> + * Copyright(c) 2016-2017 Intel Corporation.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + *
> + *   * Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + *   * Redistributions in binary form must reproduce the above copyright
> + * notice, this list of conditions and the following disclaimer in
> + * the documentation and/or other materials provided with the
> + * distribution.
> + *   * Neither the name of Intel Corporation