UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-)
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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#40408): https://edk2.groups.io/g/devel/message/40408
Mute This Topic: https://groups.io/mt/31573867/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 05/10/19 07:16, Wang, Jian J wrote: > 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; I think this return value (EFI_UNSUPPORTED) should be replaced with EFI_NOT_FOUND, here. The return value from this function will be propagated to the caller, through SmmReadSaveState() [UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c]. The latter function implements EFI_MM_CPU_PROTOCOL.ReadSaveState(), and the PI-1.7 spec writes, If the CPU does not support the specified register Register, then EFI_NOT_FOUND should be returned. If the CPU does not support the specified register width Width , then EFI_INVALID_PARAMETER is returned. I don't feel too strongly about this, but I think it's worth considering. If others think EFI_UNSUPPORTED is better, I'm OK with that, in the end. Either way, Reviewed-by: Laszlo Ersek <lersek@redhat.com> Thanks Laszlo > + } > + > // > // 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; > } > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#40525): https://edk2.groups.io/g/devel/message/40525 Mute This Topic: https://groups.io/mt/31573867/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Laszlo, > -----Original Message----- > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of > Laszlo Ersek > Sent: Tuesday, May 14, 2019 12:46 AM > To: devel@edk2.groups.io; Wang, Jian J <jian.j.wang@intel.com> > Cc: Zeng, Star <star.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni, > Ray <ray.ni@intel.com> > Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg PiSmmCpuDxeSmm: Only support > IN/OUT IO save state read (CVE-2018-12182) > > On 05/10/19 07:16, Wang, Jian J wrote: > > 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#L7 > 6 > > > > 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; > > I think this return value (EFI_UNSUPPORTED) should be replaced with > EFI_NOT_FOUND, here. > > The return value from this function will be propagated to the caller, > through SmmReadSaveState() > [UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c]. > > The latter function implements EFI_MM_CPU_PROTOCOL.ReadSaveState(), and > the PI-1.7 spec writes, > > If the CPU does not support the specified register Register, then > EFI_NOT_FOUND should be returned. If the CPU does not support the > specified register width Width , then EFI_INVALID_PARAMETER is > returned. > > I don't feel too strongly about this, but I think it's worth > considering. If others think EFI_UNSUPPORTED is better, I'm OK with > that, in the end. > > Either way, > > Reviewed-by: Laszlo Ersek <lersek@redhat.com> > I agree. EFI_NOT_FOUND is better than EFI_UNSUPPORTED. At least returning EFI_UNSUPPORTED will somewhat break spec. Thanks for catching this. Regards, Jian > Thanks > Laszlo > > > > > + } > > + > > // > > // 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; > > } > > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#40568): https://edk2.groups.io/g/devel/message/40568 Mute This Topic: https://groups.io/mt/31573867/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.