From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [207.211.31.120]) by mx.groups.io with SMTP id smtpd.web12.18485.1582535464915035179 for ; Mon, 24 Feb 2020 01:11:05 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=efql0MO5; spf=pass (domain: redhat.com, ip: 207.211.31.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1582535464; h=from:from:reply-to:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=6hZk+NrdK+DKWpHNnMRw8bnRynqLoj8iSQr7rNJ5MT0=; b=efql0MO5rD4j0jObrgUWN137Aj3cHSmz/beKZPbdAxOP5cJxvyCQYrSe3KYOcmxwMidojC 5TYwSojppwpNDYBpqEBDsUXaN44j5yy5DlNSY7Ms0uxjfqkDPt6kNbsv1NDJXVx7Sl54kM DXGXWn8M18Usf/W0mFKEUaKhErxwqQ4= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-9-1ostfYYUM_OzsKMBHGc0Cw-1; Mon, 24 Feb 2020 04:10:57 -0500 X-MC-Unique: 1ostfYYUM_OzsKMBHGc0Cw-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 34D89800D53; Mon, 24 Feb 2020 09:10:56 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-135.ams2.redhat.com [10.36.116.135]) by smtp.corp.redhat.com (Postfix) with ESMTP id 33C8E5D9CD; Mon, 24 Feb 2020 09:10:54 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 12/16] OvmfPkg/CpuHotplugSmm: introduce First SMI Handler for hot-added CPUs From: "Laszlo Ersek" To: edk2-devel-groups-io Cc: Ard Biesheuvel , Igor Mammedov , Jiewen Yao , Jordan Justen , Michael Kinney , =?UTF-8?Q?Philippe_Mathieu-Daud=c3=a9?= Reply-To: devel@edk2.groups.io, lersek@redhat.com References: <20200223172537.28464-1-lersek@redhat.com> <20200223172537.28464-13-lersek@redhat.com> Message-ID: <111145fc-be3d-2a9a-a126-c14345a8a8a4@redhat.com> Date: Mon, 24 Feb 2020 10:10:53 +0100 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: <20200223172537.28464-13-lersek@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 02/23/20 18:25, Laszlo Ersek wrote: > Implement the First SMI Handler for hot-added CPUs, in NASM. >=20 > Add the interfacing C-language function that the SMM Monarch calls. This > function launches and coordinates SMBASE relocation for a hot-added CPU. >=20 > Cc: Ard Biesheuvel > Cc: Igor Mammedov > Cc: Jiewen Yao > Cc: Jordan Justen > Cc: Michael Kinney > Cc: Philippe Mathieu-Daud=C3=A9 > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3D1512 > Signed-off-by: Laszlo Ersek > --- > OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf | 4 + > OvmfPkg/CpuHotplugSmm/FirstSmiHandlerContext.h | 41 ++++++ > OvmfPkg/CpuHotplugSmm/Smbase.h | 14 ++ > OvmfPkg/CpuHotplugSmm/FirstSmiHandler.nasm | 149 +++++++++++++++++++= + > OvmfPkg/CpuHotplugSmm/Smbase.c | 142 +++++++++++++++++++ > 5 files changed, 350 insertions(+) >=20 > diff --git a/OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf b/OvmfPkg/CpuHotplug= Smm/CpuHotplugSmm.inf > index bf4162299c7c..04322b0d7855 100644 > --- a/OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf > +++ b/OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf > @@ -5,56 +5,60 @@ > # > # SPDX-License-Identifier: BSD-2-Clause-Patent > ## > =20 > [Defines] > INF_VERSION =3D 1.29 > PI_SPECIFICATION_VERSION =3D 0x00010046 #= PI-1.7.0 > BASE_NAME =3D CpuHotplugSmm > FILE_GUID =3D 84EEA114-C6BE-4445-8F90-51D97863E363 > MODULE_TYPE =3D DXE_SMM_DRIVER > ENTRY_POINT =3D CpuHotplugEntry > =20 > # > # The following information is for reference only and not required by th= e build > # tools. > # > # VALID_ARCHITECTURES =3D IA32 X64 > # > =20 > [Sources] > ApicId.h > CpuHotplug.c > + FirstSmiHandler.nasm > + FirstSmiHandlerContext.h > PostSmmPen.nasm > QemuCpuhp.c > QemuCpuhp.h > Smbase.c > Smbase.h > =20 > [Packages] > MdePkg/MdePkg.dec > OvmfPkg/OvmfPkg.dec > UefiCpuPkg/UefiCpuPkg.dec > =20 > [LibraryClasses] > BaseLib > BaseMemoryLib > DebugLib > + LocalApicLib > MmServicesTableLib > PcdLib > SafeIntLib > + SynchronizationLib > UefiDriverEntryPoint > =20 > [Protocols] > gEfiMmCpuIoProtocolGuid ## C= ONSUMES > gEfiSmmCpuServiceProtocolGuid ## C= ONSUMES > =20 > [Pcd] > gUefiCpuPkgTokenSpaceGuid.PcdCpuHotPlugDataAddress ## C= ONSUMES > gUefiOvmfPkgTokenSpaceGuid.PcdQ35SmramAtDefaultSmbase ## C= ONSUMES > =20 > [FeaturePcd] > gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire ## C= ONSUMES > =20 > [Depex] > gEfiMmCpuIoProtocolGuid AND > gEfiSmmCpuServiceProtocolGuid > diff --git a/OvmfPkg/CpuHotplugSmm/FirstSmiHandlerContext.h b/OvmfPkg/Cpu= HotplugSmm/FirstSmiHandlerContext.h > new file mode 100644 > index 000000000000..7806a5b2ad03 > --- /dev/null > +++ b/OvmfPkg/CpuHotplugSmm/FirstSmiHandlerContext.h > @@ -0,0 +1,41 @@ > +/** @file > + Define the FIRST_SMI_HANDLER_CONTEXT structure, which is an exchange a= rea > + between the SMM Monarch and the hot-added CPU, for relocating the SMBA= SE of > + the hot-added CPU. > + > + Copyright (c) 2020, Red Hat, Inc. > + > + SPDX-License-Identifier: BSD-2-Clause-Patent > +**/ > + > +#ifndef FIRST_SMI_HANDLER_CONTEXT_H_ > +#define FIRST_SMI_HANDLER_CONTEXT_H_ > + > +// > +// The following structure is used to communicate between the SMM Monarc= h > +// (running the root MMI handler) and the hot-added CPU (handling its fi= rst > +// SMI). It is placed at SMM_DEFAULT_SMBASE, which is in SMRAM under QEM= U's > +// "SMRAM at default SMBASE" feature. > +// > +#pragma pack (1) > +typedef struct { > + // > + // When ApicIdGate is MAX_UINT64, then no hot-added CPU may proceed wi= th > + // SMBASE relocation. > + // > + // Otherwise, the hot-added CPU whose APIC ID equals ApicIdGate may pr= oceed > + // with SMBASE relocation. > + // > + // This field is intentionally wider than APIC_ID (UINT32) because we = need a > + // "gate locked" value that is different from all possible APIC_IDs. > + // > + UINT64 ApicIdGate; > + // > + // The new SMBASE value for the hot-added CPU to set in the SMRAM Save= State > + // Map, before leaving SMM with the RSM instruction. > + // > + UINT32 NewSmbase; > +} FIRST_SMI_HANDLER_CONTEXT; > +#pragma pack () > + > +#endif // FIRST_SMI_HANDLER_CONTEXT_H_ > diff --git a/OvmfPkg/CpuHotplugSmm/Smbase.h b/OvmfPkg/CpuHotplugSmm/Smbas= e.h > index cb5aed98cdd3..e73730d19926 100644 > --- a/OvmfPkg/CpuHotplugSmm/Smbase.h > +++ b/OvmfPkg/CpuHotplugSmm/Smbase.h > @@ -1,32 +1,46 @@ > /** @file > SMBASE relocation for hot-plugged CPUs. > =20 > Copyright (c) 2020, Red Hat, Inc. > =20 > SPDX-License-Identifier: BSD-2-Clause-Patent > **/ > =20 > #ifndef SMBASE_H_ > #define SMBASE_H_ > =20 > #include // EFI_STATUS > #include // EFI_BOOT_SERVICES > =20 > +#include "ApicId.h" // APIC_ID > + > EFI_STATUS > SmbaseAllocatePostSmmPen ( > OUT UINT32 *PenAddress, > IN CONST EFI_BOOT_SERVICES *BootServices > ); > =20 > VOID > SmbaseReinstallPostSmmPen ( > IN UINT32 PenAddress > ); > =20 > VOID > SmbaseReleasePostSmmPen ( > IN UINT32 PenAddress, > IN CONST EFI_BOOT_SERVICES *BootServices > ); > =20 > +VOID > +SmbaseInstallFirstSmiHandler ( > + VOID > + ); > + > +EFI_STATUS > +SmbaseRelocate ( > + IN APIC_ID ApicId, > + IN UINTN Smbase, > + IN UINT32 PenAddress > + ); > + > #endif // SMBASE_H_ > diff --git a/OvmfPkg/CpuHotplugSmm/FirstSmiHandler.nasm b/OvmfPkg/CpuHotp= lugSmm/FirstSmiHandler.nasm > new file mode 100644 > index 000000000000..d5ce3472bd14 > --- /dev/null > +++ b/OvmfPkg/CpuHotplugSmm/FirstSmiHandler.nasm > @@ -0,0 +1,149 @@ > +;-----------------------------------------------------------------------= ------- > +; @file > +; Relocate the SMBASE on a hot-added CPU when it services its first SMI. > +; > +; Copyright (c) 2020, Red Hat, Inc. > +; > +; SPDX-License-Identifier: BSD-2-Clause-Patent > +; > +; The routine runs on the hot-added CPU in the following "big real mode"= , > +; 16-bit environment; per "SMI HANDLER EXECUTION ENVIRONMENT" in the Int= el SDM > +; (table "Processor Register Initialization in SMM"): > +; > +; - CS selector: 0x3000 (most significant 16 bits of SMM_DEFAULT_SMBASE= ). > +; > +; - CS limit: 0xFFFF_FFFF. > +; > +; - CS base: SMM_DEFAULT_SMBASE (0x3_0000). > +; > +; - IP: SMM_HANDLER_OFFSET (0x8000). > +; > +; - ES, SS, DS, FS, GS selectors: 0. > +; > +; - ES, SS, DS, FS, GS limits: 0xFFFF_FFFF. > +; > +; - ES, SS, DS, FS, GS bases: 0. > +; > +; - Operand-size and address-size override prefixes can be used to acce= ss the > +; address space beyond 1MB. > +;-----------------------------------------------------------------------= ------- > + > +SECTION .data > +BITS 16 > + > +; > +; Bring in SMM_DEFAULT_SMBASE from > +; "MdePkg/Include/Register/Intel/SmramSaveStateMap.h". > +; > +SMM_DEFAULT_SMBASE: equ 0x3_0000 > + > +; > +; Field offsets in FIRST_SMI_HANDLER_CONTEXT, which resides at > +; SMM_DEFAULT_SMBASE. > +; > +ApicIdGate: equ 0 ; UINT64 > +NewSmbase: equ 8 ; UINT32 > + > +; > +; SMRAM Save State Map field offsets, per the AMD (not Intel) layout tha= t QEMU > +; implements. Relative to SMM_DEFAULT_SMBASE. > +; > +SaveStateRevId: equ 0xFEFC ; UINT32 > +SaveStateSmbase: equ 0xFEF8 ; UINT32 > +SaveStateSmbase64: equ 0xFF00 ; UINT32 > + > +; > +; CPUID constants, from "MdePkg/Include/Register/Intel/Cpuid.h". > +; > +CPUID_SIGNATURE: equ 0x00 > +CPUID_EXTENDED_TOPOLOGY: equ 0x0B > +CPUID_VERSION_INFO: equ 0x01 > + > +GLOBAL ASM_PFX (mFirstSmiHandler) ; UINT8[] > +GLOBAL ASM_PFX (mFirstSmiHandlerSize) ; UINT16 > + > +ASM_PFX (mFirstSmiHandler): > + ; > + ; Get our own APIC ID first, so we can contend for ApicIdGate. > + ; > + ; This basically reimplements GetInitialApicId() from > + ; "UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c". > + ; > + mov eax, CPUID_SIGNATURE > + cpuid > + cmp eax, CPUID_EXTENDED_TOPOLOGY > + jb GetApicIdFromVersionInfo > + > + mov eax, CPUID_EXTENDED_TOPOLOGY > + mov ecx, 0 > + cpuid > + test ebx, 0xFFFF > + jz GetApicIdFromVersionInfo > + > + ; > + ; EDX has the APIC ID, save it to ESI. > + ; > + mov esi, edx > + jmp KnockOnGate > + > +GetApicIdFromVersionInfo: > + mov eax, CPUID_VERSION_INFO > + cpuid > + shr ebx, 24 > + ; > + ; EBX has the APIC ID, save it to ESI. > + ; > + mov esi, ebx > + > +KnockOnGate: > + ; > + ; See if ApicIdGate shows our own APIC ID. If so, swap it to MAX_UINT6= 4 > + ; (close the gate), and advance. Otherwise, keep knocking. > + ; > + ; InterlockedCompareExchange64(): > + ; - Value :=3D &FIRST_SMI_HANDLER_CONTEXT.ApicIdGate > + ; - CompareValue (EDX:EAX) :=3D APIC ID (from ESI) > + ; - ExchangeValue (ECX:EBX) :=3D MAX_UINT64 > + ; > + mov edx, 0 > + mov eax, esi > + mov ecx, 0xFFFF_FFFF > + mov ebx, 0xFFFF_FFFF > + lock cmpxchg8b [ds : dword (SMM_DEFAULT_SMBASE + ApicIdGate)] > + jz ApicIdMatch > + pause > + jmp KnockOnGate > + > +ApicIdMatch: > + ; > + ; Update the SMBASE field in the SMRAM Save State Map. > + ; > + ; First, calculate the address of the SMBASE field, based on the SMM R= evision > + ; ID; store the result in EBX. > + ; > + mov eax, dword [ds : dword (SMM_DEFAULT_SMBASE + SaveStateRevId)] > + test eax, 0xFFFF > + jz LegacySaveStateMap > + > + mov ebx, SMM_DEFAULT_SMBASE + SaveStateSmbase64 > + jmp UpdateSmbase > + > +LegacySaveStateMap: > + mov ebx, SMM_DEFAULT_SMBASE + SaveStateSmbase > + > +UpdateSmbase: > + ; > + ; Load the new SMBASE value into EAX. > + ; > + mov eax, dword [ds : dword (SMM_DEFAULT_SMBASE + NewSmbase)] > + ; > + ; Save it to the SMBASE field whose address we calculated in EBX. > + ; > + mov dword [ds : dword ebx], eax > + ; > + ; We're done; leave SMM and continue to the pen. > + ; > + rsm > + > +ASM_PFX (mFirstSmiHandlerSize): > + dw $ - ASM_PFX (mFirstSmiHandler) > diff --git a/OvmfPkg/CpuHotplugSmm/Smbase.c b/OvmfPkg/CpuHotplugSmm/Smbas= e.c > index ea21153d9145..57c9e86f3e93 100644 > --- a/OvmfPkg/CpuHotplugSmm/Smbase.c > +++ b/OvmfPkg/CpuHotplugSmm/Smbase.c > @@ -1,38 +1,46 @@ > /** @file > SMBASE relocation for hot-plugged CPUs. > =20 > Copyright (c) 2020, Red Hat, Inc. > =20 > SPDX-License-Identifier: BSD-2-Clause-Patent > **/ > =20 > #include // BASE_1MB > +#include // CpuPause() > #include // CopyMem() > #include // DEBUG() > +#include // SendInitSipiSipi() > +#include // InterlockedCompareExcha= nge64() > +#include // SMM_DEFAULT_SMBASE > + > +#include "FirstSmiHandlerContext.h" // FIRST_SMI_HANDLER_CONTE= XT > =20 > #include "Smbase.h" > =20 > extern CONST UINT8 mPostSmmPen[]; > extern CONST UINT16 mPostSmmPenSize; > +extern CONST UINT8 mFirstSmiHandler[]; > +extern CONST UINT16 mFirstSmiHandlerSize; > =20 > /** > Allocate a non-SMRAM reserved memory page for the Post-SMM Pen for hot= -added > CPUs. > =20 > This function may only be called from the entry point function of the = driver. > =20 > @param[out] PenAddress The address of the allocated (normal RAM) res= erved > page. > =20 > @param[in] BootServices Pointer to the UEFI boot services table. Used= for > allocating the normal RAM (not SMRAM) reserve= d page. > =20 > @retval EFI_SUCCESS Allocation successful. > =20 > @retval EFI_BAD_BUFFER_SIZE The Post-SMM Pen template is not smaller = than > EFI_PAGE_SIZE. > =20 > @return Error codes propagated from underlying se= rvices. > DEBUG_ERROR messages have been logged. No > resources have been allocated. > **/ > @@ -89,22 +97,156 @@ SmbaseReinstallPostSmmPen ( > } > =20 > /** > Release the reserved page allocated with SmbaseAllocatePostSmmPen(). > =20 > This function may only be called from the entry point function of the = driver, > on the error path. > =20 > @param[in] PenAddress The allocation address returned by > SmbaseAllocatePostSmmPen(). > =20 > @param[in] BootServices Pointer to the UEFI boot services table. Used= for > releasing the normal RAM (not SMRAM) reserved= page. > **/ > VOID > SmbaseReleasePostSmmPen ( > IN UINT32 PenAddress, > IN CONST EFI_BOOT_SERVICES *BootServices > ) > { > BootServices->FreePages (PenAddress, 1); > } > + > +/** > + Place the handler routine for the first SMIs of hot-added CPUs at > + (SMM_DEFAULT_SMBASE + SMM_HANDLER_OFFSET). > + > + Note that this effects an "SMRAM to SMRAM" copy. > + > + Additionally, shut the APIC ID gate in FIRST_SMI_HANDLER_CONTEXT. > + > + This function may only be called from the entry point function of the = driver, > + and only after PcdQ35SmramAtDefaultSmbase has been determined to be TR= UE. > +**/ > +VOID > +SmbaseInstallFirstSmiHandler ( > + VOID > + ) > +{ > + FIRST_SMI_HANDLER_CONTEXT *Context; > + > + CopyMem ((VOID *)(UINTN)(SMM_DEFAULT_SMBASE + SMM_HANDLER_OFFSET), > + mFirstSmiHandler, mFirstSmiHandlerSize); > + > + Context =3D (VOID *)(UINTN)SMM_DEFAULT_SMBASE; > + Context->ApicIdGate =3D MAX_UINT64; > +} > + > +/** > + Relocate the SMBASE on a hot-added CPU. Then pen the hot-added CPU in = the > + normal RAM reserved memory page, set up earlier with > + SmbaseAllocatePostSmmPen() and SmbaseReinstallPostSmmPen(). > + > + The SMM Monarch is supposed to call this function from the root MMI ha= ndler. > + > + The SMM Monarch is responsible for calling SmbaseInstallFirstSmiHandle= r(), > + SmbaseAllocatePostSmmPen(), and SmbaseReinstallPostSmmPen() before cal= ling > + this function. > + > + If the OS maliciously boots the hot-added CPU ahead of letting the ACP= I CPU > + hotplug event handler broadcast the CPU hotplug MMI, then the hot-adde= d CPU > + returns to the OS rather than to the pen, upon RSM. In that case, this > + function will hang forever (unless the OS happens to signal back throu= gh the > + last byte of the pen page). > + > + @param[in] ApicId The APIC ID of the hot-added CPU whose SMBASE s= hould > + be relocated. > + > + @param[in] Smbase The new SMBASE address. The root MMI handler is > + responsible for passing in a free ("unoccupied"= ) > + SMBASE address that was pre-configured by > + PiSmmCpuDxeSmm in CPU_HOT_PLUG_DATA. > + > + @param[in] PenAddress The address of the Post-SMM Pen for hot-added C= PUs, as > + returned by SmbaseAllocatePostSmmPen(), and ins= talled > + by SmbaseReinstallPostSmmPen(). > + > + @retval EFI_SUCCESS The SMBASE of the hot-added CPU with AP= IC ID > + ApicId has been relocated to Smbase. Th= e > + hot-added CPU has reported back about l= eaving > + SMM. > + > + @retval EFI_PROTOCOL_ERROR Synchronization bug encountered around > + FIRST_SMI_HANDLER_CONTEXT.ApicIdGate. > + > + @retval EFI_INVALID_PARAMETER Smbase does not fit in 32 bits. No relo= cation > + has been attempted. > +**/ > +EFI_STATUS > +SmbaseRelocate ( > + IN APIC_ID ApicId, > + IN UINTN Smbase, > + IN UINT32 PenAddress > + ) > +{ > + EFI_STATUS Status; > + volatile UINT8 *SmmVacated; > + volatile FIRST_SMI_HANDLER_CONTEXT *Context; > + UINT64 ExchangeResult; > + > + if (Smbase > MAX_UINT32) { > + Status =3D EFI_INVALID_PARAMETER; > + DEBUG ((DEBUG_ERROR, "%a: ApicId=3D" FMT_APIC_ID " Smbase=3D0x%Lx: %= r\n", > + __FUNCTION__, ApicId, (UINT64)Smbase, Status)); > + return Status; > + } > + > + SmmVacated =3D (UINT8 *)(UINTN)PenAddress + (EFI_PAGE_SIZE - 1); > + Context =3D (VOID *)(UINTN)SMM_DEFAULT_SMBASE; > + > + // > + // Clear the last byte of the reserved page, so we notice when the hot= -added > + // CPU checks back in from the pen. > + // > + *SmmVacated =3D 0; > + > + // > + // Boot the hot-added CPU. > + // > + // If the OS is benign, and so the hot-added CPU is still in RESET sta= te, > + // then the broadcast SMI is still pending for it; it will now launch > + // directly into SMM. > + // > + // If the OS is malicious, the hot-added CPU has been booted already, = and so > + // it is already spinning on the APIC ID gate. In that case, the > + // INIT-SIPI-SIPI below will be ignored. > + // > + SendInitSipiSipi (ApicId, PenAddress); > + > + // > + // Expose the desired new SMBASE value to the hot-added CPU. > + // > + Context->NewSmbase =3D (UINT32)Smbase; > + > + // > + // Un-gate SMBASE relocation for the hot-added CPU whose APIC ID is Ap= icId. > + // > + ExchangeResult =3D InterlockedCompareExchange64 (&Context->ApicIdGate, > + MAX_UINT64, ApicId); > + if (ExchangeResult !=3D MAX_UINT64) { > + Status =3D EFI_PROTOCOL_ERROR; > + DEBUG ((DEBUG_ERROR, "%a: ApicId=3D" FMT_APIC_ID " ApicIdGate=3D0x%L= x: %r\n", > + __FUNCTION__, ApicId, ExchangeResult, Status)); > + return Status; > + } > + > + // > + // Now wait until the hot-added CPU reports back. > + // > + while (*SmmVacated =3D=3D 0) { > + CpuPause (); > + } > + > + Status =3D EFI_SUCCESS; > + return Status; > +} >=20 Overnight I managed to think up an attack, from the OS, against the "SmmVacated" byte (the last byte of the reserved page, i.e. the last byte of the Post-SMM Pen). Here's how: There are three CPUs being hotplugged in one SMI, CPU#1..CPU#3. The OS boots them all before raising the SMI (i.e. before it allows the ACPI GPE handler to take effect). After the first CPU (let's say CPU#1) returns to the OS via the RSM, the OS uses it (=3DCPU#1) to attack "SmmVacated", writing 1 to it in a loop. Meanwhile CPU#2 and CPU#3 are still in SMM; let's say CPU#2 is relocating SMBASE, while CPU#3 is spinning on the APIC ID gate. And the SMM Monarch (CPU#0) is waiting for CPU#2 to report back in through "SmmVacated", from the Post-SMM Pen. Now, the OS writes 1 to "SmmVacated" early, pretending to be CPU#2. This causes the SMM Monarch (CPU#0) to open the APIC ID gate for CPU#3 before CPU#2 actually reaches the RSM. This may cause CPU#2 and CPU#3 to both reach RSM with the same SMBASE value. So why did I think to put SmmVacated in normal RAM (in the Post-SMM Pen reserved page?) Because in the following message: http://mid.mail-archive.com/20191004160948.72097f6c@redhat.com (alt. link: ) Igor showed that putting "SmmVacated" in SMRAM is racy, even without a malicious OS. The problem is that there is no way to flip a byte in SMRAM *atomically* with RSM. So the CPU that has just completed SMBASE relocation can only flip the byte before RSM (in SMRAM) or after RSM (in normal RAM). In the latter case, the OS can attack SmmVacated -- but in the former case, we get the same race *without* any OS involvement (because the CPU just about to leave SMM via RSM actively allows the SMM Monarch to ungate the relocation code for a different CPU). So I don't think there's a 100% safe & secure way to do this. One thing we could try -- I could update the code -- is to *combine* both approaches; use two "SmmVacated" flags: one in SMRAM, set to 1 just before the RSM instruction, and the other one in normal RAM (reserved page) that this patch set already introduces. The normal RAM flag would avoid the race completely for benign OSes (like the present patchset already does), and the SMRAM flag would keep the racy window to a single instruction when the OS is malicious and the normal RAM flag cannot be trusted. I'd appreciate feedback on this; I don't know how in physical firmware, the initial SMI handler for hot-added CPUs deals with the same problem. I guess on physical platforms, the platform manual could simply say, "only hot-plug CPUs one by one". That eliminates the whole problem. In such a case, we could safely stick with the current patchset, too. --*-- BTW, I did try hot-plugging multiple CPUs at once, with libvirt: > virsh setvcpu ovmf.fedora.q35 1,3 --enable --live > > error: Operation not supported: only one hotpluggable entity can be > selected I think this is because it would require multiple "device-add" commands to be sent at the same time over the QMP monitor -- and that's something that QEMU doesn't support. (Put alternatively, "device-add" does not take a list of objects to create.) In that regard, the problem described above is academic, because QEMU already seems like a platform that can only hotplug one CPU at a time. In that sense, using APIC ID *arrays* and *loops* per hotplug SMI is not really needed; I did that because we had discussed this feature in terms of multiple CPUs from the beginning, and because QEMU's ACPI GPE handler (the CSCN AML method) also loops over multiple processors. Again, comments would be most welcome... I wouldn't like to complicate the SMI handler if it's not absolutely necessary. Thanks! Laszlo