Re: [RFC PATCH 03/23] x86/sgx: Introduce virtual EPC for use by KVM guests

From: Kai Huang <kai.huang@intel.com>
To: Sean Christopherson <seanjc@google.com>
Cc: Dave Hansen <dave.hansen@intel.com>,
	linux-sgx@vger.kernel.org, kvm@vger.kernel.org, x86@kernel.org,
	jarkko@kernel.org, luto@kernel.org, haitao.huang@intel.com,
	pbonzini@redhat.com, bp@alien8.de, tglx@linutronix.de,
	mingo@redhat.com, hpa@zytor.com
Subject: Re: [RFC PATCH 03/23] x86/sgx: Introduce virtual EPC for use by KVM guests
Date: Sat, 16 Jan 2021 11:30:13 +1300	[thread overview]
Message-ID: <20210116113013.f2281b3ac20e6b5a558ceec1@intel.com> (raw)
In-Reply-To: <YAIM8cRtW3Jn6FRD@google.com>

On Fri, 15 Jan 2021 13:45:21 -0800 Sean Christopherson wrote:
> On Sat, Jan 16, 2021, Kai Huang wrote:
> > On Fri, 15 Jan 2021 07:39:44 -0800 Dave Hansen wrote:
> > > On 1/15/21 6:07 AM, Kai Huang wrote:
> > > >>From virtual EPC's perspective, if we don't force this in kernel, then
> > > > *theoretically*, userspace can use fork() to make multiple VMs map to the
> > > > same physical EPC, which will potentially cause enclaves in all VMs to behave
> > > > abnormally. So to me, from this perspective, it's better to enforce in kernel
> > > > so that only first VM can use this virtual EPC instance, because EPC by
> > > > architectural design cannot be shared.
> > > > 
> > > > But as Sean said, KVM doesn't support VM across multiple mm structs. And if I
> > > > read code correctly, KVM doesn't support userspace to use fork() to create new
> > > > VM. For instance, when creating VM, KVM grabs current->mm and keeps it in
> > > > 'struct kvm' for bookkeeping, and kvm_vcpu_ioctl() and kvm_device_ioctl() will
> > > > refuse to work if kvm->mm doesn't equal to current->mm. So in practice, I
> > > > believe w/o enforcing this in kernel, we should also have no problem here.
> > > > 
> > > > Sean, please correct me if I am wrong.
> > > > 
> > > > Dave, if above stands, do you think it is reasonable to keep current->mm in
> > > > epc->mm and enforce in sgx_virt_epc_mmap()?
> > > 
> > > Everything you wrote above tells me the kernel should not be enforcing
> > > the behavior.  You basically said that it's only a theoretical problem,
> > > and old if someone goes and does something with KVM that's nobody can do
> > > today.
> > > 
> > > You've 100% convinced me that having the kernel enforce this is
> > > *un*reasonable.
> > 
> > Sean, I'll remove epc->mm, unless I see your further objection.
> 
> It's probably ok.  I guess worst case scenario, to avoid the mm tracking
> nightmare for oversubscription, you could prevent attaching KVM to a virtual EPC
> if there is already a mm associated with the EPC, or if there are already EPC
> pages "in" the virt EPC.

Since we are not 100% certain oversubscription will be upstreamed, I think it
makes sense to address when we do it. For now, let us just drop it. 

Makes sense? Thanks.