Re: [RFC PATCH v3 23/27] KVM: VMX: Add SGX ENCLS[ECREATE] handler to enforce CPUID restrictions

From: "Edgecombe, Rick P" <rick.p.edgecombe@intel.com>
To: "linux-sgx@vger.kernel.org" <linux-sgx@vger.kernel.org>,
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"Huang, Kai" <kai.huang@intel.com>,
	"x86@kernel.org" <x86@kernel.org>
Cc: "Huang, Haitao" <haitao.huang@intel.com>,
	"luto@kernel.org" <luto@kernel.org>,
	"jarkko@kernel.org" <jarkko@kernel.org>,
	"seanjc@google.com" <seanjc@google.com>,
	"Hansen, Dave" <dave.hansen@intel.com>,
	"vkuznets@redhat.com" <vkuznets@redhat.com>,
	"bp@alien8.de" <bp@alien8.de>,
	"mingo@redhat.com" <mingo@redhat.com>,
	"tglx@linutronix.de" <tglx@linutronix.de>,
	"hpa@zytor.com" <hpa@zytor.com>,
	"pbonzini@redhat.com" <pbonzini@redhat.com>,
	"joro@8bytes.org" <joro@8bytes.org>,
	"wanpengli@tencent.com" <wanpengli@tencent.com>,
	"jmattson@google.com" <jmattson@google.com>
Subject: Re: [RFC PATCH v3 23/27] KVM: VMX: Add SGX ENCLS[ECREATE] handler to enforce CPUID restrictions
Date: Wed, 3 Feb 2021 00:52:01 +0000	[thread overview]
Message-ID: <f60226157935d2bbc20958e6eae7c3532b72f7a3.camel@intel.com> (raw)
In-Reply-To: <d68c01baed78f859ac5fce4519646fc8a356c77d.1611634586.git.kai.huang@intel.com>

On Tue, 2021-01-26 at 22:31 +1300, Kai Huang wrote:
> +static int handle_encls_ecreate(struct kvm_vcpu *vcpu)
> +{
> +       unsigned long a_hva, m_hva, x_hva, s_hva, secs_hva;
> +       struct kvm_cpuid_entry2 *sgx_12_0, *sgx_12_1;
> +       gpa_t metadata_gpa, contents_gpa, secs_gpa;
> +       struct sgx_pageinfo pageinfo;
> +       gva_t pageinfo_gva, secs_gva;
> +       u64 attributes, xfrm, size;
> +       struct x86_exception ex;
> +       u8 max_size_log2;
> +       u32 miscselect;
> +       int trapnr, r;
> +
> +       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);
> +               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;
> +       }
> +
> +       /*
> +        * Verify alignment early.  This conveniently avoids having
> to worry
> +        * about page splits on userspace addresses.
> +        */
> +       if (!IS_ALIGNED(pageinfo.metadata, 64) ||
> +           !IS_ALIGNED(pageinfo.contents, 4096)) {
> +               kvm_inject_gp(vcpu, 0);
> +               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, pageinfo.metadata, false,
> &metadata_gpa) ||
> +           sgx_gva_to_gpa(vcpu, pageinfo.contents, false,
> &contents_gpa) ||
> +           sgx_gva_to_gpa(vcpu, secs_gva, true, &secs_gpa))
> +               return 1;
> +

Do pageinfo.metadata and pageinfo.contents need cannonical checks here?

I was noticing the other day that the guest walker could access host
memory slightly outside of a memslot if it ever got passed a gva with
bits higher that the va bits. Or at least it appeared that way. I
didn't fully wade into the bit math because all callers from the guest
did cannonical checks.