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

[Sean Christopherson <seanjc@google.com>]

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