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

* Re: [edk2-devel] [PATCH] UefiCpuPkg PiSmmCpuDxeSmm: Only support IN/OUT IO save state read (CVE-2018-12182)
  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 ` Laszlo Ersek
  2019-05-14  3:13   ` Wang, Jian J
       [not found]   ` <159E6E221B567624.17306@groups.io>
  0 siblings, 2 replies; 4+ messages in thread
From: Laszlo Ersek @ 2019-05-13 16:46 UTC (permalink / raw)
  To: devel, jian.j.wang; +Cc: Star Zeng, Eric Dong, Ray Ni

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;
>    }
>  
> 


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [edk2-devel] [PATCH] UefiCpuPkg PiSmmCpuDxeSmm: Only support IN/OUT IO save state read (CVE-2018-12182)
  2019-05-13 16:46 ` [edk2-devel] " Laszlo Ersek
@ 2019-05-14  3:13   ` Wang, Jian J
       [not found]   ` <159E6E221B567624.17306@groups.io>
  1 sibling, 0 replies; 4+ messages in thread
From: Wang, Jian J @ 2019-05-14  3:13 UTC (permalink / raw)
  To: devel@edk2.groups.io, lersek@redhat.com; +Cc: Zeng, Star, Dong, Eric, Ni, Ray

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;
> >    }
> >
> >
> 
> 
> 


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [edk2-devel] [PATCH] UefiCpuPkg PiSmmCpuDxeSmm: Only support IN/OUT IO save state read (CVE-2018-12182)
       [not found]   ` <159E6E221B567624.17306@groups.io>
@ 2019-05-14  7:58     ` Wang, Jian J
  0 siblings, 0 replies; 4+ messages in thread
From: Wang, Jian J @ 2019-05-14  7:58 UTC (permalink / raw)
  To: devel@edk2.groups.io, Wang, Jian J, lersek@redhat.com
  Cc: Zeng, Star, Dong, Eric, Ni, Ray

Push@ cf574f0a1838665498e43e9c029d8c1211cbbfa3

Regards,
Jian


> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Wang,
> Jian J
> Sent: Tuesday, May 14, 2019 11:14 AM
> To: devel@edk2.groups.io; lersek@redhat.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)
> 
> 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;
> > >    }
> > >
> > >
> >
> >
> >
> 
> 
> 


^ permalink raw reply	[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