Re: [edk2-devel] [Patch v10 2/2] CryptoPkg/BaseHashApiLib: Implement Unified Hash Calculation API

["Laszlo Ersek" <lersek@redhat.com>]

Hi,

sorry I'm late to this discussion. I'd only like to mention a potential
future improvement:

On 02/04/20 00:35, Michael D Kinney wrote:

> +[PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
> +  ## This PCD indicates the HASH algorithm to calculate hash of data
> +  #  Based on the value set, the required algorithm is chosen to calculate
> +  #  the hash of data.<BR>
> +  #  The default hashing algorithm for BaseHashApiLib is set to SHA256.<BR>
> +  #     0x00000001    - MD4.<BR>
> +  #     0x00000002    - MD5.<BR>
> +  #     0x00000003    - SHA1.<BR>
> +  #     0x00000004    - SHA256.<BR>
> +  #     0x00000005    - SHA384.<BR>
> +  #     0x00000006    - SHA512.<BR>
> +  #     0x00000007    - SM3_256.<BR>
> +  # @Prompt Set policy for hashing unsigned image for Secure Boot.
> +  # @ValidRange 0x80000001 | 0x00000001 - 0x00000007
> +  gEfiCryptoPkgTokenSpaceGuid.PcdHashApiLibPolicy|0x04|UINT8|0x00000001
> +

The platform may choose to make this PCD dynamic or dynamicEx. That's
good. But:

> +UINTN
> +EFIAPI
> +HashApiGetContextSize (
> +  VOID
> +  )
> +{
> +  switch (PcdGet8 (PcdHashApiLibPolicy)) {
> +    case HASH_API_ALGO_MD4:
> +      return Md4GetContextSize ();
> +      break;

we have direct PcdGet8() calls in the lib API implementations. And:

> +[Defines]
> +  INF_VERSION                    = 0x00010005
> +  BASE_NAME                      = BaseHashApiLib
> +  MODULE_UNI_FILE                = BaseHashApiLib.uni
> +  FILE_GUID                      = B1E566DD-DE7C-4F04-BDA0-B1295D3BE927
> +  MODULE_TYPE                    = BASE
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = BaseHashApiLib

[...]

> +[Pcd]
> +  gEfiCryptoPkgTokenSpaceGuid.PcdHashApiLibPolicy    ## CONSUMES

The lib class is not restricted to any particular firmware phase, or
module type.

This suggests that the lib instance is usable in DXE runtime drivers or
SMM drivers. If the serives are called outside of the entry point
functions, the dynamic PCD fetches would be a problem, I think.

So the idea here would be to create a minimal separate INF file + C file
for runtime applications (runtime DXE and SMM drivers), and there a
constructor function could run PcdGet8(), and stash the value in a
global variable.

Alternatively, if this is overkill, we could improve safety by restricting

  LIBRARY_CLASS = BaseHashApiLib|<module_type> <module_type> ...

to every module type except runtime DXE drivers and SMM drivers.

Thanks
Laszlo