Re: [RFC PATCH v2 2/5] x86/sgx: Require userspace to define enclave pages' protection bits
From: Jarkko Sakkinen
Date: Wed Jun 12 2019 - 15:33:27 EST
On Mon, Jun 10, 2019 at 11:17:44AM -0700, Sean Christopherson wrote:
> On Mon, Jun 10, 2019 at 08:45:06PM +0300, Jarkko Sakkinen wrote:
> > On Mon, Jun 10, 2019 at 09:15:33AM -0700, Sean Christopherson wrote:
> > > > 'flags' should would renamed as 'secinfo_flags_mask' even if the name is
> > > > longish. It would use the same values as the SECINFO flags. The field in
> > > > struct sgx_encl_page should have the same name. That would express
> > > > exactly relation between SECINFO and the new field. I would have never
> > > > asked on last iteration why SECINFO is not enough with a better naming.
> > >
> > > No, these flags do not impact the EPCM protections in any way. Userspace
> > > can extend the EPCM protections without going through the kernel. The
> > > protection flags for an enclave page impact VMA/PTE protection bits.
> > >
> > > IMO, it is best to treat the EPCM as being completely separate from the
> > > kernel's EPC management.
> > It is a clumsy API if permissions are not taken in the same format for
> > everything. There is no reason not to do it. The way mprotect() callback
> > just interprets the field is as VMA permissions.
> They are two entirely different things. The explicit protection bits are
> consumed by the kernel, while SECINFO.flags is consumed by the CPU. The
> intent is to have the protection flags be analogous to mprotect(), the
> fact that they have a similar/identical format to SECINFO is irrelevant.
> Calling the field secinfo_flags_mask is straight up wrong on SGX2, as
> userspace can use EMODPE to set SECINFO after the page is added. It's
> also wrong on SGX1 when adding TCS pages since SECINFO.RWX bits for TCS
> pages are forced to zero by hardware.
The new variable tells the limits on which kernel will co-operate with
the enclave. It is way more descriptive than 'flags'.
> > It would also be more future-proof just to have a mask covering all bits
> > of the SECINFO flags field.
> This simply doesn't work, e.g. the PENDING, MODIFIED and PR flags in the
> SECINFO are read-only from a software perspective.
It is easy to validate reserved bits from a SECINFO struct.