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. /Jarkko
