Re: [edk2-devel] [PATCH v4 3/5] OvmfPkg: Add RngLib based on TimerLib for Crypto
On 8/12/20 4:05 AM, Laszlo Ersek wrote: Arguably, the "Pkg" infix in the following file names: Bhyve/BhyvePkgDefines.fdf.inc Bhyve/BhyvePkgX64.dsc Bhyve/BhyvePkgX64.fdf remains a bit confusing, and should indeed be removed: Bhyve/BhyveDefines.fdf.inc Bhyve/BhyveX64.dsc Bhyve/BhyveX64.fdf Rebecca, could you please submit a patch with such renames? Yes, I'll submit a patch in the next couple of days - I'm still catching up from traveling this past week. -- Rebecca Cran -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#64297): https://edk2.groups.io/g/devel/message/64297 Mute This Topic: https://groups.io/mt/76119014/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 1/1] edk2-platforms: Deduplicate RISC-V SMBIOS
Hi Leif, Great, thanks! Yes, we'll fold it into the commits that introduced the mess in the first place. I just didn't want to make this effort before you sign off on the refactoring. And hope it was easier to review this way. Yes, we dropped some tables because we rethought how to apply smbios to this flexible RISC-V SoC, which doesn't really fit into the rigid view smbios has of hardware. Cheers, Daniel From: devel@edk2.groups.io on behalf of Leif Lindholm Sent: Friday, August 14, 2020 15:40 To: devel@edk2.groups.io ; Schaefer, Daniel Cc: Chang, Abner (HPS SW/FW Technologist) ; Chen, Gilbert ; Michael D Kinney ; Ard Biesheuvel Subject: Re: [edk2-devel] [PATCH 1/1] edk2-platforms: Deduplicate RISC-V SMBIOS Hi Daniel, Thanks for this rework. It looks a massive improvement. On Fri, Aug 07, 2020 at 18:44:44 +0200, Daniel Schaefer wrote: > There was too much code, which wasn't called but it could have generated > those SMBIOS table entries: > > - Type 4 for each core (4xU51, 1xE51) > - Type 7 L1 instruction/data for each core > - Type 7 L2 for U54 > - Type 44 for each core > - Type 4 for the coreplex > - Type 7 L2 for the coreplex > > Now it only has code for those entries: > > - Type 4 for SOC [1x] > - Type 7 L1 for SOC [1x] (even though every hart has own L1, but my Laptop's > Intel i5 does that also) > - Type 7 L2 for SOC [1x] > - Type 44 for each hart, associated with CPU [5x] > > In addition to simplifying the SMBIOS tables, the code for U54 and E51 is > combined, like Leif suggested in his review. > > Here's what happened to the files: > > Expanded: > > - > Platform/RISC-V/PlatformPkg/Library/FirmwareContextProcessorSpecificLib/FirmwareContextProcessorSpecificLib.c > > Deleted file: > > - Silicon/SiFive/E51/Library/PeiCoreInfoHobLib/CoreInfoHob.c > - Silicon/SiFive/U54MCCoreplex/Library/PeiCoreInfoHobLib/CoreInfoHob.c > > Merged with E51 code into single file: > > - Silicon/SiFive/U54/Library/PeiCoreInfoHobLib/CoreInfoHob.c > > Added SMBIOS Type 7 for L1 Cache, removed duplicated SMBIOS (Type 4 and 7 > code): > > - Platform/SiFive/U5SeriesPkg/Library/PeiCoreInfoHobLib/CoreInfoHob.c > > Cc: Abner Chang > Cc: Gilbert Chen > Cc: Leif Lindholm > Cc: Michael D Kinney > Cc: Ard Biesheuvel > --- > Silicon/SiFive/SiFive.dec | 2 - > .../FreedomU500VC707Board/U500.dsc| 1 - > .../FreedomU540HiFiveUnleashedBoard/U540.dsc | 1 - > .../PeiCoreInfoHobLib/PeiCoreInfoHobLib.inf | 1 - > .../PeiCoreInfoHobLib/PeiCoreInfoHobLib.inf | 47 > .../PeiCoreInfoHobLib/PeiCoreInfoHobLib.inf | 4 + > .../PeiCoreInfoHobLib/PeiCoreInfoHobLib.inf | 46 > .../FirmwareContextProcessorSpecificLib.h | 11 + > .../Include/ProcessorSpecificHobData.h| 3 +- > Silicon/SiFive/Include/Library/SiFiveE51.h| 60 - > Silicon/SiFive/Include/Library/SiFiveU54.h| 50 ++-- > .../Include/Library/SiFiveU54MCCoreplex.h | 55 > .../FirmwareContextProcessorSpecificLib.c | 26 ++ > .../Universal/Pei/PlatformPei/Platform.c | 2 +- > .../Universal/Pei/PlatformPei/Platform.c | 2 +- > .../Library/PeiCoreInfoHobLib/CoreInfoHob.c | 58 + > .../Library/PeiCoreInfoHobLib/CoreInfoHob.c | 235 - > .../Library/PeiCoreInfoHobLib/CoreInfoHob.c | 244 +++--- > .../Library/PeiCoreInfoHobLib/CoreInfoHob.c | 184 - > 19 files changed, 178 insertions(+), 854 deletions(-) I know you dropped some tables, but that's a *nice* diffstat. I guess this is effectively meant to be folded into existing commits? If so: Reviewed-by: Leif Lindholm If not, I might start grumbling about some unrelated cleanup in this patch... / Leif -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#64296): https://edk2.groups.io/g/devel/message/64296 Mute This Topic: https://groups.io/mt/76053055/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 1/1] CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest interface
I replaced the MD5 and SHAx functions with EVP functions in Hash2DxeCrypto, and it grew from ~26k to ~253k. -- Christopher Zurcher > -Original Message- > From: Yao, Jiewen > Sent: Thursday, August 13, 2020 07:38 > To: Laszlo Ersek ; devel@edk2.groups.io; Zurcher, > Christopher J > Cc: Wang, Jian J ; Lu, XiaoyuX > Subject: RE: [edk2-devel] [PATCH 1/1] CryptoPkg/BaseCryptLib: Add EVP > (Envelope) Digest interface > > 1) Agree with Laszlo. > We need extend internal protocol/ppi for this new API. > > 2) Do you have any data on the size difference between old SHA implementation > or new MD implementation? > > Thank you > Yao Jiewen > > > -Original Message- > > From: Laszlo Ersek > > Sent: Thursday, August 13, 2020 5:47 PM > > To: devel@edk2.groups.io; Zurcher, Christopher J > > > > Cc: Yao, Jiewen ; Wang, Jian J > ; > > Lu, XiaoyuX > > Subject: Re: [edk2-devel] [PATCH 1/1] CryptoPkg/BaseCryptLib: Add EVP > > (Envelope) Digest interface > > > > Hi Christopher, > > > > (+Mike, > > > > On 08/13/20 03:20, Zurcher, Christopher J wrote: > > > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2545 > > > > > > The EVP interface should be used in place of discrete digest function > > > calls. > > > > > > Cc: Jiewen Yao > > > Cc: Jian J Wang > > > Cc: Xiaoyu Lu > > > Signed-off-by: Christopher J Zurcher > > > --- > > > CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf| 1 + > > > CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf | 1 + > > > CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf | 1 + > > > CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf | 1 + > > > CryptoPkg/Include/Library/BaseCryptLib.h | 122 +++ > > > CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c| 228 > > > > > 6 files changed, 354 insertions(+) > > > > (1) This patch extends the library class header, but updates only one > > *set* of the three library instance *sets*. The other two instance > > *sets* are: > > > > - BaseCryptLibNull (just one instance), for which it should not be hard > > to provide Null implementations of the new functions; > > > > - BaseCryptLibOnProtocolPpi (three instances -- Pei, Dxe, Smm). > > > > > > BaseCryptLibOnProtocolPpi is a tough nut, because it seems to require > > extending: > > > > - the crypto service driver at CryptoPkg/Driver/, > > > > - the interface to that driver (CryptoPkg/Private/Protocol/Crypto.h -- > > reused by both "CryptoPkg/Private/Ppi/Crypto.h" and > > "CryptoPkg/Private/Protocol/SmmCrypto.h"), > > > > - the PCD_CRYPTO_SERVICE_FAMILY_ENABLE structure at > > "CryptoPkg/Include/Pcd/PcdCryptoServiceFamilyEnable.h", for configuring > > the driver, > > > > - the various PcdCryptoServiceFamilyEnable settings / build profiles in > > CryptoPkg/CryptoPkg.dsc. > > > > > > > > > > diff --git a/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf > > b/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf > > > index 4aae2aba95..3968f29412 100644 > > > --- a/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf > > > +++ b/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf > > > @@ -50,6 +50,7 @@ > > >Pk/CryptAuthenticode.c > > >Pk/CryptTs.c > > >Pem/CryptPem.c > > > + Evp/CryptEvpMd.c > > > > > >SysCall/CrtWrapper.c > > >SysCall/TimerWrapper.c > > > diff --git a/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf > > b/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf > > > index dc28e3a11d..d0b91716d0 100644 > > > --- a/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf > > > +++ b/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf > > > @@ -57,6 +57,7 @@ > > >Pk/CryptTsNull.c > > >Pem/CryptPemNull.c > > >Rand/CryptRandNull.c > > > + Evp/CryptEvpMd.c > > > > > >SysCall/CrtWrapper.c > > >SysCall/ConstantTimeClock.c > > > diff --git a/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf > > b/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf > > > index 5005beed02..9f3accd35b 100644 > > > --- a/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf > > > +++ b/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf > > > @@ -56,6 +56,7 @@ > > >Pk/CryptAuthenticodeNull.c > > >Pk/CryptTsNull.c > > >Pem/CryptPem.c > > > + Evp/CryptEvpMd.c > > > > > >SysCall/CrtWrapper.c > > >SysCall/TimerWrapper.c > > > diff --git a/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf > > b/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf > > > index 91ec3e03bf..420623cdc6 100644 > > > --- a/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf > > > +++ b/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf > > > @@ -54,6 +54,7 @@ > > >Pk/CryptAuthenticodeNull.c > > >Pk/CryptTsNull.c > > >Pem/CryptPem.c > > > + Evp/CryptEvpMd.c > > > > > >SysCall/CrtWrapper.c > > >SysCall/ConstantTimeClock.c > > > diff --git a/CryptoPkg/Include/Library/BaseCryptLib.h > > b/CryptoPkg/Include/Library/BaseCryptLib.h > > > index ae9bde9e37..f3bf8aac0c 100644 > > > --- a/CryptoPkg/Include/Library/BaseCryptLib.h > > > +++ b/CryptoPkg/Include/Lib
Re: [edk2-devel] [PATCH v7 0/5] Use RngLib instead of TimerLib for OpensslLib
Thanks Mike, I've addressed all your comments. I'll wait for a few more people to weigh in before I send out v8. -Matthew Carlson On Thu, Aug 13, 2020 at 6:12 PM Kinney, Michael D < michael.d.kin...@intel.com> wrote: > Hi Matt, > > BaseRngLibTimerLib > === > Thank you for updating BaseRngLibTimerLib to use > GetPerformanceCounterProperties(). > StartValue and EndValue are OPTIONAL, so the function DecentDelay() can be > simplified > to remove the StartValue and EndValue local variables and get the rate of > the counter > using the following: > > // Get the counter properties > CounterHz = GetPerformanceCounterProperties (NULL, NULL); > > When you compute the min delay, I see the formula will generate a value of > 0 when > the rate of the performance counter is greater than 1.5MHz. > MicroSecondDelay() > may return immediately if MicroSeconds is 0. Is this your intended > behavior? > Or did you want to make sure the min value is 1 such as: > > MinumumDelayInMicroSeconds = MAX (150 / CounterHz, 1); > > CounterHz is also type UINT64 so this is a 64-bit divide operation that > must > use the BaseLib function DivU64x64Remainder() for 32-bit builds. > > MinumumDelayInMicroSeconds = MAX (DivU64x64Remainder (150, > CounterHz, NULL), 1); > > The function DecentDelay() may interact with HW to get the performance > counter > rate and then do the divide operation. For the RngLib APIs that need the > delay, > I recommend you call DecentDelay() to get the MinumumDelayInMicroSeconds > into > a local variable and then use that value for calls to MicroSecondDelay() > in the > RngLib APIs. > > The comments in the RngLib APIs that describe the length of the delays in > uS/mS > need to be updated because the length of the delay is computed. Update > with > a more generic comment to perform a minimum delay to guarantee a different > performance counter value. > > The UNI file header and strings need to be updated to match INF/C files. > > > DxeRngLib > == > 1) Please add a UNI file for this lib. > > Best regards, > > Mike > > > -Original Message- > > From: devel@edk2.groups.io On Behalf Of Matthew > Carlson > > Sent: Thursday, August 13, 2020 12:45 PM > > To: devel@edk2.groups.io > > Cc: Ard Biesheuvel ; Anthony Perard < > anthony.per...@citrix.com>; Yao, Jiewen > > ; Wang, Jian J ; Julien > Grall ; Justen, Jordan L > > ; Laszlo Ersek ; Gao, > Liming ; Leif Lindholm > > ; Kinney, Michael D ; > Lu, XiaoyuX ; Liu, Zhiguang > > ; Sean Brogan ; > Matthew Carlson > > Subject: [edk2-devel] [PATCH v7 0/5] Use RngLib instead of TimerLib for > OpensslLib > > > > From: Matthew Carlson > > > > Hello all, > > > > This patch contains a fix for Bugzilla 1871. > > There's been a good bit of community discussion around the topic, > > so below follows a general overview of the discussion and what this > patch does. > > > > This is the seventh iteration of this patch series, focused on code > style and a > > few functions being renamed to comply with style. > > > > Back in Devel message#40590 ( > https://edk2.groups.io/g/devel/message/40590) > > around the patch series that updates OpenSSL to 1.1.1b, a comment was > made > > that suggested that platforms be in charge of the entropy/randomness that > > is provided to OpenSSL as currently the entropry source seems to be a > > hand-rolled random number generator that uses the PerformanceCounter from > > TimerLib. This causes OpenSSL to depend on TimerLib, which is often > platform > > specific. In addition to being a potentially weaker source of randomness, > > this also poses a challenge to compile BaseCryptLibOnProtocol with a > platform- > > agnostic version of TimerLib that works universally. > > > > The solution here is to allow platform to specify their source of > entropy in > > addition to providing two new RngLibs: one that uses the TimerLib as > well as > > one that uses RngProtocol to provide randomness. Then the decision to use > > RDRAND or other entropy sources is up to the platform. Mixing various > entropy > > sources is the onus of the platform. It has been suggested on > Devel#40590 and > > BZ#1871 that there should be mixing of the PerformanceCounter and RDRAND > using > > something similar to the yarrow alogirthm that FreeBSD uses for example. > This > > patch series doesn't offer an RngLib that offers that sort of mixing as > the > > ultimate source of random is defined by the platform. > > > > This patch series offers three benefits: > > 1. Dependency reduction: Removes the need for a platform specific timer > > library. We publish a single binary used on numerous platforms for > > crypto and the introduced timer lib dependency caused issues because we > > could not fulfill our platform needs with one library instance. > > > > 2. Code maintenance: Removing this additional code and leveraging an > existing > > library within Edk2 means less code to maintain. > > > > 3. Platform defined quality: A platform can choose wh
Re: [edk2-devel] [PATCH 1/1] edk2-platforms: Deduplicate RISC-V SMBIOS
Hi Daniel, Thanks for this rework. It looks a massive improvement. On Fri, Aug 07, 2020 at 18:44:44 +0200, Daniel Schaefer wrote: > There was too much code, which wasn't called but it could have generated > those SMBIOS table entries: > > - Type 4 for each core (4xU51, 1xE51) > - Type 7 L1 instruction/data for each core > - Type 7 L2 for U54 > - Type 44 for each core > - Type 4 for the coreplex > - Type 7 L2 for the coreplex > > Now it only has code for those entries: > > - Type 4 for SOC [1x] > - Type 7 L1 for SOC [1x] (even though every hart has own L1, but my Laptop's > Intel i5 does that also) > - Type 7 L2 for SOC [1x] > - Type 44 for each hart, associated with CPU [5x] > > In addition to simplifying the SMBIOS tables, the code for U54 and E51 is > combined, like Leif suggested in his review. > > Here's what happened to the files: > > Expanded: > > - > Platform/RISC-V/PlatformPkg/Library/FirmwareContextProcessorSpecificLib/FirmwareContextProcessorSpecificLib.c > > Deleted file: > > - Silicon/SiFive/E51/Library/PeiCoreInfoHobLib/CoreInfoHob.c > - Silicon/SiFive/U54MCCoreplex/Library/PeiCoreInfoHobLib/CoreInfoHob.c > > Merged with E51 code into single file: > > - Silicon/SiFive/U54/Library/PeiCoreInfoHobLib/CoreInfoHob.c > > Added SMBIOS Type 7 for L1 Cache, removed duplicated SMBIOS (Type 4 and 7 > code): > > - Platform/SiFive/U5SeriesPkg/Library/PeiCoreInfoHobLib/CoreInfoHob.c > > Cc: Abner Chang > Cc: Gilbert Chen > Cc: Leif Lindholm > Cc: Michael D Kinney > Cc: Ard Biesheuvel > --- > Silicon/SiFive/SiFive.dec | 2 - > .../FreedomU500VC707Board/U500.dsc| 1 - > .../FreedomU540HiFiveUnleashedBoard/U540.dsc | 1 - > .../PeiCoreInfoHobLib/PeiCoreInfoHobLib.inf | 1 - > .../PeiCoreInfoHobLib/PeiCoreInfoHobLib.inf | 47 > .../PeiCoreInfoHobLib/PeiCoreInfoHobLib.inf | 4 + > .../PeiCoreInfoHobLib/PeiCoreInfoHobLib.inf | 46 > .../FirmwareContextProcessorSpecificLib.h | 11 + > .../Include/ProcessorSpecificHobData.h| 3 +- > Silicon/SiFive/Include/Library/SiFiveE51.h| 60 - > Silicon/SiFive/Include/Library/SiFiveU54.h| 50 ++-- > .../Include/Library/SiFiveU54MCCoreplex.h | 55 > .../FirmwareContextProcessorSpecificLib.c | 26 ++ > .../Universal/Pei/PlatformPei/Platform.c | 2 +- > .../Universal/Pei/PlatformPei/Platform.c | 2 +- > .../Library/PeiCoreInfoHobLib/CoreInfoHob.c | 58 + > .../Library/PeiCoreInfoHobLib/CoreInfoHob.c | 235 - > .../Library/PeiCoreInfoHobLib/CoreInfoHob.c | 244 +++--- > .../Library/PeiCoreInfoHobLib/CoreInfoHob.c | 184 - > 19 files changed, 178 insertions(+), 854 deletions(-) I know you dropped some tables, but that's a *nice* diffstat. I guess this is effectively meant to be folded into existing commits? If so: Reviewed-by: Leif Lindholm If not, I might start grumbling about some unrelated cleanup in this patch... / Leif -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#64291): https://edk2.groups.io/g/devel/message/64291 Mute This Topic: https://groups.io/mt/76053055/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v4 0/2] Enable EDKII CI support for DynamicTablesPkg
Hi All, Is there anything else needed before this patch series can be merged? If possible, we would like this feature enabled in the edk2-stable202008. Regards, Sami Mujawar -Original Message- From: Sami Mujawar Sent: 07 August 2020 06:30 PM To: devel@edk2.groups.io Cc: Sami Mujawar ; Alexei Fedorov ; sean.bro...@microsoft.com; liming@intel.com; michael.d.kin...@intel.com; bret.barke...@microsoft.com; Ard Biesheuvel ; Matteo Carlini ; Laura Moretta ; nd Subject: [PATCH v4 0/2] Enable EDKII CI support for DynamicTablesPkg The TianoCore EDKII project has introduced a Core CI infrastructure using TianoCore EDKII Tools PIP modules. The v2 patch series at https://edk2.groups.io/g/devel/message/63259 adds support for building DynamicTablesPkg using the EKDII Core CI. Splitting the v2 patch series into 2 separate series. This v4 series contains the patches for enabling Core CI for DynamicTablesPkg in .pytools and .azurepipelines i.e. the last two patches from the v2 series. The v3 series containing the patches for DynamicTablesPkg has already been merged in edk2 master at: - https://github.com/tianocore/edk2/commit/2d0c42fdf2cf1855b0a042ef82d848c7716adefe - https://github.com/tianocore/edk2/commit/e3f8605a23ebe9c54ae2b17819d00e185069667d Ref (v3 series): https://edk2.groups.io/g/devel/topic/patch_v3_0_2_add_edkii_ci/76052351 There is no code change other than splitting the v2 series. The changes for this v4 series can be seen at: https://github.com/samimujawar/edk2/tree/839_dynamictablespkg_ci_v4 Sami Mujawar (2): .pytool: CI Settings to support DynamicTablesPkg .azurepipelines: Add DynamicTablesPkg to CI matrix .azurepipelines/templates/pr-gate-build-job.yml | 3 ++- .pytool/CISettings.py | 2 ++ .pytool/Readme.md | 2 +- 3 files changed, 5 insertions(+), 2 deletions(-) -- 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)' -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#64290): https://edk2.groups.io/g/devel/message/64290 Mute This Topic: https://groups.io/mt/76053995/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH EDK2 v2 1/1] SecurityPkg/DxeImageVerificationLib:Enhanced verification of Offset
On 2020/8/14 16:53, Yao, Jiewen wrote:
>> To Jiewen,
>> Sorry, I don't have environment to reproduce the issue.
>
> Please help me understand, if you don’t have environment to reproduce the
> issue, how do you guarantee that your patch does fix the problem and we don’t
> have any other vulnerabilities?
Hi, Jiewen,
You're right, as I can't reproduce the issue, I can't guarantee my patches can
fix the problem.
And as Laszlo analyzed, my patches can't solve overflow issue indeed.
Sincerely
Wenyi
>
> Thank you
> Yao Jiewen
>
>
>> -Original Message-
>> From: devel@edk2.groups.io On Behalf Of wenyi,xie
>> via groups.io
>> Sent: Friday, August 14, 2020 3:54 PM
>> To: Laszlo Ersek ; devel@edk2.groups.io; Yao, Jiewen
>> ; Wang, Jian J
>> Cc: huangmin...@huawei.com; songdongku...@huawei.com
>> Subject: Re: [edk2-devel] [PATCH EDK2 v2 1/1]
>> SecurityPkg/DxeImageVerificationLib:Enhanced verification of Offset
>>
>> To Laszlo,
>> Thank you for your detailed description, I agree with what you analyzed and
>> I'm
>> OK with your patches, it's
>> correct and much simpler.
>>
>> To Jiewen,
>> Sorry, I don't have environment to reproduce the issue.
>>
>> Thanks
>> Wenyi
>>
>> On 2020/8/14 2:50, Laszlo Ersek wrote:
>>> On 08/13/20 13:55, Wenyi Xie wrote:
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2215
There is an integer overflow vulnerability in DxeImageVerificationHandler
function when parsing the PE files attribute certificate table. In cases
where WinCertificate->dwLength is sufficiently large, it's possible to
overflow Offset back to 0 causing an endless loop.
Check offset inbetween VirtualAddress and VirtualAddress + Size.
Using SafeintLib to do offset addition with result check.
Cc: Jiewen Yao
Cc: Jian J Wang
Cc: Laszlo Ersek
Signed-off-by: Wenyi Xie
---
SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf |
>> 1 +
SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.h |
>> 1 +
SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c |
>> 111 +++-
3 files changed, 63 insertions(+), 50 deletions(-)
diff --git
>> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf
>> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf
index 1e1a639857e0..a7ac4830b3d4 100644
---
>> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf
+++
>> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf
@@ -53,6 +53,7 @@ [LibraryClasses]
SecurityManagementLib
PeCoffLib
TpmMeasurementLib
+ SafeIntLib
[Protocols]
gEfiFirmwareVolume2ProtocolGuid ## SOMETIMES_CONSUMES
diff --git
>> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.h
>> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.h
index 17955ff9774c..060273917d5d 100644
---
>> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.h
+++
>> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.h
@@ -23,6 +23,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#include
#include
#include
+#include
#include
#include
#include
diff --git
>> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
>> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
index 36b87e16d53d..dbc03e28c05b 100644
--- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
+++
>> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
@@ -1658,6 +1658,10 @@ DxeImageVerificationHandler (
EFI_STATUS HashStatus;
EFI_STATUS DbStatus;
BOOLEAN IsFound;
+ UINT32 AlignedLength;
+ UINT32 Result;
+ EFI_STATUS AddStatus;
+ BOOLEAN IsAuthDataAssigned;
SignatureList = NULL;
SignatureListSize = 0;
@@ -1667,6 +1671,7 @@ DxeImageVerificationHandler (
Action= EFI_IMAGE_EXECUTION_AUTH_UNTESTED;
IsVerified= FALSE;
IsFound = FALSE;
+ Result= 0;
//
// Check the image type and get policy setting.
@@ -1850,9 +1855,10 @@ DxeImageVerificationHandler (
// The first certificate starts at offset (SecDataDir->VirtualAddress)
from the
>> start of the file.
//
for (OffSet = SecDataDir->VirtualAddress;
- OffSet < (SecDataDir->VirtualAddress + SecDataDir->Size);
- OffSet += (WinCertificate->dwLength + ALIGN_SIZE (WinCertificate-
>>> dwLength))) {
>
Re: [edk2-devel] [PATCH EDK2 v2 1/1] SecurityPkg/DxeImageVerificationLib:Enhanced verification of Offset
> To Jiewen,
> Sorry, I don't have environment to reproduce the issue.
Please help me understand, if you don’t have environment to reproduce the
issue, how do you guarantee that your patch does fix the problem and we don’t
have any other vulnerabilities?
Thank you
Yao Jiewen
> -Original Message-
> From: devel@edk2.groups.io On Behalf Of wenyi,xie
> via groups.io
> Sent: Friday, August 14, 2020 3:54 PM
> To: Laszlo Ersek ; devel@edk2.groups.io; Yao, Jiewen
> ; Wang, Jian J
> Cc: huangmin...@huawei.com; songdongku...@huawei.com
> Subject: Re: [edk2-devel] [PATCH EDK2 v2 1/1]
> SecurityPkg/DxeImageVerificationLib:Enhanced verification of Offset
>
> To Laszlo,
> Thank you for your detailed description, I agree with what you analyzed and
> I'm
> OK with your patches, it's
> correct and much simpler.
>
> To Jiewen,
> Sorry, I don't have environment to reproduce the issue.
>
> Thanks
> Wenyi
>
> On 2020/8/14 2:50, Laszlo Ersek wrote:
> > On 08/13/20 13:55, Wenyi Xie wrote:
> >> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2215
> >>
> >> There is an integer overflow vulnerability in DxeImageVerificationHandler
> >> function when parsing the PE files attribute certificate table. In cases
> >> where WinCertificate->dwLength is sufficiently large, it's possible to
> >> overflow Offset back to 0 causing an endless loop.
> >>
> >> Check offset inbetween VirtualAddress and VirtualAddress + Size.
> >> Using SafeintLib to do offset addition with result check.
> >>
> >> Cc: Jiewen Yao
> >> Cc: Jian J Wang
> >> Cc: Laszlo Ersek
> >> Signed-off-by: Wenyi Xie
> >> ---
> >> SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf |
> 1 +
> >> SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.h |
> 1 +
> >> SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c |
> 111 +++-
> >> 3 files changed, 63 insertions(+), 50 deletions(-)
> >>
> >> diff --git
> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf
> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf
> >> index 1e1a639857e0..a7ac4830b3d4 100644
> >> ---
> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf
> >> +++
> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf
> >> @@ -53,6 +53,7 @@ [LibraryClasses]
> >>SecurityManagementLib
> >>PeCoffLib
> >>TpmMeasurementLib
> >> + SafeIntLib
> >>
> >> [Protocols]
> >>gEfiFirmwareVolume2ProtocolGuid ## SOMETIMES_CONSUMES
> >> diff --git
> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.h
> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.h
> >> index 17955ff9774c..060273917d5d 100644
> >> ---
> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.h
> >> +++
> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.h
> >> @@ -23,6 +23,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> >> #include
> >> #include
> >> #include
> >> +#include
> >> #include
> >> #include
> >> #include
> >> diff --git
> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> >> index 36b87e16d53d..dbc03e28c05b 100644
> >> --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> >> +++
> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> >> @@ -1658,6 +1658,10 @@ DxeImageVerificationHandler (
> >>EFI_STATUS HashStatus;
> >>EFI_STATUS DbStatus;
> >>BOOLEAN IsFound;
> >> + UINT32 AlignedLength;
> >> + UINT32 Result;
> >> + EFI_STATUS AddStatus;
> >> + BOOLEAN IsAuthDataAssigned;
> >>
> >>SignatureList = NULL;
> >>SignatureListSize = 0;
> >> @@ -1667,6 +1671,7 @@ DxeImageVerificationHandler (
> >>Action= EFI_IMAGE_EXECUTION_AUTH_UNTESTED;
> >>IsVerified= FALSE;
> >>IsFound = FALSE;
> >> + Result= 0;
> >>
> >>//
> >>// Check the image type and get policy setting.
> >> @@ -1850,9 +1855,10 @@ DxeImageVerificationHandler (
> >>// The first certificate starts at offset (SecDataDir->VirtualAddress)
> >> from the
> start of the file.
> >>//
> >>for (OffSet = SecDataDir->VirtualAddress;
> >> - OffSet < (SecDataDir->VirtualAddress + SecDataDir->Size);
> >> - OffSet += (WinCertificate->dwLength + ALIGN_SIZE (WinCertificate-
> >dwLength))) {
> >> + (OffSet >= SecDataDir->VirtualAddress) && (OffSet < (SecDataDir-
> >VirtualAddress + SecDataDir->Size));) {
> >> +IsAuthDataAssigned = FALSE;
> >> WinCertificate = (WIN_CERTIFICATE *) (mImageBase + OffSet);
> >> +AlignedLength = WinCertificate->dwLength + ALIGN_SIZE (W
[edk2-devel] Soft Feature Freeze starts now for edk2-stable202008
Hi, all We will enter into Soft Feature Freeze phase. In this phase, the feature under review will not be allowed to be pushed. The feature passed review can still be merged. Now, two features have passed code review. If no other comments, I will merge them into this stable tag. They are: https://edk2.groups.io/g/devel/message/64176 SEV-ES guest support, https://edk2.groups.io/g/devel/message/64283 Add a plugin to check Ecc issues for edk2 on open ci. The patch review can continue without break in edk2 community. If the patch is sent before Soft Feature Freeze, and plans to catch this stable tag, the patch contributor need reply to his patch and notify edk2 community. If the patch is sent after Soft Feature Freeze, and plans to catch this stable tag, please add edk2-stable202008 key words in the patch title and BZ, so the community know this patch target and give the feedback. Below is edk2-stable202008 tag planning https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Release-Planning Proposed Schedule Date (00:00:00 UTC-8) Description 2020-06-03 Beginning of development 2020-08-07 Feature Planning Freeze 2020-08-14 Soft Feature Freeze 2020-08-21 Hard Feature Freeze 2020-08-28 Release Thanks Liming -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#64287): https://edk2.groups.io/g/devel/message/64287 Mute This Topic: https://groups.io/mt/76184214/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v10 02/16] .pytool/Plugin: Add a plugin EccCheck
Reviewed-by: Liming Gao
-Original Message-
From: Zhang, Shenglei
Sent: 2020年8月14日 15:44
To: devel@edk2.groups.io
Cc: Sean Brogan ; Bret Barkelew
; Kinney, Michael D ;
Gao, Liming
Subject: [PATCH v10 02/16] .pytool/Plugin: Add a plugin EccCheck
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2606
EccCheck is a plugin to report Ecc issues for code in pull request , which will
be run on open ci.
But note not each kind of issue could be reported out.
It can only handle the issues, whose line number in CSV report accurately map
with their code in source code files. And Ecc issues about comments can also be
handled.
Cc: Sean Brogan
Cc: Bret Barkelew
Cc: Michael D Kinney
Cc: Liming Gao
Signed-off-by: Shenglei Zhang
---
.pytool/Plugin/EccCheck/EccCheck.py | 309 ++
.pytool/Plugin/EccCheck/EccCheck_plug_in.yaml | 11 +
.pytool/Plugin/EccCheck/Readme.md | 15 +
3 files changed, 335 insertions(+)
create mode 100644 .pytool/Plugin/EccCheck/EccCheck.py
create mode 100644 .pytool/Plugin/EccCheck/EccCheck_plug_in.yaml
create mode 100644 .pytool/Plugin/EccCheck/Readme.md
diff --git a/.pytool/Plugin/EccCheck/EccCheck.py
b/.pytool/Plugin/EccCheck/EccCheck.py
new file mode 100644
index 00..eee1ff7a77
--- /dev/null
+++ b/.pytool/Plugin/EccCheck/EccCheck.py
@@ -0,0 +1,309 @@
+# @file EccCheck.py
+#
+# Copyright (c) 2020, Intel Corporation. All rights reserved. #
+SPDX-License-Identifier: BSD-2-Clause-Patent ##
+
+import os
+import shutil
+import re
+import csv
+import xml.dom.minidom
+from typing import List, Dict, Tuple
+import logging
+from io import StringIO
+from edk2toolext.environment import shell_environment from
+edk2toolext.environment.plugintypes.ci_build_plugin import
+ICiBuildPlugin from edk2toolext.environment.var_dict import VarDict
+from edk2toollib.utility_functions import RunCmd
+
+
+class EccCheck(ICiBuildPlugin):
+"""
+A CiBuildPlugin that finds the Ecc issues of newly added code in pull
request.
+
+Configuration options:
+"EccCheck": {
+"ExceptionList": [],
+"IgnoreFiles": []
+},
+"""
+
+ReModifyFile = re.compile(r'[B-Q,S-Z]+[\d]*\t(.*)')
+FindModifyFile = re.compile(r'\+\+\+ b\/(.*)')
+LineScopePattern = (r'@@ -\d*\,*\d* \+\d*\,*\d* @@.*')
+LineNumRange = re.compile(r'@@ -\d*\,*\d* \+(\d*)\,*(\d*) @@.*')
+
+def GetTestName(self, packagename: str, environment: VarDict) -> tuple:
+""" Provide the testcase name and classname for use in reporting
+testclassname: a descriptive string for the testcase can include
whitespace
+classname: should be patterned
+ ..
+
+Args:
+ packagename: string containing name of package to build
+ environment: The VarDict for the test to run in
+Returns:
+a tuple containing the testcase name and the classname
+(testcasename, classname)
+"""
+return ("Check for efi coding style for " + packagename,
+ packagename + ".EccCheck")
+
+##
+# External function of plugin. This function is used to perform the task
of the ci_build_plugin Plugin
+#
+# - package is the edk2 path to package. This means
workspace/packagepath relative.
+# - edk2path object configured with workspace and packages path
+# - PkgConfig Object (dict) for the pkg
+# - EnvConfig Object
+# - Plugin Manager Instance
+# - Plugin Helper Obj Instance
+# - Junit Logger
+# - output_stream the StringIO output stream from this plugin via logging
+def RunBuildPlugin(self, packagename, Edk2pathObj, pkgconfig, environment,
PLM, PLMHelper, tc, output_stream=None):
+edk2_path = Edk2pathObj.WorkspacePath
+python_path = os.path.join(edk2_path, "BaseTools", "Source", "Python")
+env = shell_environment.GetEnvironment()
+env.set_shell_var('PYTHONPATH', python_path)
+env.set_shell_var('WORKSPACE', edk2_path)
+self.ECC_PASS = True
+self.ApplyConfig(pkgconfig, edk2_path, packagename)
+modify_dir_list = self.GetModifyDir(packagename)
+patch = self.GetDiff(packagename)
+ecc_diff_range = self.GetDiffRange(patch, packagename, edk2_path)
+self.GenerateEccReport(modify_dir_list, ecc_diff_range, edk2_path)
+ecc_log = os.path.join(edk2_path, "Ecc.log")
+self.RevertCode()
+if self.ECC_PASS:
+tc.SetSuccess()
+self.RemoveFile(ecc_log)
+return 0
+else:
+with open(ecc_log, encoding='utf8') as output:
+ecc_output = output.readlines()
+for line in ecc_output:
+logging.error(line.strip())
+self.RemoveFile(ecc_log)
+tc.SetFailed("EccCheck failed for {0}".format(packagename), "Ecc
detected issues")
+return 1
+
+def RevertCode(self) -> None:
+submoudle_pa
Re: [edk2-devel] [PATCH EDK2 v2 1/1] SecurityPkg/DxeImageVerificationLib:Enhanced verification of Offset
To Laszlo,
Thank you for your detailed description, I agree with what you analyzed and I'm
OK with your patches, it's
correct and much simpler.
To Jiewen,
Sorry, I don't have environment to reproduce the issue.
Thanks
Wenyi
On 2020/8/14 2:50, Laszlo Ersek wrote:
> On 08/13/20 13:55, Wenyi Xie wrote:
>> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2215
>>
>> There is an integer overflow vulnerability in DxeImageVerificationHandler
>> function when parsing the PE files attribute certificate table. In cases
>> where WinCertificate->dwLength is sufficiently large, it's possible to
>> overflow Offset back to 0 causing an endless loop.
>>
>> Check offset inbetween VirtualAddress and VirtualAddress + Size.
>> Using SafeintLib to do offset addition with result check.
>>
>> Cc: Jiewen Yao
>> Cc: Jian J Wang
>> Cc: Laszlo Ersek
>> Signed-off-by: Wenyi Xie
>> ---
>> SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf |
>> 1 +
>> SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.h |
>> 1 +
>> SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c |
>> 111 +++-
>> 3 files changed, 63 insertions(+), 50 deletions(-)
>>
>> diff --git
>> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf
>> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf
>> index 1e1a639857e0..a7ac4830b3d4 100644
>> --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf
>> +++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf
>> @@ -53,6 +53,7 @@ [LibraryClasses]
>>SecurityManagementLib
>>PeCoffLib
>>TpmMeasurementLib
>> + SafeIntLib
>>
>> [Protocols]
>>gEfiFirmwareVolume2ProtocolGuid ## SOMETIMES_CONSUMES
>> diff --git
>> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.h
>> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.h
>> index 17955ff9774c..060273917d5d 100644
>> --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.h
>> +++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.h
>> @@ -23,6 +23,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>> #include
>> #include
>> #include
>> +#include
>> #include
>> #include
>> #include
>> diff --git
>> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
>> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
>> index 36b87e16d53d..dbc03e28c05b 100644
>> --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
>> +++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
>> @@ -1658,6 +1658,10 @@ DxeImageVerificationHandler (
>>EFI_STATUS HashStatus;
>>EFI_STATUS DbStatus;
>>BOOLEAN IsFound;
>> + UINT32 AlignedLength;
>> + UINT32 Result;
>> + EFI_STATUS AddStatus;
>> + BOOLEAN IsAuthDataAssigned;
>>
>>SignatureList = NULL;
>>SignatureListSize = 0;
>> @@ -1667,6 +1671,7 @@ DxeImageVerificationHandler (
>>Action= EFI_IMAGE_EXECUTION_AUTH_UNTESTED;
>>IsVerified= FALSE;
>>IsFound = FALSE;
>> + Result= 0;
>>
>>//
>>// Check the image type and get policy setting.
>> @@ -1850,9 +1855,10 @@ DxeImageVerificationHandler (
>>// The first certificate starts at offset (SecDataDir->VirtualAddress)
>> from the start of the file.
>>//
>>for (OffSet = SecDataDir->VirtualAddress;
>> - OffSet < (SecDataDir->VirtualAddress + SecDataDir->Size);
>> - OffSet += (WinCertificate->dwLength + ALIGN_SIZE
>> (WinCertificate->dwLength))) {
>> + (OffSet >= SecDataDir->VirtualAddress) && (OffSet <
>> (SecDataDir->VirtualAddress + SecDataDir->Size));) {
>> +IsAuthDataAssigned = FALSE;
>> WinCertificate = (WIN_CERTIFICATE *) (mImageBase + OffSet);
>> +AlignedLength = WinCertificate->dwLength + ALIGN_SIZE
>> (WinCertificate->dwLength);
>
> I disagree with this patch.
>
> The primary reason for my disagreement is that the bug report
> is inexact, and
> so this patch tries to fix the wrong thing.
>
> With edk2 master at commit 65904cdbb33c, it is *not* possible to
> overflow the OffSet variable to zero with "WinCertificate->dwLength"
> *purely*, and cause an endless loop. Note that we have (at commit
> 65904cdbb33c):
>
> for (OffSet = SecDataDir->VirtualAddress;
>OffSet < (SecDataDir->VirtualAddress + SecDataDir->Size);
>OffSet += (WinCertificate->dwLength + ALIGN_SIZE
> (WinCertificate->dwLength))) {
> WinCertificate = (WIN_CERTIFICATE *) (mImageBase + OffSet);
> if ((SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) <= sizeof
> (WIN_CERTI
[edk2-devel] [PATCH v10 02/16] .pytool/Plugin: Add a plugin EccCheck
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2606
EccCheck is a plugin to report Ecc issues for code in pull request
, which will be run on open ci.
But note not each kind of issue could be reported out.
It can only handle the issues, whose line number in CSV report
accurately map with their code in source code files. And Ecc issues
about comments can also be handled.
Cc: Sean Brogan
Cc: Bret Barkelew
Cc: Michael D Kinney
Cc: Liming Gao
Signed-off-by: Shenglei Zhang
---
.pytool/Plugin/EccCheck/EccCheck.py | 309 ++
.pytool/Plugin/EccCheck/EccCheck_plug_in.yaml | 11 +
.pytool/Plugin/EccCheck/Readme.md | 15 +
3 files changed, 335 insertions(+)
create mode 100644 .pytool/Plugin/EccCheck/EccCheck.py
create mode 100644 .pytool/Plugin/EccCheck/EccCheck_plug_in.yaml
create mode 100644 .pytool/Plugin/EccCheck/Readme.md
diff --git a/.pytool/Plugin/EccCheck/EccCheck.py
b/.pytool/Plugin/EccCheck/EccCheck.py
new file mode 100644
index 00..eee1ff7a77
--- /dev/null
+++ b/.pytool/Plugin/EccCheck/EccCheck.py
@@ -0,0 +1,309 @@
+# @file EccCheck.py
+#
+# Copyright (c) 2020, Intel Corporation. All rights reserved.
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+##
+
+import os
+import shutil
+import re
+import csv
+import xml.dom.minidom
+from typing import List, Dict, Tuple
+import logging
+from io import StringIO
+from edk2toolext.environment import shell_environment
+from edk2toolext.environment.plugintypes.ci_build_plugin import ICiBuildPlugin
+from edk2toolext.environment.var_dict import VarDict
+from edk2toollib.utility_functions import RunCmd
+
+
+class EccCheck(ICiBuildPlugin):
+"""
+A CiBuildPlugin that finds the Ecc issues of newly added code in pull
request.
+
+Configuration options:
+"EccCheck": {
+"ExceptionList": [],
+"IgnoreFiles": []
+},
+"""
+
+ReModifyFile = re.compile(r'[B-Q,S-Z]+[\d]*\t(.*)')
+FindModifyFile = re.compile(r'\+\+\+ b\/(.*)')
+LineScopePattern = (r'@@ -\d*\,*\d* \+\d*\,*\d* @@.*')
+LineNumRange = re.compile(r'@@ -\d*\,*\d* \+(\d*)\,*(\d*) @@.*')
+
+def GetTestName(self, packagename: str, environment: VarDict) -> tuple:
+""" Provide the testcase name and classname for use in reporting
+testclassname: a descriptive string for the testcase can include
whitespace
+classname: should be patterned ..
+
+Args:
+ packagename: string containing name of package to build
+ environment: The VarDict for the test to run in
+Returns:
+a tuple containing the testcase name and the classname
+(testcasename, classname)
+"""
+return ("Check for efi coding style for " + packagename, packagename +
".EccCheck")
+
+##
+# External function of plugin. This function is used to perform the task
of the ci_build_plugin Plugin
+#
+# - package is the edk2 path to package. This means
workspace/packagepath relative.
+# - edk2path object configured with workspace and packages path
+# - PkgConfig Object (dict) for the pkg
+# - EnvConfig Object
+# - Plugin Manager Instance
+# - Plugin Helper Obj Instance
+# - Junit Logger
+# - output_stream the StringIO output stream from this plugin via logging
+def RunBuildPlugin(self, packagename, Edk2pathObj, pkgconfig, environment,
PLM, PLMHelper, tc, output_stream=None):
+edk2_path = Edk2pathObj.WorkspacePath
+python_path = os.path.join(edk2_path, "BaseTools", "Source", "Python")
+env = shell_environment.GetEnvironment()
+env.set_shell_var('PYTHONPATH', python_path)
+env.set_shell_var('WORKSPACE', edk2_path)
+self.ECC_PASS = True
+self.ApplyConfig(pkgconfig, edk2_path, packagename)
+modify_dir_list = self.GetModifyDir(packagename)
+patch = self.GetDiff(packagename)
+ecc_diff_range = self.GetDiffRange(patch, packagename, edk2_path)
+self.GenerateEccReport(modify_dir_list, ecc_diff_range, edk2_path)
+ecc_log = os.path.join(edk2_path, "Ecc.log")
+self.RevertCode()
+if self.ECC_PASS:
+tc.SetSuccess()
+self.RemoveFile(ecc_log)
+return 0
+else:
+with open(ecc_log, encoding='utf8') as output:
+ecc_output = output.readlines()
+for line in ecc_output:
+logging.error(line.strip())
+self.RemoveFile(ecc_log)
+tc.SetFailed("EccCheck failed for {0}".format(packagename), "Ecc
detected issues")
+return 1
+
+def RevertCode(self) -> None:
+submoudle_params = "submodule update --init"
+RunCmd("git", submoudle_params)
+reset_params = "reset HEAD --hard"
+RunCmd("git", reset_params)
+
+def GetDiff(self, pkg: str) -> List[str]:
+return_buffer = StringIO()
+params = "diff --unifi
[edk2-devel] [PATCH v10 00/16] Add a plugin to check Ecc issues for edk2 on open ci
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2606 As planed we will enable Ecc check for edk2 on open ci. And they are ready now. I appreciate receiving feedback and comments if someone find errors or false positive issues. I created a pipline of EccCheck for my forked edk2. Welcome everyone to create pull request to test the quality of this plugin. My forked tree: https://github.com/shenglei10/edk2 And I also created some test cases for ECC plugin. Below are test cases. https://github.com/shenglei10/edk2/tree/ECC Results can be view in below azure server. https://dev.azure.com/shengleizhang/shengleizhang/_build?definitionId=12&_a=summary Patches 1/16: It's a lib necessary for py3 to run Ecc on azure servers. 2/16: EccCheck.py is a plugin to report Ecc issues for commits. It can be run on azure servers for open ci, or a local virtual environment. 3/16~16/16: We consider some cases that will report out Ecc issues but they won't be fixed, like submodule and industry standard related things. So we add two configuration fields "Exception" and "IgnoreFiles" for people to use. These patches add configuration in yaml files for Ecc check. Cc: Bob Feng Cc: Bret Barkelew Cc: Michael D Kinney Cc: Liming Gao Cc: Sean Brogan v2: Update 1/17, fix the bug that the script can't hanlde multiple commits. v3: Update 1/17, set the only workalbe workspace is edk2 root directory. Update 2/17, designate the version of antlr4 is 4.7.1. Add 4/17~17/17. v4. Update 1/17, remove the function EdksetupRebuild(), instead add function SetupEnvironment(). Update variables' format and type hints to pass flake8 and mypy. v5. Conver the former method to plugin solution, to align with other check points on open ci. v6. The 1/16 patch is missed in v5 series. Now add it in v6. v7. Fix a bug that Ecc plugin can not be run correctly under Linux OS. v8. Enable error code config section to ignore certain kinds of issues, which are always false positive in partial Ecc scaning. All patches except 2/16 have been R-B and are not updated in v8 series. To avoid making noise in community, I only send cover letter and 2/16 patch. v9. Update 2/16, 3/16, 5/16 and 16/16. 1. Enable directory path for "IgnoreFiles" section in xxxPkg.yaml. So that users can skip a certain directory and don't need to fill in with file names. 2. Add submodule pathes in "IgnoreFiles" in MdeModulePkg.ci.yaml, CryptoPkg.ci.yaml and UnitTestFrameworkPkg.ci.yaml. v10. Update 2/16 patch, adding a step to revert code when ecc plugin ends. All patches except 2/16 have been R-B and are not updated in v10 series. To avoid making noise in community, I only send cover letter and 2/16 patch. Shenglei Zhang (16): pip-requirements.txt: Add Ecc required lib .pytool/Plugin: Add a plugin EccCheck MdeModulePkg/MdeModulePkg.ci.yaml: Add configuration for Ecc check ArmVirtPkg/ArmVirtPkg.ci.yaml: Add configuration for Ecc check CryptoPkg/CryptoPkg.ci.yaml: Add configuration for Ecc check EmulatorPkg/EmulatorPkg.ci.yaml: Add configuration for Ecc check FatPkg/FatPkg.ci.yaml: Add configuration for Ecc check FmpDevicePkg/FmpDevicePkg.ci.yaml: Add configuration for Ecc check MdePkg/MdePkg.ci.yaml: Add configuration for Ecc check NetworkPkg/NetworkPkg.ci.yaml: Add configuration for Ecc check OvmfPkg/OvmfPkg.ci.yaml: Add configuration for Ecc check PcAtChipsetPkg/PcAtChipsetPkg.ci.yaml: Add configuration for Ecc check SecurityPkg/SecurityPkg.ci.yaml: Add configuration for Ecc check ShellPkg/ShellPkg.ci.yaml: Add configuration for Ecc check UefiCpuPkg/UefiCpuPkg.ci.yaml: Add configuration for Ecc check UnitTestFrameworkPkg: Add configuration for Ecc check in yaml file .pytool/Plugin/EccCheck/EccCheck.py | 309 ++ .pytool/Plugin/EccCheck/EccCheck_plug_in.yaml | 11 + .pytool/Plugin/EccCheck/Readme.md | 15 + ArmVirtPkg/ArmVirtPkg.ci.yaml | 12 + CryptoPkg/CryptoPkg.ci.yaml | 13 + EmulatorPkg/EmulatorPkg.ci.yaml | 12 + FatPkg/FatPkg.ci.yaml | 12 + FmpDevicePkg/FmpDevicePkg.ci.yaml | 12 + MdeModulePkg/MdeModulePkg.ci.yaml | 14 + MdePkg/MdePkg.ci.yaml | 12 + NetworkPkg/NetworkPkg.ci.yaml | 12 + OvmfPkg/OvmfPkg.ci.yaml | 12 + PcAtChipsetPkg/PcAtChipsetPkg.ci.yaml | 13 + SecurityPkg/SecurityPkg.ci.yaml | 12 + ShellPkg/ShellPkg.ci.yaml | 12 + UefiCpuPkg/UefiCpuPkg.ci.yaml | 12 + .../UnitTestFrameworkPkg.ci.yaml | 12 + pip-requirements.txt | 1 + 18 files changed, 508 insertions(+) create mode 100644 .pytool/Plugin/EccCheck/EccCheck.py create mode 100644 .pytool/Plugin/EccCheck/EccCheck_plug_in.yaml create mode 100644 .pytool/Plugin/EccCheck/Readme
