Re: [PATCH v9 3/3] x86/sgx: Fine grained SGX MCA behavior for virtualization

From: "Huang, Kai" <kai.huang@intel.com>
To: "linux-sgx@vger.kernel.org" <linux-sgx@vger.kernel.org>,
	"Luck, Tony" <tony.luck@intel.com>,
	"Li, Zhiquan1" <zhiquan1.li@intel.com>,
	"Hansen, Dave" <dave.hansen@intel.com>,
	"jarkko@kernel.org" <jarkko@kernel.org>,
	"dave.hansen@linux.intel.com" <dave.hansen@linux.intel.com>
Cc: "tglx@linutronix.de" <tglx@linutronix.de>,
	"Du, Fan" <fan.du@intel.com>, "Christopherson,,
	Sean" <seanjc@google.com>, "Zhang, Cathy" <cathy.zhang@intel.com>,
	"bp@suse.de" <bp@suse.de>
Subject: Re: [PATCH v9 3/3] x86/sgx: Fine grained SGX MCA behavior for virtualization
Date: Wed, 19 Oct 2022 10:59:20 +0000	[thread overview]
Message-ID: <10c4b928a37fdf96df767fc7b8f1348f6af05984.camel@intel.com> (raw)
In-Reply-To: <061580727e503d092ca3867919fa0f26391568eb.camel@intel.com>

On Fri, 2022-10-14 at 00:19 +0000, Huang, Kai wrote:
> On Thu, 2022-10-13 at 16:57 -0700, Dave Hansen wrote:
> > On 10/13/22 16:40, Huang, Kai wrote:
> > > What's your suggestion?
> > 
> > I'm fresh out of suggestions.
> 
> Thanks Dave.  I'll dig more and find out a solution.
> 

Hi Dave,

Not related to this series, I think there's a bug in the current virtual EPC
driver page fault handler in case of fork() (sorry didn't notice it at the first
place):

static int __sgx_vepc_fault(struct sgx_vepc *vepc,
                            struct vm_area_struct *vma, unsigned long addr)
{
	......
        /* Calculate index of EPC page in virtual EPC's page_array */
        index = vma->vm_pgoff + PFN_DOWN(addr - vma->vm_start);

        epc_page = xa_load(&vepc->page_array, index);
        if (epc_page)
                return 0;

	...
}

As you can see if the EPC page has already been populated at a given index of
one virtual EPC instance, the current fault handler just assumes the mapping is
already there and returns success immediately.  This causes a bug when one
virtual EPC instance is shared by multi processes via fork(): if the EPC page at
one index is already populated by the parent process, when the child accesses
the same page using different virtual address, the fault handler just returns
success w/o actually setting up the mapping for the child, resulting in endless
page fault.

This needs to be fixed in no matter what way.

My thinking is we can just enforce in the kernel that one virtual EPC instance
can only be mmap()-ed by the process which opens /dev/sgx_vepc (a new virtual
EPC instance is created on each open).  KVM userspace should never use fork() to
share virtual EPC instance otherwise userspace is using it in the wrong way. 
Thus enforcing in the kernel won't break any KVM userspace.

Does this make sense?

If it is OK to you, I'll send out a patch to fix.

-- 
Thanks,
-Kai