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

["Xing, Cedric" <cedric.xing@intel.com>]

> From: Andy Lutomirski [mailto:luto@kernel.org]
> Sent: Monday, July 01, 2019 11:00 AM
> 
> On Wed, Jun 19, 2019 at 3:24 PM Sean Christopherson
> <sean.j.christopherson@intel.com> wrote:
> >  static int sgx_mmap(struct file *file, struct vm_area_struct *vma)  {
> >         struct sgx_encl *encl = file->private_data;
> > +       unsigned long allowed_rwx;
> >         int ret;
> >
> > +       allowed_rwx = sgx_allowed_rwx(encl, vma);
> > +       if (vma->vm_flags & (VM_READ | VM_WRITE | VM_EXEC) &
> ~allowed_rwx)
> > +               return -EACCES;
> > +
> >         ret = sgx_encl_mm_add(encl, vma->vm_mm);
> >         if (ret)
> >                 return ret;
> >
> > +       if (!(allowed_rwx & VM_READ))
> > +               vma->vm_flags &= ~VM_MAYREAD;
> > +       if (!(allowed_rwx & VM_WRITE))
> > +               vma->vm_flags &= ~VM_MAYWRITE;
> > +       if (!(allowed_rwx & VM_EXEC))
> > +               vma->vm_flags &= ~VM_MAYEXEC;
> > +
> 
> I'm with Cedric here -- this is no good.  The reason I think we
> need .may_mprotect or similar is exactly to avoid doing this.
> 
> mmap() just needs to make the same type of VMA regardless of the pages
> in the range.

Instead of making decisions on its own, a more generic approach is for SGX subsystem/module to ask LSM for a decision, by calling security_file_mprotect() - as a new mapping could be considered as changing protection from PROT_NONE to (vma->vm_flags & (VM_READ | VM_WRITE | VM_EXEC)).

.may_mprotect() also solves part of the problem - i.e. VMAs will be created consistently but non-existent pages still cannot be mapped, which however is necessary for #PF driven EAUG in SGX2. Given that security_file_mprotect() is invoked by mprotect() syscall, it looks to me a more streamlined solution to call the same hook (security_file_mprotect) from both places (mmap and mprotect).