[edk2-devel] [PATCH] UefiCpuPkg PiSmmCpuDxeSmm: Only support IN/OUT IO save state read (CVE-2018-12182)
Wang, Jian J
jian.j.wang at intel.com
Tue May 14 07:58:42 UTC 2019
Push@ cf574f0a1838665498e43e9c029d8c1211cbbfa3
Regards,
Jian
> -----Original Message-----
> From: devel at edk2.groups.io [mailto:devel at edk2.groups.io] On Behalf Of Wang,
> Jian J
> Sent: Tuesday, May 14, 2019 11:14 AM
> To: devel at edk2.groups.io; lersek at redhat.com
> Cc: Zeng, Star <star.zeng at intel.com>; Dong, Eric <eric.dong at intel.com>; Ni,
> Ray <ray.ni at 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 at edk2.groups.io [mailto:devel at edk2.groups.io] On Behalf Of
> > Laszlo Ersek
> > Sent: Tuesday, May 14, 2019 12:46 AM
> > To: devel at edk2.groups.io; Wang, Jian J <jian.j.wang at intel.com>
> > Cc: Zeng, Star <star.zeng at intel.com>; Dong, Eric <eric.dong at intel.com>; Ni,
> > Ray <ray.ni at 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 at 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 at intel.com>
> > > Cc: Ray Ni <ray.ni at intel.com>
> > > Cc: Laszlo Ersek <lersek at redhat.com>
> > > Signed-off-by: Star Zeng <star.zeng at 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 at 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 (#40588): https://edk2.groups.io/g/devel/message/40588
Mute This Topic: https://groups.io/mt/31573867/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list