OvmfPkg/CpuHotplugSmm: fix CPU hotplug race just before SMI broadcast

The "virsh setvcpus" (plural) command may hot-plug several VCPUs in quick
succession -- it means a series of "device_add" QEMU monitor commands,
back-to-back.

If a "device_add" occurs *just before* ACPI raises the broadcast SMI,
then:

- OVMF processes the hot-added CPU well.

- However, QEMU's post-SMI ACPI loop -- which clears the pending events
  for the hot-added CPUs that were collected before raising the SMI -- is
  unaware of the stray CPU. Thus, the pending event is not cleared for it.

As a result of the stuck event, at the next hot-plug, OVMF tries to re-add
(relocate for the 2nd time) the already-known CPU. At that time, the AP is
already in the normal edk2 SMM busy-wait however, so it doesn't respond to
the exchange that the BSP intends to do in SmbaseRelocate(). Thus the VM
gets stuck in SMM.

(Because of the above symptom, this is not considered a security patch; it
doesn't seem exploitable by a malicious guest OS.)

In CpuHotplugMmi(), skip the supposedly hot-added CPU if it's already
known. The post-SMI ACPI loop will clear the pending event for it this
time.

Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Fixes: bc498ac4ca7590479cfd91ad1bb8a36286b0dc21
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2929
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Message-Id: <20200826222129.25798-2-lersek@redhat.com>
Reviewed-by: Ard Biesheuvel <ard.biesheuvel@arm.com>

diff --git a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
index 20e6bec04f41981fc8af311eececc28559fb8a77..cfe698ed2b5e1a555dc38f41654204d35da48254 100644 (file)
--- a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
+++ b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
@@ -193,9 +193,28 @@ CpuHotplugMmi (
   NewSlot = 0;\r
   while (PluggedIdx < PluggedCount) {\r
     APIC_ID NewApicId;\r
+    UINT32  CheckSlot;\r
     UINTN   NewProcessorNumberByProtocol;\r
 \r
     NewApicId = mPluggedApicIds[PluggedIdx];\r
+\r
+    //\r
+    // Check if the supposedly hot-added CPU is already known to us.\r
+    //\r
+    for (CheckSlot = 0;\r
+         CheckSlot < mCpuHotPlugData->ArrayLength;\r
+         CheckSlot++) {\r
+      if (mCpuHotPlugData->ApicId[CheckSlot] == NewApicId) {\r
+        break;\r
+      }\r
+    }\r
+    if (CheckSlot < mCpuHotPlugData->ArrayLength) {\r
+      DEBUG ((DEBUG_VERBOSE, "%a: APIC ID " FMT_APIC_ID " was hot-plugged "\r
+        "before; ignoring it\n", __FUNCTION__, NewApicId));\r
+      PluggedIdx++;\r
+      continue;\r
+    }\r
+\r
     //\r
     // Find the first empty slot in CPU_HOT_PLUG_DATA.\r
     //\r