From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Thu, 03 Oct 2019 02:21:34 -0700 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 8B64B10CC1E3; Thu, 3 Oct 2019 09:21:33 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-154.rdu2.redhat.com [10.10.120.154]) by smtp.corp.redhat.com (Postfix) with ESMTP id F420A5C22C; Thu, 3 Oct 2019 09:21:27 +0000 (UTC) Subject: Re: [edk2-devel] [RFC PATCH v2 38/44] UefiCpuPkg: Allow AP booting under SEV-ES To: "Lendacky, Thomas" , "devel@edk2.groups.io" Cc: Jordan Justen , Ard Biesheuvel , Michael D Kinney , Liming Gao , Eric Dong , Ray Ni , "Singh, Brijesh" , =?UTF-8?Q?Philippe_Mathieu-Daud=c3=a9?= References: <81e310d1f2929f839cd166d1c7de6694220743b6.1568922729.git.thomas.lendacky@amd.com> <284e15f0-25ee-bb69-dcd1-09e146346c69@redhat.com> From: "Laszlo Ersek" Message-ID: Date: Thu, 3 Oct 2019 11:21:27 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.6.2 (mx1.redhat.com [10.5.110.65]); Thu, 03 Oct 2019 09:21:33 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 10/02/19 19:58, Lendacky, Thomas wrote: > On 10/2/19 10:15 AM, Laszlo Ersek via Groups.Io wrote: >> Adding Phil. >> >> I'm looking at this patch only because one thing caught my attention in >> the previous one, "OvmfPkg: Add support for SEV-ES AP reset vector >> re-directing": >> >> On 09/19/19 21:53, Lendacky, Thomas wrote: >>> From: Tom Lendacky >>> >>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2198 >>> >>> Typically, an AP is booted using the INIT-SIPI-SIPI sequence. This >>> sequence is intercepted by the hypervisor, which sets the AP's registers >>> to the values requested by the sequence. At that point, the hypervisor can >>> start the AP, which will then begin execution at the appropriate location. >>> >>> Under SEV-ES, AP booting presents some challenges since the hypervisor is >>> not allowed to alter the AP's register state. In this situation, we have >>> to distinguish between the AP's first boot and AP's subsequent boots. >>> >>> First boot: >>> Once the AP's register state has been defined (which is before the guest >>> is first booted) it cannot be altered. Should the hypervisor attempt to >>> alter the register state, the change would be detected by the hardware >>> and the VMRUN instruction would fail. Given this, the first boot for the >>> AP is required to begin execution with this initial register state, which >>> is typically the reset vector. This prevents the BSP from directing the >>> AP startup location through the INIT-SIPI-SIPI sequence. >>> >>> To work around this, provide a four-byte field at offset 0xffffffd0 that >>> can contain an IP / CS register combination, that if non-zero, causes >>> the AP to perform a far jump to that location instead of a near jump to >>> EarlyBspInitReal16. Before booting the AP for the first time, the BSP >>> should set the IP / CS value for the AP based on the value that would be >>> derived from the INIT-SIPI-SIPI sequence. >> >> I don't understand how this can work: the guest-phys address 0xffffffd0 >> is backed by read-only pflash in most OVMF deployments. >> >> In addition: >> >> [...] >> >>> @@ -1002,6 +1204,7 @@ WakeUpAP ( >>> CpuMpData->InitFlag != ApInitDone) { >>> ResetVectorRequired = TRUE; >>> AllocateResetVector (CpuMpData); >>> + AllocateSevEsAPMemory (CpuMpData); >>> FillExchangeInfoData (CpuMpData); >>> SaveLocalApicTimerSetting (CpuMpData); >>> } >>> @@ -1038,6 +1241,15 @@ WakeUpAP ( >>> } >>> } >>> if (ResetVectorRequired) { >>> + // >>> + // For SEV-ES, set the jump address for initial AP boot >>> + // >>> + if (CpuMpData->SevEsActive) { >>> + SEV_ES_AP_JMP_FAR *JmpFar = (SEV_ES_AP_JMP_FAR *)0xFFFFFFD0; >>> + >>> + JmpFar->ApStart.Rip = 0; >>> + JmpFar->ApStart.Segment = (UINT16) (ExchangeInfo->BufferStart >> 4); >>> + } >> >> Even if the address is backed by a single "unified" pflash, mapped r/w >> -- which we can call a "non-standard OVMF deployment" nowadays --, a >> normal store doesn't appear sufficient to me. The first write to pflash >> will flip it to "programming mode", and the values stored are supposed >> to be pflash commands (not just the raw data we intend to put in place). > > There is a corresponding patch in Qemu that does not set the > KVM_MEM_READONLY flag for the ROM when starting an SEV-ES guest, thus > allowing the MP library to update the location with desired address. Isn't that a price too high to pay? It seems to allow the memory mapped contents of the pflash chip to diverge from the host-side file, even if the QEMU command line specifies that the pflash chip is to be mapped r/o. With regard to SMM, I think this could even be considered a security problem -- if the pflash region is not marked as readonly, then the guest OS could write to it as well (with direct hardware access, like the code seen above). The write would not trap to QEMU, and QEMU couldn't prevent the write. The OS could use this to inject code into the pflash region (e.g., SEC). Although the host-side pflash file would not be modified, if the OS performed a warm reboot (or S3 cycle) afterwards, the code it injected into the flash region would be executed with firmware privileges. AMD Publication #56421 states that SMM is currently out of scope for SEV-ES, so I don't think there's a problem right now; I'm just generally concerned that here we're enabling something I've always considered a big no-no for SMM builds. I still have to read your next response though (to my suggestion to use a fixed RAM address, I believe). Thanks! Laszlo