Re: [edk2-devel] [PATCH] UefiCpuPkg PiSmmCpuDxeSmm: Only support IN/OUT IO save state read (CVE-2018-12182)
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; ler...@redhat.com > Cc: Zeng, Star ; Dong, Eric ; Ni, > Ray > 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 > > Cc: Zeng, Star ; Dong, Eric ; Ni, > > Ray > > 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 > > > > > > 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 > > > Cc: Ray Ni > > > Cc: Laszlo Ersek > > > Signed-off-by: Star Zeng > > > --- > > > 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 +36
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
> Cc: Zeng, Star ; Dong, Eric ; Ni,
> Ray
> 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
> >
> > 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
> > Cc: Ray Ni
> > Cc: Laszlo Ersek
> > Signed-off-by: Star Zeng
> > ---
> > 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 retur
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
>
> 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
> Cc: Ray Ni
> Cc: Laszlo Ersek
> Signed-off-by: Star Zeng
> ---
> 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
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(
