On 6/10/19 12:44 PM, Andy Lutomirski wrote:
On Mon, Jun 10, 2019 at 9:00 AM Jarkko Sakkinen
<jarkko.sakkinen@xxxxxxxxxxxxxxx> 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(¤t->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(¤t->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()?
