From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Mon, 13 May 2019 09:46:23 -0700 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 96E758764F; Mon, 13 May 2019 16:46:22 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-123-237.rdu2.redhat.com [10.10.123.237]) by smtp.corp.redhat.com (Postfix) with ESMTP id 0F9BA5D706; Mon, 13 May 2019 16:46:20 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg PiSmmCpuDxeSmm: Only support IN/OUT IO save state read (CVE-2018-12182) To: devel@edk2.groups.io, jian.j.wang@intel.com Cc: Star Zeng , Eric Dong , Ray Ni References: <20190510051615.318124-1-jian.j.wang@intel.com> From: "Laszlo Ersek" Message-ID: <09a4eb80-b18a-223e-654c-1cb0384a87e8@redhat.com> Date: Mon, 13 May 2019 18:46:19 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20190510051615.318124-1-jian.j.wang@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.26]); Mon, 13 May 2019 16:46:22 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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(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; > } > >