| Message ID | 20190606021145.12604-4-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | security: x86/sgx: SGX vs. LSM | expand |
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. /Jarkk
On Mon, Jun 10, 2019 at 9:00 AM Jarkko Sakkinen <jarkko.sakkinen@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(¤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.
On 6/10/19 12:44 PM, Andy Lutomirski wrote: > On Mon, Jun 10, 2019 at 9:00 AM Jarkko Sakkinen > <jarkko.sakkinen@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(¤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()?
diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c index ef5c2ce0f37b..44b2d73de7c3 100644 --- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c +++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c @@ -577,6 +577,44 @@ static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr, return ret; } +static int sgx_encl_page_copy(void *dst, unsigned long src, unsigned long prot) +{ + struct vm_area_struct *vma; + int ret; + + if (!(prot & VM_EXEC)) + return 0; + + /* Hold mmap_sem across copy_from_user() to avoid a TOCTOU race. */ + down_read(¤t->mm->mmap_sem); + + vma = find_vma(current->mm, src); + if (!vma) { + ret = -EFAULT; + 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; +} + /** * sgx_ioc_enclave_add_page - handler for %SGX_IOC_ENCLAVE_ADD_PAGE * @@ -616,13 +654,12 @@ static long sgx_ioc_enclave_add_page(struct file *filep, unsigned int cmd, data = kmap(data_page); - if (copy_from_user((void *)data, (void __user *)addp->src, PAGE_SIZE)) { - ret = -EFAULT; - goto out; - } - prot = addp->flags & (PROT_READ | PROT_WRITE | PROT_EXEC); + ret = sgx_encl_page_copy(data, addp->src, prot); + if (ret) + goto out; + ret = sgx_encl_add_page(encl, addp->addr, data, &secinfo, addp->mrmask, prot); if (ret)
Do not allow an enclave page to be mapped with PROT_EXEC if the source vma does not have VM_MAYEXEC. This effectively enforces noexec as do_mmap() clears VM_MAYEXEC if the vma is being loaded from a noexec path, i.e. prevents executing a file by loading it into an enclave. Checking noexec indirectly by way of VM_MAYEXEC naturally handles any other cases that clear VM_MAYEXEC to deny execute permissions. Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- arch/x86/kernel/cpu/sgx/driver/ioctl.c | 47 +++++++++++++++++++++++--- 1 file changed, 42 insertions(+), 5 deletions(-)