From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.24; helo=mga09.intel.com; envelope-from=ray.ni@intel.com; receiver=edk2-devel@lists.01.org Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 23947211CFFC2 for ; Wed, 27 Feb 2019 22:54:44 -0800 (PST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga008.jf.intel.com ([10.7.209.65]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 27 Feb 2019 22:54:44 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.58,422,1544515200"; d="scan'208";a="121430492" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by orsmga008.jf.intel.com with ESMTP; 27 Feb 2019 22:54:44 -0800 Received: from fmsmsx157.amr.corp.intel.com (10.18.116.73) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.408.0; Wed, 27 Feb 2019 22:54:43 -0800 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by FMSMSX157.amr.corp.intel.com (10.18.116.73) with Microsoft SMTP Server (TLS) id 14.3.408.0; Wed, 27 Feb 2019 22:54:43 -0800 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.74]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.134]) with mapi id 14.03.0415.000; Thu, 28 Feb 2019 14:54:41 +0800 From: "Ni, Ray" To: "Chen, Chen A" , "edk2-devel@lists.01.org" CC: "Dong, Eric" Thread-Topic: [edk2] [PATCH 2/2] UefiCpuPkg/Microcode: Add verification logic before parsing payload Thread-Index: AQHUzytUL8JNcTLLo0mbYURPHANPGaX0vDWw Date: Thu, 28 Feb 2019 06:54:41 +0000 Message-ID: <734D49CCEBEEF84792F5B80ED585239D5C04FD79@SHSMSX104.ccr.corp.intel.com> References: <20190228060252.23944-1-chen.a.chen@intel.com> <20190228060252.23944-3-chen.a.chen@intel.com> In-Reply-To: <20190228060252.23944-3-chen.a.chen@intel.com> Accept-Language: en-US, zh-CN X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH 2/2] UefiCpuPkg/Microcode: Add verification logic before parsing payload X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 28 Feb 2019 06:54:45 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable > -----Original Message----- > From: edk2-devel On Behalf Of Chen A > Chen > Sent: Thursday, February 28, 2019 2:03 PM > To: edk2-devel@lists.01.org > Cc: Dong, Eric > Subject: [edk2] [PATCH 2/2] UefiCpuPkg/Microcode: Add verification logic > before parsing payload >=20 > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D1020 >=20 > The function MicrocodeDetect may receive untrusted input. Add verificatio= n > logic to check Microcode Payload Header. >=20 > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Chen A Chen > Cc: Ray Ni > Cc: Eric Dong > --- > UefiCpuPkg/Library/MpInitLib/Microcode.c | 190 > +++++++++++++++++++++---------- > 1 file changed, 127 insertions(+), 63 deletions(-) >=20 > diff --git a/UefiCpuPkg/Library/MpInitLib/Microcode.c > b/UefiCpuPkg/Library/MpInitLib/Microcode.c > index 5f9ae22794..1d5e964791 100644 > --- a/UefiCpuPkg/Library/MpInitLib/Microcode.c > +++ b/UefiCpuPkg/Library/MpInitLib/Microcode.c > @@ -32,6 +32,69 @@ GetCurrentMicrocodeSignature ( > return BiosSignIdMsr.Bits.MicrocodeUpdateSignature; > } >=20 > +/** > + Verify Microcode. > + > + @param[in] MicrocodeHeader Point to the Microcode payload buffe= r. > + > + @retval TRUE Valid payload > + @retval FALSE Invalid payload > +**/ > +BOOLEAN > +VerifyMicrocodeHeader ( 1. "Verify" cannot tell what the return value stands for. Use "IsMicrocodeH= eaderValid"? > + IN CPU_MICROCODE_HEADER *MicrocodeHeader > + ) > +{ > + UINTN TotalSize; > + UINTN DataSize; > + > + if (MicrocodeHeader =3D=3D NULL) { > + return FALSE; > + } 2. Can we use assert here? > + > + /// > + /// Verify Version > + /// > + if (MicrocodeHeader->HeaderVersion !=3D 0x1) { > + return FALSE; > + } > + if (MicrocodeHeader->LoaderRevision !=3D 0x1) { > + return FALSE; > + } > + > + /// > + /// Verify TotalSize > + /// > + if (MicrocodeHeader->DataSize =3D=3D 0) { > + TotalSize =3D 2048; > + } else { > + TotalSize =3D MicrocodeHeader->TotalSize; > + } > + if (TotalSize <=3D sizeof (CPU_MICROCODE_HEADER)) { > + return FALSE; > + } 3. Can you please add more comments here to describe the alignment requirem= ent? > + if ((TotalSize & (SIZE_1KB - 1)) !=3D 0) { > + return FALSE; > + } > + > + /// > + /// Verify DataSize > + /// > + if (MicrocodeHeader->DataSize =3D=3D 0) { > + DataSize =3D 2048 - sizeof (CPU_MICROCODE_HEADER); > + } else { > + DataSize =3D MicrocodeHeader->DataSize; > + } > + if (DataSize > TotalSize - sizeof (CPU_MICROCODE_HEADER)) { 4. if MIcrocodeHeader->DataSize is 0, seems the if-check is redundant. > + return FALSE; > + } 5. Can you please add more comments here to describe the alignment requirem= ent? > + if ((DataSize & 0x3) !=3D 0) { > + return FALSE; > + } > + > + return TRUE; > +} > + > /** > Detect whether specified processor can find matching microcode patch a= nd > load it. >=20 > @@ -166,6 +229,18 @@ MicrocodeDetect ( > // > CorrectMicrocode =3D FALSE; >=20 > + if (!VerifyMicrocodeHeader (MicrocodeEntryPoint)) { > + // > + // It is the padding data between the microcode patches for microc= ode > patches alignment. > + // Because the microcode patch is the multiple of 1-KByte, the pad= ding > data should not > + // exist if the microcode patch alignment value is not larger than= 1-KByte. > So, the microcode > + // alignment value should be larger than 1-KByte. We could skip > SIZE_1KB padding data to > + // find the next possible microcode patch header. > + // > + MicrocodeEntryPoint =3D (CPU_MICROCODE_HEADER *) (((UINTN) > MicrocodeEntryPoint) + SIZE_1KB); > + continue; > + } > + > // > // Save an in-complete CheckSum32 from CheckSum Part1 for common > parts. > // > @@ -184,90 +259,79 @@ MicrocodeDetect ( > InCompleteCheckSum32 -=3D MicrocodeEntryPoint->ProcessorFlags; > InCompleteCheckSum32 -=3D MicrocodeEntryPoint->Checksum; >=20 > - if (MicrocodeEntryPoint->HeaderVersion =3D=3D 0x1) { > + // > + // It is the microcode header. It is not the padding data between > microcode patches > + // because the padding data should not include 0x00000001 and it sho= uld > be the repeated > + // byte format (like 0xXYXYXYXY....). > + // > + if (MicrocodeEntryPoint->ProcessorSignature.Uint32 =3D=3D Eax.Uint32= && > + MicrocodeEntryPoint->UpdateRevision > LatestRevision && > + (MicrocodeEntryPoint->ProcessorFlags & (1 << PlatformId)) > + ) { > // > - // It is the microcode header. It is not the padding data between > microcode patches > - // because the padding data should not include 0x00000001 and it s= hould > be the repeated > - // byte format (like 0xXYXYXYXY....). > + // Calculate CheckSum Part1. > // > - if (MicrocodeEntryPoint->ProcessorSignature.Uint32 =3D=3D Eax.Uint= 32 && > - MicrocodeEntryPoint->UpdateRevision > LatestRevision && > - (MicrocodeEntryPoint->ProcessorFlags & (1 << PlatformId)) > - ) { > + CheckSum32 =3D InCompleteCheckSum32; > + CheckSum32 +=3D MicrocodeEntryPoint->ProcessorSignature.Uint32; > + CheckSum32 +=3D MicrocodeEntryPoint->ProcessorFlags; > + CheckSum32 +=3D MicrocodeEntryPoint->Checksum; > + if (CheckSum32 =3D=3D 0) { > + CorrectMicrocode =3D TRUE; > + ProcessorFlags =3D MicrocodeEntryPoint->ProcessorFlags; > + } > + } else if ((MicrocodeEntryPoint->DataSize !=3D 0) && > + (MicrocodeEntryPoint->UpdateRevision > LatestRevision)) { > + ExtendedTableLength =3D MicrocodeEntryPoint->TotalSize - > (MicrocodeEntryPoint->DataSize + > + sizeof (CPU_MICROCODE_HEADER)); > + if (ExtendedTableLength !=3D 0) { > // > - // Calculate CheckSum Part1. > + // Extended Table exist, check if the CPU in support list > // > - CheckSum32 =3D InCompleteCheckSum32; > - CheckSum32 +=3D MicrocodeEntryPoint->ProcessorSignature.Uint32; > - CheckSum32 +=3D MicrocodeEntryPoint->ProcessorFlags; > - CheckSum32 +=3D MicrocodeEntryPoint->Checksum; > - if (CheckSum32 =3D=3D 0) { > - CorrectMicrocode =3D TRUE; > - ProcessorFlags =3D MicrocodeEntryPoint->ProcessorFlags; > - } > - } else if ((MicrocodeEntryPoint->DataSize !=3D 0) && > - (MicrocodeEntryPoint->UpdateRevision > LatestRevision))= { > - ExtendedTableLength =3D MicrocodeEntryPoint->TotalSize - > (MicrocodeEntryPoint->DataSize + > - sizeof (CPU_MICROCODE_HEADER)); > - if (ExtendedTableLength !=3D 0) { > - // > - // Extended Table exist, check if the CPU in support list > - // > - ExtendedTableHeader =3D > (CPU_MICROCODE_EXTENDED_TABLE_HEADER *) ((UINT8 *) > (MicrocodeEntryPoint) > - + MicrocodeEntryPoint->DataSize + size= of > (CPU_MICROCODE_HEADER)); > + ExtendedTableHeader =3D > (CPU_MICROCODE_EXTENDED_TABLE_HEADER *) ((UINT8 *) > (MicrocodeEntryPoint) > + + MicrocodeEntryPoint->DataSize + sizeof > (CPU_MICROCODE_HEADER)); > + // > + // Calculate Extended Checksum > + // > + if ((ExtendedTableLength % 4) =3D=3D 0) { > // > - // Calculate Extended Checksum > + // Calculate CheckSum Part2. > // > - if ((ExtendedTableLength % 4) =3D=3D 0) { > + CheckSum32 =3D CalculateSum32 ((UINT32 *) ExtendedTableHeader, > ExtendedTableLength); > + if (CheckSum32 =3D=3D 0) { > // > - // Calculate CheckSum Part2. > + // Checksum correct > // > - CheckSum32 =3D CalculateSum32 ((UINT32 *) ExtendedTableHeade= r, > ExtendedTableLength); > - if (CheckSum32 =3D=3D 0) { > + ExtendedTableCount =3D ExtendedTableHeader- > >ExtendedSignatureCount; > + ExtendedTable =3D (CPU_MICROCODE_EXTENDED_TABLE *) > (ExtendedTableHeader + 1); > + for (Index =3D 0; Index < ExtendedTableCount; Index ++) { > // > - // Checksum correct > + // Calculate CheckSum Part3. > // > - ExtendedTableCount =3D ExtendedTableHeader- > >ExtendedSignatureCount; > - ExtendedTable =3D (CPU_MICROCODE_EXTENDED_TABLE *) > (ExtendedTableHeader + 1); > - for (Index =3D 0; Index < ExtendedTableCount; Index ++) { > + CheckSum32 =3D InCompleteCheckSum32; > + CheckSum32 +=3D ExtendedTable->ProcessorSignature.Uint32; > + CheckSum32 +=3D ExtendedTable->ProcessorFlag; > + CheckSum32 +=3D ExtendedTable->Checksum; > + if (CheckSum32 =3D=3D 0) { > // > - // Calculate CheckSum Part3. > + // Verify Header > // > - CheckSum32 =3D InCompleteCheckSum32; > - CheckSum32 +=3D ExtendedTable->ProcessorSignature.Uint32= ; > - CheckSum32 +=3D ExtendedTable->ProcessorFlag; > - CheckSum32 +=3D ExtendedTable->Checksum; > - if (CheckSum32 =3D=3D 0) { > + if ((ExtendedTable->ProcessorSignature.Uint32 =3D=3D Eax= .Uint32) && > + (ExtendedTable->ProcessorFlag & (1 << PlatformId)) )= { > // > - // Verify Header > + // Find one > // > - if ((ExtendedTable->ProcessorSignature.Uint32 =3D=3D E= ax.Uint32) && > - (ExtendedTable->ProcessorFlag & (1 << PlatformId))= ) { > - // > - // Find one > - // > - CorrectMicrocode =3D TRUE; > - ProcessorFlags =3D ExtendedTable->ProcessorFlag; > - break; > - } > + CorrectMicrocode =3D TRUE; > + ProcessorFlags =3D ExtendedTable->ProcessorFlag; > + break; > } > - ExtendedTable ++; > } > + ExtendedTable ++; > } > } > } > } > - } else { > - // > - // It is the padding data between the microcode patches for microc= ode > patches alignment. > - // Because the microcode patch is the multiple of 1-KByte, the pad= ding > data should not > - // exist if the microcode patch alignment value is not larger than= 1-KByte. > So, the microcode > - // alignment value should be larger than 1-KByte. We could skip SI= ZE_1KB > padding data to > - // find the next possible microcode patch header. > - // > - MicrocodeEntryPoint =3D (CPU_MICROCODE_HEADER *) (((UINTN) > MicrocodeEntryPoint) + SIZE_1KB); > - continue; > } > + > // > // Get the next patch. > // > -- > 2.16.2.windows.1 >=20 > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel