Re: [PATCH V4 00/31] x86/sgx and selftests/sgx: Support SGX2
From: Jarkko Sakkinen
Date: Thu Apr 14 2022 - 07:26:49 EST
On Wed, 2022-04-13 at 14:10 -0700, Reinette Chatre wrote:
> Now that the discussions surrounding the support for SGX2 is settling,
> the kselftest audience is added to the discussion for the first time
> to consider the testing of the new features.
>
> V3: https://lore.kernel.org/lkml/cover.1648847675.git.reinette.chatre@xxxxxxxxx/
>
> Changes since V3 that directly impact user space:
> - SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS ioctl()'s struct
> sgx_enclave_restrict_permissions no longer provides entire secinfo,
> just the new permissions in new "permissions" struct member. (Jarkko)
> - Rename SGX_IOC_ENCLAVE_MODIFY_TYPE ioctl() to
> SGX_IOC_ENCLAVE_MODIFY_TYPES. (Jarkko)
> - SGX_IOC_ENCLAVE_MODIFY_TYPES ioctl()'s struct sgx_enclave_modify_type
> no longer provides entire secinfo, just the new page type in new
> "page_type" struct member. (Jarkko)
>
> Details about changes since V3 that do not directly impact user space:
> - Add new patch to enable VA pages to be added without invoking reclaimer
> directly if no EPC pages are available, failing instead. This enables
> VA pages to be added with enclave's mutex held. Fixes an issue
> encountered by Haitao. More details in new patch "x86/sgx: Support VA page
> allocation without reclaiming".
> - While refactoring, change existing code to consistently use
> IS_ALIGNED(). (Jarkko)
> - Many patches received a tag from Jarkko.
> - Many smaller changes, please refer to individual patches.
>
> V2: https://lore.kernel.org/lkml/cover.1644274683.git.reinette.chatre@xxxxxxxxx/
>
> Changes since V2 that directly impact user space:
> - Maximum allowed permissions of dynamically added pages is RWX,
> previously limited to RW. (Jarkko)
> Dynamically added pages are initially created with architecturally
> limited EPCM permissions of RW. mmap() and mprotect() of these pages
> with RWX permissions would no longer be blocked by SGX driver. PROT_EXEC
> on dynamically added pages will be possible after running ENCLU[EMODPE]
> from within the enclave with appropriate VMA permissions.
>
> - The kernel no longer attempts to track the EPCM runtime permissions. (Jarkko)
> Consequences are:
> - Kernel does not modify PTEs to follow EPCM permissions. User space
> will receive #PF with SGX error code in cases where the V2
> implementation would have resulted in regular (non-SGX) page fault
> error code.
> - SGX_IOC_ENCLAVE_RELAX_PERMISSIONS is removed. This ioctl() was used
> to clear PTEs after permissions were modified from within the enclave
> and ensure correct PTEs are installed. Since PTEs no longer track
> EPCM permissions the changes in EPCM permissions would not impact PTEs.
> As long as new permissions are within the maximum vetted permissions
> (vm_max_prot_bits) only ENCLU[EMODPE] from within enclave is needed,
> as accompanied by appropriate VMA permissions.
>
> - struct sgx_enclave_restrict_perm renamed to
> sgx_enclave_restrict_permissions (Jarkko)
>
> - struct sgx_enclave_modt renamed to struct sgx_enclave_modify_type
> to be consistent with the verbose naming of other SGX uapi structs.
>
> Details about changes since V2 that do not directly impact user space:
> - Kernel no longer tracks the runtime EPCM permissions with the aim of
> installing accurate PTEs. (Jarkko)
> - In support of this change the following patches were removed:
> Documentation/x86: Document SGX permission details
> x86/sgx: Support VMA permissions more relaxed than enclave permissions
> x86/sgx: Add pfn_mkwrite() handler for present PTEs
> x86/sgx: Add sgx_encl_page->vm_run_prot_bits for dynamic permission changes
> x86/sgx: Support relaxing of enclave page permissions
> - No more handling of scenarios where VMA permissions may be more
> relaxed than what the EPCM allows. Enclaves are not prevented
> from accessing such pages and the EPCM permissions are entrusted
> to control access as supported by the SGX error code in page faults.
> - No more explicit setting of protection bits in page fault handler.
> Protection bits are inherited from VMA similar to SGX1 support.
>
> - Selftest patches are moved to the end of the series. (Jarkko)
>
> - New patch contributed by Jarkko to avoid duplicated code:
> x86/sgx: Export sgx_encl_page_alloc()
>
> - New patch separating changes from existing patch. (Jarkko)
> x86/sgx: Export sgx_encl_{grow,shrink}()
>
> - New patch to keep one required benefit from the (now removed) kernel
> EPCM permission tracking:
> x86/sgx: Support loading enclave page without VMA permissions check
>
> - Updated cover letter to reflect architecture changes.
>
> - Many smaller changes, please refer to individual patches.
>
> V1: https://lore.kernel.org/linux-sgx/cover.1638381245.git.reinette.chatre@xxxxxxxxx/
>
> Changes since V1 that directly impact user space:
> - SGX2 permission changes changed from a single ioctl() named
> SGX_IOC_PAGE_MODP to two new ioctl()s:
> SGX_IOC_ENCLAVE_RELAX_PERMISSIONS and
> SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS, supported by two different
> parameter structures (SGX_IOC_ENCLAVE_RELAX_PERMISSIONS does
> not support a result output parameter) (Jarkko).
>
> User space flow impact: After user space runs ENCLU[EMODPE] it
> needs to call SGX_IOC_ENCLAVE_RELAX_PERMISSIONS to have PTEs
> updated. Previously running SGX_IOC_PAGE_MODP in this scenario
> resulted in EPCM.PR being set but calling
> SGX_IOC_ENCLAVE_RELAX_PERMISSIONS will not result in EPCM.PR
> being set anymore and thus no need for an additional
> ENCLU[EACCEPT].
>
> - SGX_IOC_ENCLAVE_RELAX_PERMISSIONS and
> SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS
> obtain new permissions from secinfo as parameter instead of
> the permissions directly (Jarkko).
>
> - ioctl() supporting SGX2 page type change is renamed from
> SGX_IOC_PAGE_MODT to SGX_IOC_ENCLAVE_MODIFY_TYPE (Jarkko).
>
> - SGX_IOC_ENCLAVE_MODIFY_TYPE obtains new page type from secinfo
> as parameter instead of the page type directly (Jarkko).
>
> - ioctl() supporting SGX2 page removal is renamed from
> SGX_IOC_PAGE_REMOVE to SGX_IOC_ENCLAVE_REMOVE_PAGES (Jarkko).
>
> - All ioctl() parameter structures have been renamed as a result of the
> ioctl() renaming:
> SGX_IOC_ENCLAVE_RELAX_PERMISSIONS => struct sgx_enclave_relax_perm
> SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS => struct sgx_enclave_restrict_perm
> SGX_IOC_ENCLAVE_MODIFY_TYPE => struct sgx_enclave_modt
> SGX_IOC_ENCLAVE_REMOVE_PAGES => struct sgx_enclave_remove_pages
>
> Changes since V1 that do not directly impact user space:
> - Number of patches in series increased from 25 to 32 primarily because
> of splitting the original submission:
> - Wrappers for the new SGX2 functions are introduced in three separate
> patches replacing the original "x86/sgx: Add wrappers for SGX2
> functions"
> (Jarkko).
> - Moving and renaming sgx_encl_ewb_cpumask() is done with two patches
> replacing the original "x86/sgx: Use more generic name for enclave
> cpumask function" (Jarkko).
> - Support for SGX2 EPCM permission changes is split into two ioctls(),
> one for relaxing and one for restricting permissions, each introduced
> by a new patch replacing the original "x86/sgx: Support enclave page
> permission changes" (Jarkko).
> - Extracted code used by existing ioctls() for usage by new ioctl()s
> into a new utility in new patch "x86/sgx: Create utility to validate
> user provided offset and length" (Dave did not specifically ask for
> this but it addresses his review feedback).
> - Two new Documentation patches to support the SGX2 work
> ("Documentation/x86: Introduce enclave runtime management") and
> a dedicated section on the enclave permission management
> ("Documentation/x86: Document SGX permission details") (Andy).
> - Most patches were reworked to improve the language by:
> * aiming to refer to exact item instead of English rephrasing (Jarkko).
> * use ioctl() instead of ioctl throughout (Dave).
> * Use "relaxed" instead of "exceed" when referring to permissions
> (Dave).
> - Improved documentation with several additions to
> Documentation/x86/sgx.rst.
> - Many smaller changes, please refer to individual patches.
>
> Hi Everybody,
>
> The current Linux kernel support for SGX includes support for SGX1 that
> requires that an enclave be created with properties that accommodate all
> usages over its (the enclave's) lifetime. This includes properties such
> as permissions of enclave pages, the number of enclave pages, and the
> number of threads supported by the enclave.
>
> Consequences of this requirement to have the enclave be created to
> accommodate all usages include:
> * pages needing to support relocated code are required to have RWX
> permissions for their entire lifetime,
> * an enclave needs to be created with the maximum stack and heap
> projected to be needed during the enclave's entire lifetime which
> can be longer than the processes running within it,
> * an enclave needs to be created with support for the maximum number
> of threads projected to run in the enclave.
>
> Since SGX1 a few more functions were introduced, collectively called
> SGX2, that support modifications to an initialized enclave. Hardware
> supporting these functions are already available as listed on
> https://github.com/ayeks/SGX-hardware
>
> This series adds support for SGX2, also referred to as Enclave Dynamic
> Memory Management (EDMM). This includes:
>
> * Support modifying EPCM permissions of regular enclave pages belonging
> to an initialized enclave. Only permission restriction is supported
> via a new ioctl() SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS. Relaxing of
> EPCM permissions can only be done from within the enclave with the
> SGX instruction ENCLU[EMODPE].
>
> * Support dynamic addition of regular enclave pages to an initialized
> enclave. At creation new pages are architecturally limited to RW EPCM
> permissions but will be accessible with PROT_EXEC after the enclave
> runs ENCLU[EMODPE] to relax EPCM permissions to RWX.
> Pages are dynamically added to an initialized enclave from the SGX
> page fault handler.
>
> * Support expanding an initialized enclave to accommodate more threads.
> More threads can be accommodated by an enclave with the addition of
> Thread Control Structure (TCS) pages that is done by changing the
> type of regular enclave pages to TCS pages using a new ioctl()
> SGX_IOC_ENCLAVE_MODIFY_TYPES.
>
> * Support removing regular and TCS pages from an initialized enclave.
> Removing pages is accomplished in two stages as supported by two new
> ioctl()s SGX_IOC_ENCLAVE_MODIFY_TYPES (same ioctl() as mentioned in
> previous bullet) and SGX_IOC_ENCLAVE_REMOVE_PAGES.
>
> * Tests covering all the new flows, some edge cases, and one
> comprehensive stress scenario.
>
> No additional work is needed to support SGX2 in a virtualized
> environment. All tests included in this series passed when run from
> a guest as tested with the recent QEMU release based on 6.2.0
> that supports SGX.
>
> Patches 1 through 14 prepare the existing code for SGX2 support by
> introducing the SGX2 functions, refactoring code, and tracking enclave
> page types.
>
> Patches 15 through 21 enable the SGX2 features and include a
> Documentation patch.
>
> Patches 22 through 31 test several scenarios of all the enabled
> SGX2 features.
>
> This series is based on v5.18-rc2.
>
> Your feedback will be greatly appreciated.
>
> Regards,
>
> Reinette
>
> Jarkko Sakkinen (1):
> x86/sgx: Export sgx_encl_page_alloc()
>
> Reinette Chatre (30):
> x86/sgx: Add short descriptions to ENCLS wrappers
> x86/sgx: Add wrapper for SGX2 EMODPR function
> x86/sgx: Add wrapper for SGX2 EMODT function
> x86/sgx: Add wrapper for SGX2 EAUG function
> x86/sgx: Support loading enclave page without VMA permissions check
> x86/sgx: Export sgx_encl_ewb_cpumask()
> x86/sgx: Rename sgx_encl_ewb_cpumask() as sgx_encl_cpumask()
> x86/sgx: Move PTE zap code to new sgx_zap_enclave_ptes()
> x86/sgx: Make sgx_ipi_cb() available internally
> x86/sgx: Create utility to validate user provided offset and length
> x86/sgx: Keep record of SGX page type
> x86/sgx: Export sgx_encl_{grow,shrink}()
> x86/sgx: Support VA page allocation without reclaiming
> x86/sgx: Support restricting of enclave page permissions
> x86/sgx: Support adding of pages to an initialized enclave
> x86/sgx: Tighten accessible memory range after enclave initialization
> x86/sgx: Support modifying SGX page type
> x86/sgx: Support complete page removal
> x86/sgx: Free up EPC pages directly to support large page ranges
> Documentation/x86: Introduce enclave runtime management section
> selftests/sgx: Add test for EPCM permission changes
> selftests/sgx: Add test for TCS page permission changes
> selftests/sgx: Test two different SGX2 EAUG flows
> selftests/sgx: Introduce dynamic entry point
> selftests/sgx: Introduce TCS initialization enclave operation
> selftests/sgx: Test complete changing of page type flow
> selftests/sgx: Test faulty enclave behavior
> selftests/sgx: Test invalid access to removed enclave page
> selftests/sgx: Test reclaiming of untouched page
> selftests/sgx: Page removal stress test
>
> Documentation/x86/sgx.rst | 15 +
> arch/x86/include/asm/sgx.h | 8 +
> arch/x86/include/uapi/asm/sgx.h | 61 +
> arch/x86/kernel/cpu/sgx/encl.c | 329 +++-
> arch/x86/kernel/cpu/sgx/encl.h | 15 +-
> arch/x86/kernel/cpu/sgx/encls.h | 33 +
> arch/x86/kernel/cpu/sgx/ioctl.c | 640 +++++++-
> arch/x86/kernel/cpu/sgx/main.c | 75 +-
> arch/x86/kernel/cpu/sgx/sgx.h | 3 +
> tools/testing/selftests/sgx/defines.h | 23 +
> tools/testing/selftests/sgx/load.c | 41 +
> tools/testing/selftests/sgx/main.c | 1435 +++++++++++++++++
> tools/testing/selftests/sgx/main.h | 1 +
> tools/testing/selftests/sgx/test_encl.c | 68 +
> .../selftests/sgx/test_encl_bootstrap.S | 6 +
> 15 files changed, 2625 insertions(+), 128 deletions(-)
>
>
> base-commit: ce522ba9ef7e2d9fb22a39eb3371c0c64e2a433e
IMHO, we can pull this after +1 version. I think I had only one nit
(one character to a struct name it was), and I've been testing this
series *extensively* with real-world code (wasm run-time that we are
developing), so I'm confident that it is *good enough*.
Reinette, for the EMODT patch, as long as you fix the struct name
you can add my reviewed-by and also tested-by to that patch before
you send it! It's so narrow change.
BR, Jarkko