On Wed, Feb 03, 2021, Dave Hansen wrote: > On 2/2/21 3:56 PM, Sean Christopherson wrote: > >> I seem to remember much stronger language in the SDM about this. I've > >> always thought of SGX as a big unrecoverable machine-check party waiting > >> to happen. > >> > >> I'll ask around internally at Intel and see what folks say. Basically, > >> should we be afraid of a big bad EPC access? > > If bad accesses to the EPC can cause machine checks, then EPC should never be > > mapped into userspace, i.e. the SGX driver should never have been merged. > > The SDM doesn't define the behavior well enough. I'll try to get that > fixed. > > But, there is some documentation of the abort page semantics: > > > https://download.01.org/intel-sgx/sgx-linux/2.10/docs/Intel_SGX_Developer_Reference_Linux_2.10_Open_Source.pdf > > Basically, writes get dropped and reads get all 1's on all the > implementations in the wild. I actually would have much rather gotten a > fault, but oh well. > > It sounds like we need to at least modify KVM to make sure not to map > and access EPC addresses. Why? KVM will read garbage, but KVM needs to be careful with the data it reads, irrespective of SGX, because the data is user/guest controlled even in the happy case. I'm not at all opposed to preventing KVM from accessing EPC, but I really don't want to add a special check in KVM to avoid reading EPC. KVM generally isn't aware of physical backings, and the relevant KVM code is shared between all architectures. > We might even want to add some VM_WARN_ON()s in the code that creates kernel > mappings to catch these mappings if they happen anywhere else. One thought for handling this would be to extend __ioremap_check_other() to flag EPC in some way, and then disallow memremap() to EPC. A clever way to do that without disallowing SGX's initial memremap() would be to tap into SGX's sgx_epc_sections array, as the per-section check would activate only after each section is initialized/map by SGX. Disallowing memremap(), without warning, would address the KVM flow (the memremap() in __kvm_map_gfn()) without forcing KVM to explicitly check for EPC. E.g. something like: diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c index c519fc5f6948..f263f3554f27 100644 --- a/arch/x86/kernel/cpu/sgx/main.c +++ b/arch/x86/kernel/cpu/sgx/main.c @@ -26,6 +26,19 @@ static LIST_HEAD(sgx_active_page_list); static DEFINE_SPINLOCK(sgx_reclaimer_lock); +bool is_sgx_epc(resource_size_t addr, unsigned long size) +{ + resource_size_t end = addr + size - 1; + int i; + + for (i = 0; i < sgx_nr_epc_sections; i++) { + if (<check for overlap with sgx_epc_sections[i]) + return true; + } + + return false; +} + /* * Reset dirty EPC pages to uninitialized state. Laundry can be left with SECS * pages whose child pages blocked EREMOVE. diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c index 9e5ccc56f8e0..145fc6fc6bc5 100644 --- a/arch/x86/mm/ioremap.c +++ b/arch/x86/mm/ioremap.c @@ -34,6 +34,7 @@ */ struct ioremap_desc { unsigned int flags; + bool sgx_epc; }; /* @@ -110,8 +111,14 @@ static unsigned int __ioremap_check_encrypted(struct resource *res) * The EFI runtime services data area is not covered by walk_mem_res(), but must * be mapped encrypted when SEV is active. */ -static void __ioremap_check_other(resource_size_t addr, struct ioremap_desc *desc) +static void __ioremap_check_other(resource_size_t addr, unsigned long size, + struct ioremap_desc *desc) { + if (sgx_is_epc(addr, size)) { + desc->sgx_epc = true; + return; + } + if (!sev_active()) return; @@ -155,7 +162,7 @@ static void __ioremap_check_mem(resource_size_t addr, unsigned long size, walk_mem_res(start, end, desc, __ioremap_collect_map_flags); - __ioremap_check_other(addr, desc); + __ioremap_check_other(addr, size, desc); } /* @@ -210,6 +217,13 @@ __ioremap_caller(resource_size_t phys_addr, unsigned long size, return NULL; } + /* + * Don't allow mapping SGX EPC, it's not accessible via normal reads + * and writes. + */ + if (io_desc.epc) + return NULL; + /* * Mappings have to be page-aligned */
