Re: [PATCH v3 0/1] SecurityPkg/BaseHashLib: Implement Unified Hash Calculation API

["Sukerkar, Amol N" <amol.n.sukerkar@intel.com>]

Hi Mike,

This design does not implement any registration. The hashing algorithm is selected from the array index specified by PcdSystemHashPolicy value, just like switch..case, based on recommendation by Jian. Are you referring to the document attached to Bugzilla ticket mentioned below? I plan to update it as soon as we agree on the final design. Apologies if it was misleading today.

The reason this lib was added to SecurityPkg and not CryptoPkg was done because the decision to choose hashing algorithm is based on PCD, PcdSystemHashPolicy. CryptoPkg only provides API for accessing specific hashing algorithm and there is no mechanism to choose, as there is no precedent to using a PCD in CryptoPkg and it does not look like that needs to change. On the other hand, we actually do have API support in SecurityPkg (HashInstanceLib). Our design provides similar API support, although, it is much simpler and does not involve registration as in HashInstanceLib. Do you still think this lib should be implemented in CryptoPkg? If yes, how do you propose the user choose the desired hashing mechanism?

Thanks,
Amol

-----Original Message-----
From: Kinney, Michael D <michael.d.kinney@intel.com> 
Sent: Wednesday, January 15, 2020 4:52 PM
To: Sukerkar, Amol N <amol.n.sukerkar@intel.com>; devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Agrawal, Sachin <sachin.agrawal@intel.com>; Musti, Srinivas <srinivas.musti@intel.com>; Lakkimsetti, Subash <subash.lakkimsetti@intel.com>
Subject: RE: [PATCH v3 0/1] SecurityPkg/BaseHashLib: Implement Unified Hash Calculation API

Amol,

I still think the handle based registration is too complex for this feature.

I recommend a simpler lib design and add it to CryptoPkg instead of SecurityPkg.  Providing a different method to access the hashing functions in BaseCryptLib is not a Security feature, it is a Crypto feature.

Thanks,

Mike


> -----Original Message-----
> From: Sukerkar, Amol N <amol.n.sukerkar@intel.com>
> Sent: Wednesday, January 15, 2020 2:57 PM
> To: devel@edk2.groups.io
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Yao, Jiewen 
> <jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Agrawal, 
> Sachin <sachin.agrawal@intel.com>; Musti, Srinivas 
> <srinivas.musti@intel.com>; Lakkimsetti, Subash 
> <subash.lakkimsetti@intel.com>
> Subject: [PATCH v3 0/1] SecurityPkg/BaseHashLib:
> Implement Unified Hash Calculation API
> 
> Currently, the UEFI drivers using the SHA/SM3 hashing algorithms use 
> hard-coded API to calculate the hash, for instance, sha_256(...), etc. 
> Since SHA384 and/or
> SM3_256 are being increasingly adopted for robustness, it becomes 
> cumbersome to modify each driver that calls into hash calculating API.
> 
> To better achieve this, we are proposing a Unified API, which can be 
> used by UEFI drivers, that provides the drivers with flexibility to 
> use the desired hashing algorithm based on the required robnustness.
> 
> Alternatively, the design document is also attached to Bugzilla, 
> https://bugzilla.tianocore.org/show_bug.cgi?id=2151.
> 
> Sukerkar, Amol N (1):
>   SecurityPkg/BaseHashLib: Implement Unified Hash Calculation API
> 
>  SecurityPkg/Library/BaseHashLib/BaseHashLibCommon.c |
> 151 ++++++++++++++++++++
>  SecurityPkg/Library/BaseHashLib/BaseHashLibDxe.c    |
> 100 +++++++++++++
>  SecurityPkg/Library/BaseHashLib/BaseHashLibPei.c    |
> 103 +++++++++++++
>  SecurityPkg/Include/Library/BaseHashLib.h           |
> 85 +++++++++++
>  SecurityPkg/Library/BaseHashLib/BaseHashLibCommon.h |
> 141 ++++++++++++++++++
>  SecurityPkg/Library/BaseHashLib/BaseHashLibDxe.inf  |
> 46 ++++++
>  SecurityPkg/Library/BaseHashLib/BaseHashLibDxe.uni  |
> 17 +++
>  SecurityPkg/Library/BaseHashLib/BaseHashLibPei.inf  |
> 51 +++++++
>  SecurityPkg/Library/BaseHashLib/BaseHashLibPei.uni  |
> 16 +++
>  SecurityPkg/SecurityPkg.dec                         |
> 23 ++-
>  SecurityPkg/SecurityPkg.dsc                         |
> 10 +-
>  SecurityPkg/SecurityPkg.uni                         |
> 15 +-
>  12 files changed, 755 insertions(+), 3 deletions(-)  create mode 
> 100644 SecurityPkg/Library/BaseHashLib/BaseHashLibCommon.c
>  create mode 100644
> SecurityPkg/Library/BaseHashLib/BaseHashLibDxe.c
>  create mode 100644
> SecurityPkg/Library/BaseHashLib/BaseHashLibPei.c
>  create mode 100644
> SecurityPkg/Include/Library/BaseHashLib.h
>  create mode 100644
> SecurityPkg/Library/BaseHashLib/BaseHashLibCommon.h
>  create mode 100644
> SecurityPkg/Library/BaseHashLib/BaseHashLibDxe.inf
>  create mode 100644
> SecurityPkg/Library/BaseHashLib/BaseHashLibDxe.uni
>  create mode 100644
> SecurityPkg/Library/BaseHashLib/BaseHashLibPei.inf
>  create mode 100644
> SecurityPkg/Library/BaseHashLib/BaseHashLibPei.uni
> 
> --
> 2.16.2.windows.1