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

Kai Huang
2021-01-15T14:08:15Z

On Wed, 6 Jan 2021 21:02:54 -0800 Dave Hansen wrote:
> On 1/6/21 5:42 PM, Kai Huang wrote:
> >> I understand why this made sense for regular enclaves, but I'm having a
> >> harder time here. If you mmap(fd, MAP_SHARED), fork(), and then pass
> >> that mapping through to two different guests, you get to hold the
> >> pieces, just like if you did the same with normal memory.
> >>
> >> Why does the kernel need to enforce this policy?
> > Does Sean's reply in another email satisfy you?
>
> I'm not totally convinced.
>
> Please give it a go in the changelog for the next one and try to
> convince me that this is a good idea. Focus on what the downsides will
> be if the kernel does not enforce this policy. What will break, and why
> will it be bad? Why is the kernel in the best position to thwart the
> badness?

Hi Dave, Sean,

Sorry for late reply of this. I feel I should try again to get consensus here.

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()?