Re: [RFC PATCH v3 00/27] KVM SGX virtualization support

Sean Christopherson
2021-02-03T17:37:28Z

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
*/