From: Kai Huang <kai.huang@intel.com>
To: Sean Christopherson <seanjc@google.com>
Cc: kvm@vger.kernel.org, x86@kernel.org, linux-sgx@vger.kernel.org,
linux-kernel@vger.kernel.org, jarkko@kernel.org, luto@kernel.org,
dave.hansen@intel.com, rick.p.edgecombe@intel.com,
haitao.huang@intel.com, pbonzini@redhat.com, bp@alien8.de,
tglx@linutronix.de, mingo@redhat.com, hpa@zytor.com,
jmattson@google.com, joro@8bytes.org, vkuznets@redhat.com,
wanpengli@tencent.com
Subject: Re: [PATCH 21/25] KVM: VMX: Add SGX ENCLS[ECREATE] handler to enforce CPUID restrictions
Date: Tue, 02 Mar 2021 21:44:36 +1300 [thread overview]
Message-ID: <247ec46adddb6f043c15d68f52f83efe7b9db3dd.camel@intel.com> (raw)
In-Reply-To: <YD0iT3c8FMPGUNjo@google.com>
On Mon, 2021-03-01 at 09:20 -0800, Sean Christopherson wrote:
> On Mon, Mar 01, 2021, Kai Huang wrote:
> > +static int handle_encls_ecreate(struct kvm_vcpu *vcpu)
> > +{
> > + struct kvm_cpuid_entry2 *sgx_12_0, *sgx_12_1;
> > + gva_t pageinfo_gva, secs_gva;
> > + gva_t metadata_gva, contents_gva;
> > + gpa_t metadata_gpa, contents_gpa, secs_gpa;
> > + unsigned long metadata_hva, contents_hva, secs_hva;
> > + struct sgx_pageinfo pageinfo;
> > + struct sgx_secs *contents;
> > + u64 attributes, xfrm, size;
> > + u32 miscselect;
> > + struct x86_exception ex;
> > + u8 max_size_log2;
> > + int trapnr, r;
> > +
>
> (see below)
>
> --- cut here --- >8
>
> > + sgx_12_0 = kvm_find_cpuid_entry(vcpu, 0x12, 0);
> > + sgx_12_1 = kvm_find_cpuid_entry(vcpu, 0x12, 1);
> > + if (!sgx_12_0 || !sgx_12_1) {
> > + kvm_inject_gp(vcpu, 0);
>
> This should probably be an emulation failure. This code is reached iff SGX1 is
> enabled in the guest, userspace done messed up if they enabled SGX1 without
> defining CPUID.0x12.1. That also makes it more obvious that burying this in a
> helper after a bunch of other checks isn't wrong, i.e. KVM has already verified
> that SGX1 is enabled in the guest.
>
> > + return 1;
> > + }
>
> ---
>
> > +
> > + if (sgx_get_encls_gva(vcpu, kvm_rbx_read(vcpu), 32, 32, &pageinfo_gva) ||
> > + sgx_get_encls_gva(vcpu, kvm_rcx_read(vcpu), 4096, 4096, &secs_gva))
> > + return 1;
> > +
> > + /*
> > + * Copy the PAGEINFO to local memory, its pointers need to be
> > + * translated, i.e. we need to do a deep copy/translate.
> > + */
> > + r = kvm_read_guest_virt(vcpu, pageinfo_gva, &pageinfo,
> > + sizeof(pageinfo), &ex);
> > + if (r == X86EMUL_PROPAGATE_FAULT) {
> > + kvm_inject_emulated_page_fault(vcpu, &ex);
> > + return 1;
> > + } else if (r != X86EMUL_CONTINUE) {
> > + sgx_handle_emulation_failure(vcpu, pageinfo_gva, size);
> > + return 0;
> > + }
> > +
> > + if (sgx_get_encls_gva(vcpu, pageinfo.metadata, 64, 64, &metadata_gva) ||
> > + sgx_get_encls_gva(vcpu, pageinfo.contents, 4096, 4096,
> > + &contents_gva))
> > + return 1;
> > +
> > + /*
> > + * Translate the SECINFO, SOURCE and SECS pointers from GVA to GPA.
> > + * Resume the guest on failure to inject a #PF.
> > + */
> > + if (sgx_gva_to_gpa(vcpu, metadata_gva, false, &metadata_gpa) ||
> > + sgx_gva_to_gpa(vcpu, contents_gva, false, &contents_gpa) ||
> > + sgx_gva_to_gpa(vcpu, secs_gva, true, &secs_gpa))
> > + return 1;
> > +
> > + /*
> > + * ...and then to HVA. The order of accesses isn't architectural, i.e.
> > + * KVM doesn't have to fully process one address at a time. Exit to
> > + * userspace if a GPA is invalid.
> > + */
> > + if (sgx_gpa_to_hva(vcpu, metadata_gpa, &metadata_hva) ||
> > + sgx_gpa_to_hva(vcpu, contents_gpa, &contents_hva) ||
> > + sgx_gpa_to_hva(vcpu, secs_gpa, &secs_hva))
> > + return 0;
> > + /*
> > + * Copy contents into kernel memory to prevent TOCTOU attack. E.g. the
> > + * guest could do ECREATE w/ SECS.SGX_ATTR_PROVISIONKEY=0, and
> > + * simultaneously set SGX_ATTR_PROVISIONKEY to bypass the check to
> > + * enforce restriction of access to the PROVISIONKEY.
> > + */
> > + contents = (struct sgx_secs *)__get_free_page(GFP_KERNEL);
> > + if (!contents)
> > + return -ENOMEM;
>
> --- cut here --- >8
>
> > +
> > + /* Exit to userspace if copying from a host userspace address fails. */
> > + if (sgx_read_hva(vcpu, contents_hva, (void *)contents, PAGE_SIZE))
>
> This, and every failure path below, will leak 'contents'. The easiest thing is
> probably to wrap everything in "cut here" in a separate helper. The CPUID
> lookups can be , e.g.
>
> contents = (struct sgx_secs *)__get_free_page(GFP_KERNEL);
> if (!contents)
> return -ENOMEM;
>
> r = __handle_encls_ecreate(vcpu, &pageinfo, secs);
>
> free_page((unsigned long)contents);
> return r;
>
> And then the helper can be everything below, plus the CPUID lookup:
>
> sgx_12_0 = kvm_find_cpuid_entry(vcpu, 0x12, 0);
> sgx_12_1 = kvm_find_cpuid_entry(vcpu, 0x12, 1);
> if (!sgx_12_0 || !sgx_12_1) {
> vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
> vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION;
> vcpu->run->internal.ndata = 0;
> return 0;
> }
>
>
>
Hi Sean,
I ended up with below:
1) Copy contents is out of __handle_encls_ecreate(), since from my perspective it is
more reasonable split in this way logically, that when __handle_encls_ecreate() is
called, pageinfo is already ready, but policy enforcement still needs to be checked.
2) Finding sgx_12_0/1 is at first in __handle_encls_ecreate(), since provisionkey bit
check requires sgx_12_1->eax.
3) __handle_encls_ecreate() requires secs_gva since sgx_inject_fault() requires it.
Is it OK to you?
+static int __handle_encls_ecreate(struct kvm_vcpu *vcpu,
+ struct sgx_pageinfo *pageinfo,
+ unsigned long secs_hva,
+ gva_t secs_gva)
+{
+ struct sgx_secs *contents = (struct sgx_secs *)pageinfo->contents;
+ struct kvm_cpuid_entry2 *sgx_12_0, *sgx_12_1;
+ u64 attributes, xfrm, size;
+ u32 miscselect;
+ u8 max_size_log2;
+ int trapnr;
+
+ sgx_12_0 = kvm_find_cpuid_entry(vcpu, 0x12, 0);
+ sgx_12_1 = kvm_find_cpuid_entry(vcpu, 0x12, 1);
+ if (!sgx_12_0 || !sgx_12_1) {
+ vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
+ vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION;
+ vcpu->run->internal.ndata = 0;
+ return 0;
+ }
+
+ miscselect = contents->miscselect;
+ attributes = contents->attributes;
+ xfrm = contents->xfrm;
+ size = contents->size;
+
+ /* Enforce restriction of access to the PROVISIONKEY. */
+ if (!vcpu->kvm->arch.sgx_provisioning_allowed &&
+ (attributes & SGX_ATTR_PROVISIONKEY)) {
+ if (sgx_12_1->eax & SGX_ATTR_PROVISIONKEY)
+ pr_warn_once("KVM: SGX PROVISIONKEY advertised but not
allowed\n");
+ kvm_inject_gp(vcpu, 0);
+ return 1;
+ }
+
+ /* Enforce CPUID restrictions on MISCSELECT, ATTRIBUTES and XFRM. */
+ if ((u32)miscselect & ~sgx_12_0->ebx ||
+ (u32)attributes & ~sgx_12_1->eax ||
+ (u32)(attributes >> 32) & ~sgx_12_1->ebx ||
+ (u32)xfrm & ~sgx_12_1->ecx ||
+ (u32)(xfrm >> 32) & ~sgx_12_1->edx) {
+ kvm_inject_gp(vcpu, 0);
+ return 1;
+ }
+
+ /* Enforce CPUID restriction on max enclave size. */
+ max_size_log2 = (attributes & SGX_ATTR_MODE64BIT) ? sgx_12_0->edx >> 8 :
+ sgx_12_0->edx;
+ if (size >= BIT_ULL(max_size_log2))
+ kvm_inject_gp(vcpu, 0);
+
+ if (sgx_virt_ecreate(pageinfo, (void __user *)secs_hva, &trapnr))
+ return sgx_inject_fault(vcpu, secs_gva, trapnr);
+
+ return kvm_skip_emulated_instruction(vcpu);
+}
+
+static int handle_encls_ecreate(struct kvm_vcpu *vcpu)
+{
+ gva_t pageinfo_gva, secs_gva;
+ gva_t metadata_gva, contents_gva;
+ gpa_t metadata_gpa, contents_gpa, secs_gpa;
+ unsigned long metadata_hva, contents_hva, secs_hva;
+ struct sgx_pageinfo pageinfo;
+ struct sgx_secs *contents;
+ struct x86_exception ex;
+ int r;
+
+ if (sgx_get_encls_gva(vcpu, kvm_rbx_read(vcpu), 32, 32, &pageinfo_gva) ||
+ sgx_get_encls_gva(vcpu, kvm_rcx_read(vcpu), 4096, 4096, &secs_gva))
+ return 1;
+
+ /*
+ * Copy the PAGEINFO to local memory, its pointers need to be
+ * translated, i.e. we need to do a deep copy/translate.
+ */
+ r = kvm_read_guest_virt(vcpu, pageinfo_gva, &pageinfo,
+ sizeof(pageinfo), &ex);
+ if (r == X86EMUL_PROPAGATE_FAULT) {
+ kvm_inject_emulated_page_fault(vcpu, &ex);
+ return 1;
+ } else if (r != X86EMUL_CONTINUE) {
+ sgx_handle_emulation_failure(vcpu, pageinfo_gva,
+ sizeof(pageinfo));
+ return 0;
+ }
+
+ if (sgx_get_encls_gva(vcpu, pageinfo.metadata, 64, 64, &metadata_gva) ||
+ sgx_get_encls_gva(vcpu, pageinfo.contents, 4096, 4096,
+ &contents_gva))
+ return 1;
+
+ /*
+ * Translate the SECINFO, SOURCE and SECS pointers from GVA to GPA.
+ * Resume the guest on failure to inject a #PF.
+ */
+ if (sgx_gva_to_gpa(vcpu, metadata_gva, false, &metadata_gpa) ||
+ sgx_gva_to_gpa(vcpu, contents_gva, false, &contents_gpa) ||
+ sgx_gva_to_gpa(vcpu, secs_gva, true, &secs_gpa))
+ return 1;
+
+ /*
+ * ...and then to HVA. The order of accesses isn't architectural, i.e.
+ * KVM doesn't have to fully process one address at a time. Exit to
+ * userspace if a GPA is invalid.
+ */
+ if (sgx_gpa_to_hva(vcpu, metadata_gpa, &metadata_hva) ||
+ sgx_gpa_to_hva(vcpu, contents_gpa, &contents_hva) ||
+ sgx_gpa_to_hva(vcpu, secs_gpa, &secs_hva))
+ return 0;
+
+ /*
+ * Copy contents into kernel memory to prevent TOCTOU attack. E.g. the
+ * guest could do ECREATE w/ SECS.SGX_ATTR_PROVISIONKEY=0, and
+ * simultaneously set SGX_ATTR_PROVISIONKEY to bypass the check to
+ * enforce restriction of access to the PROVISIONKEY.
+ */
+ contents = (struct sgx_secs *)__get_free_page(GFP_KERNEL);
+ if (!contents)
+ return -ENOMEM;
+
+ /* Exit to userspace if copying from a host userspace address fails. */
+ if (sgx_read_hva(vcpu, contents_hva, (void *)contents, PAGE_SIZE)) {
+ free_page((unsigned long)contents);
+ return 0;
+ }
+
+ pageinfo.metadata = metadata_hva;
+ pageinfo.contents = (u64)contents;
+
+ r = __handle_encls_ecreate(vcpu, &pageinfo, secs_hva, secs_gva);
+
+ free_page((unsigned long)contents);
+
+ return r;
+}
next prev parent reply other threads:[~2021-03-02 21:48 UTC|newest]
Thread overview: 61+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-01 9:44 [PATCH 00/25] KVM SGX virtualization support Kai Huang
2021-03-01 9:44 ` [PATCH 01/25] x86/cpufeatures: Make SGX_LC feature bit depend on SGX bit Kai Huang
2021-03-01 9:44 ` [PATCH 02/25] x86/cpufeatures: Add SGX1 and SGX2 sub-features Kai Huang
2021-03-01 10:00 ` Borislav Petkov
2021-03-01 10:19 ` Kai Huang
2021-03-01 10:30 ` Borislav Petkov
2021-03-01 10:40 ` Kai Huang
2021-03-01 10:53 ` Borislav Petkov
2021-03-01 11:28 ` Kai Huang
2021-03-01 11:32 ` Borislav Petkov
2021-03-01 11:43 ` Kai Huang
2021-03-01 11:58 ` Borislav Petkov
2021-03-01 12:08 ` Kai Huang
2021-03-02 15:48 ` Haitao Huang
2021-03-02 15:58 ` Dave Hansen
2021-03-07 22:49 ` Kai Huang
2021-03-10 15:30 ` Jarkko Sakkinen
2021-03-02 16:02 ` Sean Christopherson
2021-03-02 17:53 ` Boris Petkov
2021-03-02 18:27 ` Kai Huang
2021-03-01 9:44 ` [PATCH 03/25] x86/sgx: Wipe out EREMOVE from sgx_free_epc_page() Kai Huang
2021-03-01 17:29 ` Sean Christopherson
2021-03-02 0:32 ` Kai Huang
2021-03-01 9:44 ` [PATCH 04/25] x86/sgx: Add SGX_CHILD_PRESENT hardware error code Kai Huang
2021-03-01 9:44 ` [PATCH 05/25] x86/sgx: Introduce virtual EPC for use by KVM guests Kai Huang
2021-03-01 16:21 ` Sean Christopherson
2021-03-02 0:33 ` Kai Huang
2021-03-01 9:45 ` [PATCH 06/25] x86/cpu/intel: Allow SGX virtualization without Launch Control support Kai Huang
2021-03-05 17:29 ` Borislav Petkov
2021-03-07 23:50 ` Kai Huang
2021-03-08 0:19 ` Kai Huang
2021-03-10 15:32 ` Jarkko Sakkinen
2021-03-01 9:45 ` [PATCH 07/25] x86/sgx: Initialize virtual EPC driver even when SGX driver is disabled Kai Huang
2021-03-01 9:45 ` [PATCH 08/25] x86/sgx: Expose SGX architectural definitions to the kernel Kai Huang
2021-03-01 9:45 ` [PATCH 09/25] x86/sgx: Move ENCLS leaf definitions to sgx.h Kai Huang
2021-03-01 16:25 ` Sean Christopherson
2021-03-02 0:34 ` Kai Huang
2021-03-01 9:45 ` [PATCH 10/25] x86/sgx: Add SGX2 ENCLS leaf definitions (EAUG, EMODPR and EMODT) Kai Huang
2021-03-01 9:45 ` [PATCH 11/25] x86/sgx: Add encls_faulted() helper Kai Huang
2021-03-01 9:45 ` [PATCH 12/25] x86/sgx: Add helper to update SGX_LEPUBKEYHASHn MSRs Kai Huang
2021-03-01 16:57 ` Sean Christopherson
2021-03-02 0:34 ` Kai Huang
2021-03-01 9:45 ` [PATCH 13/25] x86/sgx: Add helpers to expose ECREATE and EINIT to KVM Kai Huang
2021-03-01 9:45 ` [PATCH 14/25] x86/sgx: Move provisioning device creation out of SGX driver Kai Huang
2021-03-01 9:45 ` [PATCH 15/25] KVM: x86: Export kvm_mmu_gva_to_gpa_{read,write}() for SGX (VMX) Kai Huang
2021-03-01 9:45 ` [PATCH 16/25] KVM: x86: Define new #PF SGX error code bit Kai Huang
2021-03-01 9:45 ` [PATCH 17/25] KVM: x86: Add support for reverse CPUID lookup of scattered features Kai Huang
2021-03-01 9:45 ` [PATCH 18/25] KVM: x86: Add reverse-CPUID lookup support for scattered SGX features Kai Huang
2021-03-01 9:45 ` [PATCH 19/25] KVM: VMX: Add basic handling of VM-Exit from SGX enclave Kai Huang
2021-03-01 16:52 ` Sean Christopherson
2021-03-02 0:50 ` Kai Huang
2021-03-01 9:45 ` [PATCH 20/25] KVM: VMX: Frame in ENCLS handler for SGX virtualization Kai Huang
2021-03-01 9:45 ` [PATCH 21/25] KVM: VMX: Add SGX ENCLS[ECREATE] handler to enforce CPUID restrictions Kai Huang
2021-03-01 17:20 ` Sean Christopherson
2021-03-02 0:54 ` Kai Huang
2021-03-02 5:34 ` Kai Huang
2021-03-02 8:44 ` Kai Huang [this message]
2021-03-01 9:45 ` [PATCH 22/25] KVM: VMX: Add emulation of SGX Launch Control LE hash MSRs Kai Huang
2021-03-01 9:45 ` [PATCH 23/25] KVM: VMX: Add ENCLS[EINIT] handler to support SGX Launch Control (LC) Kai Huang
2021-03-01 9:45 ` [PATCH 24/25] KVM: VMX: Enable SGX virtualization for SGX1, SGX2 and LC Kai Huang
2021-03-01 9:46 ` [PATCH 25/25] KVM: x86: Add capability to grant VM access to privileged SGX attribute Kai Huang
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=247ec46adddb6f043c15d68f52f83efe7b9db3dd.camel@intel.com \
--to=kai.huang@intel.com \
--cc=bp@alien8.de \
--cc=dave.hansen@intel.com \
--cc=haitao.huang@intel.com \
--cc=hpa@zytor.com \
--cc=jarkko@kernel.org \
--cc=jmattson@google.com \
--cc=joro@8bytes.org \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-sgx@vger.kernel.org \
--cc=luto@kernel.org \
--cc=mingo@redhat.com \
--cc=pbonzini@redhat.com \
--cc=rick.p.edgecombe@intel.com \
--cc=seanjc@google.com \
--cc=tglx@linutronix.de \
--cc=vkuznets@redhat.com \
--cc=wanpengli@tencent.com \
--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).