> From: Andy Lutomirski [mailto:luto@xxxxxxxxxx] > Sent: Monday, July 01, 2019 11:00 AM > > On Wed, Jun 19, 2019 at 3:24 PM Sean Christopherson > <sean.j.christopherson@xxxxxxxxx> 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).
