On Wed, Jun 19, 2019 at 08:20:18AM -0700, Sean Christopherson wrote:
> 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).
You're probably right that in practice it would hard to do much with
EMODT.
> - 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;
> };
LGTM but why it isn't like:
__u16 mrmask;
__u8 prot;
__u8 reserved[5];
/Jarkko
