Re: [edk2-devel] [PATCH 12/16] OvmfPkg/CpuHotplugSmm: introduce First SMI Handler for hot-added CPUs

["Laszlo Ersek" <lersek@redhat.com>]

On 02/23/20 18:25, Laszlo Ersek wrote:
> Implement the First SMI Handler for hot-added CPUs, in NASM.
> 
> Add the interfacing C-language function that the SMM Monarch calls. This
> function launches and coordinates SMBASE relocation for a hot-added CPU.
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Michael Kinney <michael.d.kinney@intel.com>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1512
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  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(+)
> 
> diff --git a/OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf b/OvmfPkg/CpuHotplugSmm/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
>  ##
>  
>  [Defines]
>    INF_VERSION                = 1.29
>    PI_SPECIFICATION_VERSION   = 0x00010046                            # PI-1.7.0
>    BASE_NAME                  = CpuHotplugSmm
>    FILE_GUID                  = 84EEA114-C6BE-4445-8F90-51D97863E363
>    MODULE_TYPE                = DXE_SMM_DRIVER
>    ENTRY_POINT                = CpuHotplugEntry
>  
>  #
>  # The following information is for reference only and not required by the build
>  # tools.
>  #
>  # VALID_ARCHITECTURES        = IA32 X64
>  #
>  
>  [Sources]
>    ApicId.h
>    CpuHotplug.c
> +  FirstSmiHandler.nasm
> +  FirstSmiHandlerContext.h
>    PostSmmPen.nasm
>    QemuCpuhp.c
>    QemuCpuhp.h
>    Smbase.c
>    Smbase.h
>  
>  [Packages]
>    MdePkg/MdePkg.dec
>    OvmfPkg/OvmfPkg.dec
>    UefiCpuPkg/UefiCpuPkg.dec
>  
>  [LibraryClasses]
>    BaseLib
>    BaseMemoryLib
>    DebugLib
> +  LocalApicLib
>    MmServicesTableLib
>    PcdLib
>    SafeIntLib
> +  SynchronizationLib
>    UefiDriverEntryPoint
>  
>  [Protocols]
>    gEfiMmCpuIoProtocolGuid                                           ## CONSUMES
>    gEfiSmmCpuServiceProtocolGuid                                     ## CONSUMES
>  
>  [Pcd]
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuHotPlugDataAddress                ## CONSUMES
>    gUefiOvmfPkgTokenSpaceGuid.PcdQ35SmramAtDefaultSmbase             ## CONSUMES
>  
>  [FeaturePcd]
>    gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire                     ## CONSUMES
>  
>  [Depex]
>    gEfiMmCpuIoProtocolGuid AND
>    gEfiSmmCpuServiceProtocolGuid
> diff --git a/OvmfPkg/CpuHotplugSmm/FirstSmiHandlerContext.h b/OvmfPkg/CpuHotplugSmm/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 area
> +  between the SMM Monarch and the hot-added CPU, for relocating the SMBASE 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 Monarch
> +// (running the root MMI handler) and the hot-added CPU (handling its first
> +// SMI). It is placed at SMM_DEFAULT_SMBASE, which is in SMRAM under QEMU's
> +// "SMRAM at default SMBASE" feature.
> +//
> +#pragma pack (1)
> +typedef struct {
> +  //
> +  // When ApicIdGate is MAX_UINT64, then no hot-added CPU may proceed with
> +  // SMBASE relocation.
> +  //
> +  // Otherwise, the hot-added CPU whose APIC ID equals ApicIdGate may proceed
> +  // 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/Smbase.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.
>  
>    Copyright (c) 2020, Red Hat, Inc.
>  
>    SPDX-License-Identifier: BSD-2-Clause-Patent
>  **/
>  
>  #ifndef SMBASE_H_
>  #define SMBASE_H_
>  
>  #include <Uefi/UefiBaseType.h> // EFI_STATUS
>  #include <Uefi/UefiSpec.h>     // EFI_BOOT_SERVICES
>  
> +#include "ApicId.h"            // APIC_ID
> +
>  EFI_STATUS
>  SmbaseAllocatePostSmmPen (
>    OUT UINT32                  *PenAddress,
>    IN  CONST EFI_BOOT_SERVICES *BootServices
>    );
>  
>  VOID
>  SmbaseReinstallPostSmmPen (
>    IN UINT32 PenAddress
>    );
>  
>  VOID
>  SmbaseReleasePostSmmPen (
>    IN UINT32                  PenAddress,
>    IN CONST EFI_BOOT_SERVICES *BootServices
>    );
>  
> +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/CpuHotplugSmm/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 Intel 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 access 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 that 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_UINT64
> +  ; (close the gate), and advance. Otherwise, keep knocking.
> +  ;
> +  ; InterlockedCompareExchange64():
> +  ; - Value                   := &FIRST_SMI_HANDLER_CONTEXT.ApicIdGate
> +  ; - CompareValue  (EDX:EAX) := APIC ID (from ESI)
> +  ; - ExchangeValue (ECX:EBX) := 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 Revision
> +  ; 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/Smbase.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.
>  
>    Copyright (c) 2020, Red Hat, Inc.
>  
>    SPDX-License-Identifier: BSD-2-Clause-Patent
>  **/
>  
>  #include <Base.h>                             // BASE_1MB
> +#include <Library/BaseLib.h>                  // CpuPause()
>  #include <Library/BaseMemoryLib.h>            // CopyMem()
>  #include <Library/DebugLib.h>                 // DEBUG()
> +#include <Library/LocalApicLib.h>             // SendInitSipiSipi()
> +#include <Library/SynchronizationLib.h>       // InterlockedCompareExchange64()
> +#include <Register/Intel/SmramSaveStateMap.h> // SMM_DEFAULT_SMBASE
> +
> +#include "FirstSmiHandlerContext.h"           // FIRST_SMI_HANDLER_CONTEXT
>  
>  #include "Smbase.h"
>  
>  extern CONST UINT8 mPostSmmPen[];
>  extern CONST UINT16 mPostSmmPenSize;
> +extern CONST UINT8 mFirstSmiHandler[];
> +extern CONST UINT16 mFirstSmiHandlerSize;
>  
>  /**
>    Allocate a non-SMRAM reserved memory page for the Post-SMM Pen for hot-added
>    CPUs.
>  
>    This function may only be called from the entry point function of the driver.
>  
>    @param[out] PenAddress   The address of the allocated (normal RAM) reserved
>                             page.
>  
>    @param[in] BootServices  Pointer to the UEFI boot services table. Used for
>                             allocating the normal RAM (not SMRAM) reserved page.
>  
>    @retval EFI_SUCCESS          Allocation successful.
>  
>    @retval EFI_BAD_BUFFER_SIZE  The Post-SMM Pen template is not smaller than
>                                 EFI_PAGE_SIZE.
>  
>    @return                      Error codes propagated from underlying services.
>                                 DEBUG_ERROR messages have been logged. No
>                                 resources have been allocated.
>  **/
> @@ -89,22 +97,156 @@ SmbaseReinstallPostSmmPen (
>  }
>  
>  /**
>    Release the reserved page allocated with SmbaseAllocatePostSmmPen().
>  
>    This function may only be called from the entry point function of the driver,
>    on the error path.
>  
>    @param[in] PenAddress    The allocation address returned by
>                             SmbaseAllocatePostSmmPen().
>  
>    @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 TRUE.
> +**/
> +VOID
> +SmbaseInstallFirstSmiHandler (
> +  VOID
> +  )
> +{
> +  FIRST_SMI_HANDLER_CONTEXT *Context;
> +
> +  CopyMem ((VOID *)(UINTN)(SMM_DEFAULT_SMBASE + SMM_HANDLER_OFFSET),
> +    mFirstSmiHandler, mFirstSmiHandlerSize);
> +
> +  Context = (VOID *)(UINTN)SMM_DEFAULT_SMBASE;
> +  Context->ApicIdGate = 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 handler.
> +
> +  The SMM Monarch is responsible for calling SmbaseInstallFirstSmiHandler(),
> +  SmbaseAllocatePostSmmPen(), and SmbaseReinstallPostSmmPen() before calling
> +  this function.
> +
> +  If the OS maliciously boots the hot-added CPU ahead of letting the ACPI CPU
> +  hotplug event handler broadcast the CPU hotplug MMI, then the hot-added 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 through the
> +  last byte of the pen page).
> +
> +  @param[in] ApicId      The APIC ID of the hot-added CPU whose SMBASE should
> +                         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 CPUs, as
> +                         returned by SmbaseAllocatePostSmmPen(), and installed
> +                         by SmbaseReinstallPostSmmPen().
> +
> +  @retval EFI_SUCCESS            The SMBASE of the hot-added CPU with APIC ID
> +                                 ApicId has been relocated to Smbase. The
> +                                 hot-added CPU has reported back about leaving
> +                                 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 relocation
> +                                 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 = EFI_INVALID_PARAMETER;
> +    DEBUG ((DEBUG_ERROR, "%a: ApicId=" FMT_APIC_ID " Smbase=0x%Lx: %r\n",
> +      __FUNCTION__, ApicId, (UINT64)Smbase, Status));
> +    return Status;
> +  }
> +
> +  SmmVacated = (UINT8 *)(UINTN)PenAddress + (EFI_PAGE_SIZE - 1);
> +  Context = (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 = 0;
> +
> +  //
> +  // Boot the hot-added CPU.
> +  //
> +  // If the OS is benign, and so the hot-added CPU is still in RESET state,
> +  // 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 = (UINT32)Smbase;
> +
> +  //
> +  // Un-gate SMBASE relocation for the hot-added CPU whose APIC ID is ApicId.
> +  //
> +  ExchangeResult = InterlockedCompareExchange64 (&Context->ApicIdGate,
> +                     MAX_UINT64, ApicId);
> +  if (ExchangeResult != MAX_UINT64) {
> +    Status = EFI_PROTOCOL_ERROR;
> +    DEBUG ((DEBUG_ERROR, "%a: ApicId=" FMT_APIC_ID " ApicIdGate=0x%Lx: %r\n",
> +      __FUNCTION__, ApicId, ExchangeResult, Status));
> +    return Status;
> +  }
> +
> +  //
> +  // Now wait until the hot-added CPU reports back.
> +  //
> +  while (*SmmVacated == 0) {
> +    CpuPause ();
> +  }
> +
> +  Status = EFI_SUCCESS;
> +  return Status;
> +}
> 

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 (=CPU#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: <https://edk2.groups.io/g/devel/message/48475>)

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