[PATCH 0/2] OvmfPkg: fix race conditions in VCPU hotplug (for edk2-stable202008)

[PATCH 0/2] OvmfPkg: fix race conditions in VCPU hotplug (for edk2-stable202008)

[Laszlo Ersek]

Ref:    https://bugzilla.tianocore.org/show_bug.cgi?id=2929
Repo:   https://pagure.io/lersek/edk2.git
Branch: cpu_hotplug_races_bz_2929

Proposing these bugfixes for edk2-stable202008. We should either include
them in the release, or revert commit 5ba203b54e59
("OvmfPkg/SmmControl2Dxe: negotiate ICH9_LPC_SMI_F_CPU_HOTPLUG",
2020-08-24). Please see the BZ for more details.

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>

Thanks,
Laszlo

Laszlo Ersek (2):
  OvmfPkg/CpuHotplugSmm: fix CPU hotplug race just before SMI broadcast
  OvmfPkg/CpuHotplugSmm: fix CPU hotplug race just after SMI broadcast

 OvmfPkg/CpuHotplugSmm/CpuHotplug.c | 19 +++++++++++
 OvmfPkg/CpuHotplugSmm/Smbase.c     | 35 ++++++++++++++++----
 2 files changed, 48 insertions(+), 6 deletions(-)

-- 
2.19.1.3.g30247aa5d201

[PATCH 1/2] OvmfPkg/CpuHotplugSmm: fix CPU hotplug race just before SMI broadcast

[Laszlo Ersek]

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>
---
 OvmfPkg/CpuHotplugSmm/CpuHotplug.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

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

[PATCH 2/2] OvmfPkg/CpuHotplugSmm: fix CPU hotplug race just after SMI broadcast

[Laszlo Ersek]

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 after* ACPI raises the broadcast SMI, then:

- the CPU_FOREACH() loop in QEMU's ich9_apm_ctrl_changed() cannot make the
  SMI pending for the new CPU -- at that time, the new CPU doesn't even
  exist yet,

- OVMF will find the new CPU however (in the CPU hotplug register block),
  in QemuCpuhpCollectApicIds().

As a result, when the firmware sends an INIT-SIPI-SIPI to the new CPU in
SmbaseRelocate(), expecting it to boot into SMM (due to the pending SMI),
the new CPU instead boots straight into the post-RSM (normal mode) "pen",
skipping its initial SMI handler.

The CPU halts nicely in the pen, but its SMBASE is never relocated, and
the SMRAM message exchange with the BSP falls apart -- the BSP gets stuck
in the following loop:

  //
  // Wait until the hot-added CPU is just about to execute RSM.
  //
  while (Context->AboutToLeaveSmm == 0) {
    CpuPause ();
  }

because the new CPU's initial SMI handler never sets the flag to nonzero.

Fix this by sending a directed SMI to the new CPU just before sending it
the INIT-SIPI-SIPI. The various scenarios are documented in the code --
the cases affected by the patch are documented under point (2).

Note that this is not considered a security patch, as for a malicious
guest OS, the issue is not exploitable -- the symptom is a hang on the
BSP, in the above-noted loop in SmbaseRelocate(). Instead, the patch fixes
behavior for a benign guest OS.

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: 51a6fb41181529e4b50ea13377425bda6bb69ba6
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2929
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/CpuHotplugSmm/Smbase.c | 35 ++++++++++++++++----
 1 file changed, 29 insertions(+), 6 deletions(-)

diff --git a/OvmfPkg/CpuHotplugSmm/Smbase.c b/OvmfPkg/CpuHotplugSmm/Smbase.c
index 170571221d84..d8f45c431324 100644
--- a/OvmfPkg/CpuHotplugSmm/Smbase.c
+++ b/OvmfPkg/CpuHotplugSmm/Smbase.c
@@ -220,14 +220,37 @@ SmbaseRelocate (
   //
   // 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.
+  // There are 2*2 cases to consider:
   //
-  // 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.
+  // (1) The CPU was hot-added before the SMI was broadcast.
   //
+  // (1.1) The OS is benign.
+  //
+  //       The hot-added CPU is in RESET state, with the broadcast SMI pending
+  //       for it. The directed SMI below will be ignored (it's idempotent),
+  //       and the INIT-SIPI-SIPI will launch the CPU directly into SMM.
+  //
+  // (1.2) The OS is malicious.
+  //
+  //       The hot-added CPU has been booted, by the OS. Thus, the hot-added
+  //       CPU is spinning on the APIC ID gate. In that case, both the SMI and
+  //       the INIT-SIPI-SIPI below will be ignored.
+  //
+  // (2) The CPU was hot-added after the SMI was broadcast.
+  //
+  // (2.1) The OS is benign.
+  //
+  //       The hot-added CPU is in RESET state, with no SMI pending for it. The
+  //       directed SMI will latch the SMI for the CPU. Then the INIT-SIPI-SIPI
+  //       will launch the CPU into SMM.
+  //
+  // (2.2) The OS is malicious.
+  //
+  //       The hot-added CPU is executing OS code. The directed SMI will pull
+  //       the hot-added CPU into SMM, where it will start spinning on the APIC
+  //       ID gate. The INIT-SIPI-SIPI will be ignored.
+  //
+  SendSmiIpi (ApicId);
   SendInitSipiSipi (ApicId, PenAddress);
 
   //
-- 
2.19.1.3.g30247aa5d201

Re: [PATCH 0/2] OvmfPkg: fix race conditions in VCPU hotplug (for edk2-stable202008)

[Ard Biesheuvel]

On 8/27/20 12:21 AM, Laszlo Ersek wrote:
> Ref:    https://bugzilla.tianocore.org/show_bug.cgi?id=2929
> Repo:   https://pagure.io/lersek/edk2.git
> Branch: cpu_hotplug_races_bz_2929
> 
> Proposing these bugfixes for edk2-stable202008. We should either include
> them in the release, or revert commit 5ba203b54e59
> ("OvmfPkg/SmmControl2Dxe: negotiate ICH9_LPC_SMI_F_CPU_HOTPLUG",
> 2020-08-24). Please see the BZ for more details.
> 
> 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>
> 


For the series,

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@arm.com>

I have no objections to incorporating these changes into the upcoming 
stable tag.

Re: [edk2-devel] [PATCH 0/2] OvmfPkg: fix race conditions in VCPU hotplug (for edk2-stable202008)

[Laszlo Ersek]

On 08/27/20 09:50, Ard Biesheuvel wrote:
> On 8/27/20 12:21 AM, Laszlo Ersek wrote:
>> Ref:    https://bugzilla.tianocore.org/show_bug.cgi?id=2929
>> Repo:   https://pagure.io/lersek/edk2.git
>> Branch: cpu_hotplug_races_bz_2929
>>
>> Proposing these bugfixes for edk2-stable202008. We should either include
>> them in the release, or revert commit 5ba203b54e59
>> ("OvmfPkg/SmmControl2Dxe: negotiate ICH9_LPC_SMI_F_CPU_HOTPLUG",
>> 2020-08-24). Please see the BZ for more details.
>>
>> 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>
>>
> 
> 
> For the series,
> 
> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> 
> I have no objections to incorporating these changes into the upcoming
> stable tag.

Thanks a lot for your help, Ard!

Merged as commit range 63d92674d240..cbccf995920a, via
<https://github.com/tianocore/edk2/pull/905>.

Laszlo