public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH] UefiCpuPkg PiSmmCpuDxeSmm: Only support IN/OUT IO save state read (CVE-2018-12182)
@ 2019-05-10  5:16 Wang, Jian J
  2019-05-13 16:46 ` [edk2-devel] " Laszlo Ersek
  0 siblings, 1 reply; 4+ messages in thread
From: Wang, Jian J @ 2019-05-10  5:16 UTC (permalink / raw)
  To: devel; +Cc: Star Zeng, Eric Dong, Ray Ni, Laszlo Ersek

From: Star Zeng <star.zeng@intel.com>

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1136
CVE: CVE-2018-12182

Customer met system hang-up during serial port loopback test in OS.
It is a corner case happened with one CPU core doing "out dx,al" and
another CPU core(s) doing "rep outs dx,byte ptr [rsi]".

Detailed code flow is as below.

1. Serial port loopback test in OS.
One CPU core: "out dx,al" -> Writing B2h, SMI will happen.
Another CPU core(s): "rep outs dx,byte ptr [rsi]".

2. SMI happens to enter SMM.
"out dx" (SMM_IO_TYPE_OUT_DX) is saved as I/O instruction type in
SMRAM save state for CPU doing "out dx,al".
"rep outs dx" (SMM_IO_TYPE_REP_OUTS) is saved as I/O instruction
type and rsi is save as I/O Memory Address in SMRAM save state for
CPU doing "rep outs dx, byte ptr [rsi]".

NOTE: I/O Memory Address (rsi) is a virtual address mapped by
OS/Virtual Machine.

3. Some SMM code calls EFI_SMM_CPU_PROTOCOL.ReadSaveState() with
EFI_SMM_SAVE_STATE_REGISTER_IO and parse data returned.

For example:
https://github.com/tianocore/edk2/blob/master/QuarkSocPkg/
QuarkNorthCluster/Smm/DxeSmm/QncSmmDispatcher/QNC/QNCSmmSw.c#L76

4. SmmReadSaveState() is executed to read save state for
EFI_SMM_SAVE_STATE_REGISTER_IO.

- The SmmReadSaveState() function in
  "UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c" calls the
  SmmCpuFeaturesReadSaveStateRegister() function, from the platform's
  SmmCpuFeaturesLib instance.

- If that platform-specific function returns EFI_UNSUPPORTED, then
  PiSmmCpuDxeSmm falls back to the common function
  ReadSaveStateRegister(), defined in file
  "UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c".

Current ReadSaveStateRegister() in
UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c is trying to copy data
from I/O Memory Address for EFI_SMM_SAVE_STATE_IO_TYPE_REP_PREFIX,
PF will happen as SMM page table does not know and cover this
OS/Virtual Machine virtual address.

Same case is for SmmCpuFeaturesReadSaveStateRegister() in platform-
specific SmmCpuFeaturesLib instance if it has similar implementation
to read save state for EFI_SMM_SAVE_STATE_REGISTER_IO with
EFI_SMM_SAVE_STATE_IO_TYPE_REP_PREFIX.

Same case is for "ins", 'outs' and 'rep ins'.

So to fix the problem, this patch updates the code to only support
IN/OUT, but not INS/OUTS/REP INS/REP OUTS for SmmReadSaveState().

Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Star Zeng <star.zeng@intel.com>
---
 UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c
index 26e365eabc..08cb9c05cf 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c
@@ -360,7 +360,6 @@ ReadSaveStateRegister (
   UINT32                      SmmRevId;
   SMRAM_SAVE_STATE_IOMISC     IoMisc;
   EFI_SMM_SAVE_STATE_IO_INFO  *IoInfo;
-  VOID                        *IoMemAddr;
 
   //
   // Check for special EFI_SMM_SAVE_STATE_REGISTER_LMA
@@ -406,6 +405,14 @@ ReadSaveStateRegister (
       return EFI_NOT_FOUND;
     }
 
+    //
+    // Only support IN/OUT, but not INS/OUTS/REP INS/REP OUTS.
+    //
+    if ((mSmmCpuIoType[IoMisc.Bits.Type] != EFI_SMM_SAVE_STATE_IO_TYPE_INPUT) &&
+        (mSmmCpuIoType[IoMisc.Bits.Type] != EFI_SMM_SAVE_STATE_IO_TYPE_OUTPUT)) {
+      return EFI_UNSUPPORTED;
+    }
+
     //
     // Compute index for the I/O Length and I/O Type lookup tables
     //
@@ -425,13 +432,7 @@ ReadSaveStateRegister (
     IoInfo->IoPort = (UINT16)IoMisc.Bits.Port;
     IoInfo->IoWidth = mSmmCpuIoWidth[IoMisc.Bits.Length].IoWidth;
     IoInfo->IoType = mSmmCpuIoType[IoMisc.Bits.Type];
-    if (IoInfo->IoType == EFI_SMM_SAVE_STATE_IO_TYPE_INPUT || IoInfo->IoType == EFI_SMM_SAVE_STATE_IO_TYPE_OUTPUT) {
-      ReadSaveStateRegister (CpuIndex, EFI_SMM_SAVE_STATE_REGISTER_RAX, mSmmCpuIoWidth[IoMisc.Bits.Length].Width, &IoInfo->IoData);
-    }
-    else {
-      ReadSaveStateRegisterByIndex(CpuIndex, SMM_SAVE_STATE_REGISTER_IOMEMADDR_INDEX, sizeof(IoMemAddr), &IoMemAddr);
-      CopyMem(&IoInfo->IoData, IoMemAddr, mSmmCpuIoWidth[IoMisc.Bits.Length].Width);
-    }
+    ReadSaveStateRegister (CpuIndex, EFI_SMM_SAVE_STATE_REGISTER_RAX, mSmmCpuIoWidth[IoMisc.Bits.Length].Width, &IoInfo->IoData);
     return EFI_SUCCESS;
   }
 
-- 
2.17.1.windows.2


^ permalink raw reply related	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-05-14  7:58 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-05-10  5:16 [PATCH] UefiCpuPkg PiSmmCpuDxeSmm: Only support IN/OUT IO save state read (CVE-2018-12182) Wang, Jian J
2019-05-13 16:46 ` [edk2-devel] " Laszlo Ersek
2019-05-14  3:13   ` Wang, Jian J
     [not found]   ` <159E6E221B567624.17306@groups.io>
2019-05-14  7:58     ` Wang, Jian J

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox