Re: [RFC PATCH v3 04/12] x86/sgx: Require userspace to define enclave pages' protection bits

[Sean Christopherson <sean.j.christopherson@intel.com>]

On Wed, Jun 19, 2019 at 05:43:11PM +0300, Jarkko Sakkinen wrote:
> On Mon, 2019-06-17 at 15:24 -0700, Sean Christopherson wrote:
> > +	__u32	flags;
> 
> This should be changed to secinfo_flags_mask containing a mask of the
> allowed bits for the secinfo flags because of two obvious reasons:
> 
> 1. Protection flags are used mainly with syscalls and contain also other
>    things than just the permissions that do not apply in this context.
> 2. Having a mask for all secinfo flags is more future proof.
> 
> With the protection flags you end up reserving bits forever for things
> that we will never have any use for (e.g. PROT_SEM).
> 
> Looking the change you convert 'flags' (wondering why it isn't called
> 'prot') to VM flags, which means that you essentially gain absolutely
> nothing and loose some potential versatility as a side-effect by doing
> that.

Ah, I see where you're coming from.  My intent was that supported flags
would be SGX specific, not generic PROT_* flags.  I.e. bits 2:0 are used
for PROT_{READ,WRITE,EXEC}, bit 7 can be used for SGX_ZERO_PAGE, etc...

I have two objections to 'secinfo_flags_mask':

  - A full SECINFO mask is problematic for literally every other bit/field
    currently defined in SECINFO.FLAGS, e.g. masking PAGE_TYPE, PENDING
    and MODIFIED adds no value that I can think of, but would require the
    kernel do to weird things like reject page types and EMODPR requests
    (due to their PENDING/MODIFIED interaction).

  - The kernel doesn't actually restrict SECINFO based on the param, it's
    restricting VM_MAY* flags in the vmas.  'secinfo_flags_mask' implies
    the kernel is somehow masking SECINFO.

What about something like this?

/**
 * struct sgx_enclave_add_page - parameter structure for the
 *                               %SGX_IOC_ENCLAVE_ADD_PAGE ioctl
 * @addr:	address within the ELRANGE
 * @src:	address for the page data
 * @secinfo:	address for the SECINFO data
 * @mrmask:	bitmask for the measured 256 byte chunks
 * @prot:	maximal PROT_{READ,WRITE,EXEC} permissions for the page
 */
struct sgx_enclave_add_page {
	__u64		addr;
	__u64		src;
	__u64		secinfo;
	__u16		mrmask;
	__u8		prot;
	__u8		pad;
	__u64[2]	reserved;
};