From: Jordan Hand <jorhand@linux.microsoft.com>
To: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>,
linux-kernel@vger.kernel.org, x86@kernel.org,
linux-sgx@vger.kernel.org
Cc: akpm@linux-foundation.org, dave.hansen@intel.com,
sean.j.christopherson@intel.com, nhorman@redhat.com,
npmccallum@redhat.com, haitao.huang@intel.com,
andriy.shevchenko@linux.intel.com, tglx@linutronix.de,
kai.svahn@intel.com, bp@alien8.de, josh@joshtriplett.org,
luto@kernel.org, kai.huang@intel.com, rientjes@google.com,
cedric.xing@intel.com, puiterwijk@redhat.com,
linux-security-module@vger.kernel.org,
Suresh Siddha <suresh.b.siddha@intel.com>,
Haitao Huang <haitao.huang@linux.intel.com>
Subject: Re: [PATCH v26 10/22] x86/sgx: Linux Enclave Driver
Date: Tue, 18 Feb 2020 19:26:31 -0800 [thread overview]
Message-ID: <15074c16-4832-456d-dd12-af8548e46d6d@linux.microsoft.com> (raw)
In-Reply-To: <20200209212609.7928-11-jarkko.sakkinen@linux.intel.com>
I ran our validation tests for the Open Enclave SDK against this patch
set and came across a potential issue.
On 2/9/20 1:25 PM, Jarkko Sakkinen wrote:
> +/**
> + * sgx_encl_may_map() - Check if a requested VMA mapping is allowed
> + * @encl: an enclave
> + * @start: lower bound of the address range, inclusive
> + * @end: upper bound of the address range, exclusive
> + * @vm_prot_bits: requested protections of the address range
> + *
> + * Iterate through the enclave pages contained within [@start, @end) to verify
> + * the permissions requested by @vm_prot_bits do not exceed that of any enclave
> + * page to be mapped. Page addresses that do not have an associated enclave
> + * page are interpreted to zero permissions.
> + *
> + * Return:
> + * 0 on success,
> + * -EACCES if VMA permissions exceed enclave page permissions
> + */
> +int sgx_encl_may_map(struct sgx_encl *encl, unsigned long start,
> + unsigned long end, unsigned long vm_prot_bits)
> +{
> + unsigned long idx, idx_start, idx_end;
> + struct sgx_encl_page *page;
> +
> + /* PROT_NONE always succeeds. */
> + if (!vm_prot_bits)
> + return 0;
> +
> + idx_start = PFN_DOWN(start);
> + idx_end = PFN_DOWN(end - 1);
> +
> + for (idx = idx_start; idx <= idx_end; ++idx) {
> + mutex_lock(&encl->lock);
> + page = radix_tree_lookup(&encl->page_tree, idx);
> + mutex_unlock(&encl->lock);
> +
> + if (!page || (~page->vm_max_prot_bits & vm_prot_bits))
> + return -EACCES;
> + }
> +
> + return 0;
> +}
> +static struct sgx_encl_page *sgx_encl_page_alloc(struct sgx_encl *encl,
> + unsigned long offset,
> + u64 secinfo_flags)
> +{
> + struct sgx_encl_page *encl_page;
> + unsigned long prot;
> +
> + encl_page = kzalloc(sizeof(*encl_page), GFP_KERNEL);
> + if (!encl_page)
> + return ERR_PTR(-ENOMEM);
> +
> + encl_page->desc = encl->base + offset;
> + encl_page->encl = encl;
> +
> + prot = _calc_vm_trans(secinfo_flags, SGX_SECINFO_R, PROT_READ) |
> + _calc_vm_trans(secinfo_flags, SGX_SECINFO_W, PROT_WRITE) |
> + _calc_vm_trans(secinfo_flags, SGX_SECINFO_X, PROT_EXEC);
> +
> + /*
> + * TCS pages must always RW set for CPU access while the SECINFO
> + * permissions are *always* zero - the CPU ignores the user provided
> + * values and silently overwrites them with zero permissions.
> + */
> + if ((secinfo_flags & SGX_SECINFO_PAGE_TYPE_MASK) == SGX_SECINFO_TCS)
> + prot |= PROT_READ | PROT_WRITE;
> +
> + /* Calculate maximum of the VM flags for the page. */
> + encl_page->vm_max_prot_bits = calc_vm_prot_bits(prot, 0);
During mprotect (in mm/mprotect.c line 525) the following checks if
READ_IMPLIES_EXECUTE and a PROT_READ is being requested. If so and
VM_MAYEXEC is set, it also adds PROT_EXEC to the request.
if (rier && (vma->vm_flags & VM_MAYEXEC))
prot |= PROT_EXEC;
But if we look at sgx_encl_page_alloc(), we see vm_max_prot_bits is set
without taking VM_MAYEXEC into account:
encl_page->vm_max_prot_bits = calc_vm_prot_bits(prot, 0);
sgx_encl_may_map() checks that the requested protection can be added with:
if (!page || (~page->vm_max_prot_bits & vm_prot_bits))
return -EACCESS
This means that for any process where READ_IMPLIES_EXECUTE is set and
page where (vma->vm_flags & VM_MAYEXEC) == true, mmap/mprotect calls to
that request PROT_READ on a page that was not added with PROT_EXEC will
fail.
- Jordan
next prev parent reply other threads:[~2020-02-19 3:26 UTC|newest]
Thread overview: 55+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-02-09 21:25 [PATCH v26 00/22] Intel SGX foundations Jarkko Sakkinen
2020-02-09 21:25 ` [PATCH v26 01/22] x86/cpufeatures: x86/msr: Add Intel SGX hardware bits Jarkko Sakkinen
2020-02-09 21:25 ` [PATCH v26 02/22] x86/cpufeatures: x86/msr: Intel SGX Launch Control " Jarkko Sakkinen
2020-02-09 21:25 ` [PATCH v26 03/22] x86/mm: x86/sgx: Signal SIGSEGV with PF_SGX Jarkko Sakkinen
2020-02-09 21:25 ` [PATCH v26 04/22] x86/sgx: Add SGX microarchitectural data structures Jarkko Sakkinen
2020-02-09 21:25 ` [PATCH v26 05/22] x86/sgx: Add wrappers for ENCLS leaf functions Jarkko Sakkinen
2020-02-09 21:25 ` [PATCH v26 06/22] x86/cpu/intel: Detect SGX supprt Jarkko Sakkinen
2020-02-12 16:57 ` Sean Christopherson
2020-02-13 18:04 ` Jarkko Sakkinen
2020-02-09 21:25 ` [PATCH v26 07/22] x86/sgx: Enumerate and track EPC sections Jarkko Sakkinen
2020-02-09 21:25 ` [PATCH v26 08/22] x86/sgx: Add functions to allocate and free EPC pages Jarkko Sakkinen
2020-02-09 21:25 ` [PATCH v26 09/22] mm: Introduce vm_ops->may_mprotect() Jarkko Sakkinen
2020-02-09 21:25 ` [PATCH v26 10/22] x86/sgx: Linux Enclave Driver Jarkko Sakkinen
2020-02-13 13:59 ` Jethro Beekman
2020-02-13 18:07 ` Sean Christopherson
2020-02-14 9:24 ` Jethro Beekman
2020-02-14 17:11 ` Sean Christopherson
2020-02-14 17:40 ` Andy Lutomirski
2020-02-14 17:52 ` Sean Christopherson
2020-02-15 16:56 ` Andy Lutomirski
2020-02-18 22:12 ` Sean Christopherson
2020-02-15 18:05 ` Dr. Greg
2020-02-15 7:32 ` Jarkko Sakkinen
2020-02-15 7:35 ` Jarkko Sakkinen
2020-02-19 3:26 ` Jordan Hand [this message]
2020-02-20 18:13 ` Sean Christopherson
2020-02-20 18:33 ` Jordan Hand
2020-02-20 18:48 ` Sean Christopherson
2020-02-20 22:16 ` Jarkko Sakkinen
2020-02-21 0:11 ` Jordan Hand
2020-02-21 12:53 ` Jarkko Sakkinen
2020-02-21 0:32 ` Andy Lutomirski
2020-02-21 13:01 ` Jarkko Sakkinen
2020-02-20 18:51 ` Andy Lutomirski
2020-02-20 19:15 ` Sean Christopherson
2020-02-20 22:10 ` Jarkko Sakkinen
2020-02-09 21:25 ` [PATCH v26 11/22] selftests/x86: Recurse into subdirectories Jarkko Sakkinen
2020-02-09 21:25 ` [PATCH v26 12/22] selftests/x86: Add a selftest for SGX Jarkko Sakkinen
2020-02-09 21:26 ` [PATCH v26 13/22] x86/sgx: Add provisioning Jarkko Sakkinen
2020-02-13 10:49 ` Jethro Beekman
2020-02-15 7:25 ` Jarkko Sakkinen
2020-02-09 21:26 ` [PATCH v26 14/22] x86/sgx: Add a page reclaimer Jarkko Sakkinen
2020-02-09 21:26 ` [PATCH v26 15/22] x86/sgx: ptrace() support for the SGX driver Jarkko Sakkinen
2020-02-09 21:26 ` [PATCH v26 16/22] x86/vdso: Add support for exception fixup in vDSO functions Jarkko Sakkinen
2020-02-09 21:26 ` [PATCH v26 17/22] x86/fault: Add helper function to sanitize error code Jarkko Sakkinen
2020-02-09 21:26 ` [PATCH v26 18/22] x86/traps: Attempt to fixup exceptions in vDSO before signaling Jarkko Sakkinen
2020-02-09 21:26 ` [PATCH v26 19/22] x86/vdso: Add __vdso_sgx_enter_enclave() to wrap SGX enclave transitions Jarkko Sakkinen
2020-02-13 13:29 ` Jethro Beekman
2020-02-15 7:26 ` Jarkko Sakkinen
2020-02-09 21:26 ` [PATCH v26 20/22] selftests/x86: Add vDSO selftest for SGX Jarkko Sakkinen
2020-02-09 21:26 ` [PATCH v26 21/22] x86/sgx: Update MAINTAINERS Jarkko Sakkinen
2020-02-09 21:26 ` [PATCH v26 22/22] docs: x86/sgx: Document SGX micro architecture and kernel internals Jarkko Sakkinen
2020-02-22 4:13 ` Randy Dunlap
2020-02-23 17:04 ` Jarkko Sakkinen
2020-02-23 17:05 ` Jarkko Sakkinen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=15074c16-4832-456d-dd12-af8548e46d6d@linux.microsoft.com \
--to=jorhand@linux.microsoft.com \
--cc=akpm@linux-foundation.org \
--cc=andriy.shevchenko@linux.intel.com \
--cc=bp@alien8.de \
--cc=cedric.xing@intel.com \
--cc=dave.hansen@intel.com \
--cc=haitao.huang@intel.com \
--cc=haitao.huang@linux.intel.com \
--cc=jarkko.sakkinen@linux.intel.com \
--cc=josh@joshtriplett.org \
--cc=kai.huang@intel.com \
--cc=kai.svahn@intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-security-module@vger.kernel.org \
--cc=linux-sgx@vger.kernel.org \
--cc=luto@kernel.org \
--cc=nhorman@redhat.com \
--cc=npmccallum@redhat.com \
--cc=puiterwijk@redhat.com \
--cc=rientjes@google.com \
--cc=sean.j.christopherson@intel.com \
--cc=suresh.b.siddha@intel.com \
--cc=tglx@linutronix.de \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).