From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-3.8 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 9AD32C433DB for ; Wed, 3 Feb 2021 23:46:31 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 6792B64E08 for ; Wed, 3 Feb 2021 23:46:31 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233258AbhBCXqa (ORCPT ); Wed, 3 Feb 2021 18:46:30 -0500 Received: from mga09.intel.com ([134.134.136.24]:22083 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232458AbhBCXq3 (ORCPT ); Wed, 3 Feb 2021 18:46:29 -0500 IronPort-SDR: uJlE3O+J2P5WqmwZehxz+4mHQHNVGJe/ZIDxsOGv1kf8Fb02cfk8I4QAQiPVwuY5Mbyb+5/X7c i79wb1nflMyg== X-IronPort-AV: E=McAfee;i="6000,8403,9884"; a="181280703" X-IronPort-AV: E=Sophos;i="5.79,399,1602572400"; d="scan'208";a="181280703" Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga102.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 03 Feb 2021 15:45:43 -0800 IronPort-SDR: yE6xGgJSCqIzKBLtqWisJrZaAzfH3JnZvrLFJkPMBnP5SgTBvsNqKW7F2QSjCXCGLoZHcRGjl9 tywWW1g9xCXQ== X-IronPort-AV: E=Sophos;i="5.79,399,1602572400"; d="scan'208";a="433668893" Received: from rvchebia-mobl.amr.corp.intel.com ([10.251.7.104]) by orsmga001-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 03 Feb 2021 15:45:38 -0800 Message-ID: Subject: Re: [RFC PATCH v3 23/27] KVM: VMX: Add SGX ENCLS[ECREATE] handler to enforce CPUID restrictions From: Kai Huang To: Sean Christopherson Cc: "Edgecombe, Rick P" , "linux-sgx@vger.kernel.org" , "kvm@vger.kernel.org" , "x86@kernel.org" , "Huang, Haitao" , "luto@kernel.org" , "jarkko@kernel.org" , "Hansen, Dave" , "vkuznets@redhat.com" , "bp@alien8.de" , "mingo@redhat.com" , "tglx@linutronix.de" , "hpa@zytor.com" , "pbonzini@redhat.com" , "joro@8bytes.org" , "wanpengli@tencent.com" , "jmattson@google.com" Date: Thu, 04 Feb 2021 12:45:34 +1300 In-Reply-To: References: Content-Type: text/plain; charset="UTF-8" User-Agent: Evolution 3.38.3 (3.38.3-1.fc33) MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org On Wed, 2021-02-03 at 15:36 -0800, Sean Christopherson wrote: > On Thu, Feb 04, 2021, Kai Huang wrote: > > On Wed, 2021-02-03 at 11:36 -0800, Sean Christopherson wrote: > > > On Wed, Feb 03, 2021, Edgecombe, Rick P wrote: > > > > On Tue, 2021-01-26 at 22:31 +1300, Kai Huang wrote: > > > > > +       /* Exit to userspace if copying from a host userspace address > > > > > fails. */ > > > > > +       if (sgx_read_hva(vcpu, m_hva, &miscselect, > > > > > sizeof(miscselect)) || > > > > > +           sgx_read_hva(vcpu, a_hva, &attributes, > > > > > sizeof(attributes)) || > > > > > +           sgx_read_hva(vcpu, x_hva, &xfrm, sizeof(xfrm)) || > > > > > +           sgx_read_hva(vcpu, s_hva, &size, sizeof(size))) > > > > > +               return 0; > > > > > + > > > > > +       /* 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; > > > > > +       } > > > > > > > > Don't you need to deep copy the pageinfo.contents struct as well? > > > > Otherwise the guest could change these after they were checked. > > > > > > > > But it seems it is checked by the HW and something is caught that would > > > > inject a GP anyway? Can you elaborate on the importance of these > > > > checks? > > > > > > Argh, yes. These checks are to allow migration between systems with different > > > SGX capabilities, and more importantly to prevent userspace from doing an end > > > around on the restricted access to PROVISIONKEY. > > > > > > IIRC, earlier versions did do a deep copy, but then I got clever. Anyways, yeah, > > > sadly the entire pageinfo.contents page will need to be copied. > > > > I don't fully understand the problem. Are you worried about contents being updated by > > other vcpus during the trap?  > > > > And I don't see how copy can avoid this problem. Even you do copy, the content can > > still be modified afterwards, correct? So what's the point of copying? > > The goal isn't correctness, it's to prevent a TOCTOU bug. E.g. the guest could > do ECREATE w/ SECS.SGX_ATTR_PROVISIONKEY=0, and simultaneously set > SGX_ATTR_PROVISIONKEY to bypass the above check. Oh ok. Agreed. However, such attack would require precise timing. Not sure whether it is feasible in practice. > > > Looks a better solution is to kick all vcpus and put them into block state > > while KVM is doing ENCLS for guest. > > No. (a) it won't work, as the memory is writable from host userspace. (b) that > does not scale, at all. Good point. Agreed.