Re: [edk2-devel] [PATCH] UefiCpuPkg PiSmmCpuDxeSmm: Only support IN/OUT IO save state read (CVE-2018-12182)

["Laszlo Ersek" <lersek@redhat.com>]

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