[RFC PATCH v2 3/5] x86/sgx: Enforce noexec filesystem restriction for enclaves

[Stephen Smalley]

On 6/10/19 12:44 PM, Andy Lutomirski wrote:
> On Mon, Jun 10, 2019 at 9:00 AM Jarkko Sakkinen
> <jarkko.sakkinen at linux.intel.com> wrote:
>>
>> On Wed, Jun 05, 2019 at 07:11:43PM -0700, Sean Christopherson wrote:
>>> +             goto out;
>>> +     }
>>> +
>>> +     /*
>>> +      * Query VM_MAYEXEC as an indirect path_noexec() check (see do_mmap()),
>>> +      * but with some future proofing against other cases that may deny
>>> +      * execute permissions.
>>> +      */
>>> +     if (!(vma->vm_flags & VM_MAYEXEC)) {
>>> +             ret = -EACCES;
>>> +             goto out;
>>> +     }
>>> +
>>> +     if (copy_from_user(dst, (void __user *)src, PAGE_SIZE))
>>> +             ret = -EFAULT;
>>> +     else
>>> +             ret = 0;
>>> +
>>> +out:
>>> +     up_read(&current->mm->mmap_sem);
>>> +
>>> +     return ret;
>>> +}
>>
>> I would suggest to express the above instead like this for clarity
>> and consistency:
>>
>>                  goto err_map_sem;
>>          }
>>
>>          /* Query VM_MAYEXEC as an indirect path_noexec() check
>>           * (see do_mmap()).
>>           */
>>          if (!(vma->vm_flags & VM_MAYEXEC)) {
>>                  ret = -EACCES;
>>                  goto err_mmap_sem;
>>          }
>>
>>          if (copy_from_user(dst, (void __user *)src, PAGE_SIZE)) {
>>                  ret = -EFAULT;
>>                  goto err_mmap_sem;
>>          }
>>
>>          return 0;
>>
>> err_mmap_sem:
>>          up_read(&current->mm->mmap_sem);
>>          return ret;
>> }
>>
>> The comment about future proofing is unnecessary.
>>
> 
> I'm also torn as to whether this patch is needed at all.  If we ever
> get O_MAYEXEC, then enclave loaders should use it to enforce noexec in
> userspace.  Otherwise I'm unconvinced it's that special.

What's a situation where we would want to allow this?  Why is it 
different than do_mmap()?