From: Matthew Carlson <macarl@microsoft.com>

This fixes bugzilla 1871.
See PR here: https://github.com/tianocore/edk2/pull/831

Matthew Carlson (2):
  CryptoPkg: OpensslLib: Use RngLib to generate entropy in rand_pool
  MdePkg: TimerRngLib: Added RngLib that uses TimerLib

 CryptoPkg/Library/OpensslLib/rand_pool.c           | 202 ++------------------
 CryptoPkg/Library/OpensslLib/rand_pool_noise.c     |  29 ---
 CryptoPkg/Library/OpensslLib/rand_pool_noise_tsc.c |  43 -----
 MdePkg/Library/BaseRngLibTimer/RngLibTimer.c       | 153 +++++++++++++++
 CryptoPkg/CryptoPkg.dsc                            |   2 +
 CryptoPkg/Library/OpensslLib/OpensslLib.inf        |  15 +-
 CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf  |  15 +-
 CryptoPkg/Library/OpensslLib/rand_pool_noise.h     |  29 ---
 MdePkg/Library/BaseRngLibTimer/BaseRngLibTimer.inf |  37 ++++
 MdePkg/Library/BaseRngLibTimer/BaseRngLibTimer.uni |  17 ++
 MdePkg/MdePkg.dsc                                  |   1 +
 11 files changed, 230 insertions(+), 313 deletions(-)
 delete mode 100644 CryptoPkg/Library/OpensslLib/rand_pool_noise.c
 delete mode 100644 CryptoPkg/Library/OpensslLib/rand_pool_noise_tsc.c
 create mode 100644 MdePkg/Library/BaseRngLibTimer/RngLibTimer.c
 delete mode 100644 CryptoPkg/Library/OpensslLib/rand_pool_noise.h
 create mode 100644 MdePkg/Library/BaseRngLibTimer/BaseRngLibTimer.inf
 create mode 100644 MdePkg/Library/BaseRngLibTimer/BaseRngLibTimer.uni

-- 
2.27.0.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#63474): https://edk2.groups.io/g/devel/message/63474
Mute This Topic: https://groups.io/mt/75890825/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Hi Matthew,

A few comments:

1) MdePkg/Libray/BaseRngLibTimer
  a) Since this implementation of the RngLib class is layered
     on top of any TimerLib instance a platform selects, the 
     dir/name of the lib should be BasRngLibTimerLib.
  b) BaseRngLibTimer.inf - I see the comment that it should not 
     be used in a production system.  I think you should add the
     reason why that it should not be used in production system
     which is that using the performance counter from a TimerLib
     is not a true source of random values.
  c) BaseRngLibTimer.uni - The file header description is incorrect.
     It refers to CPU RdRand.  The UNI string also makes reference
     to low-quality random numbers.  I am not sure if there is a
     clear definition of low-quality random numbers.  Should update
     to match description in INF.
  d) RngLibTimer.c - This is a library of type BASE.  Should include
     <Base.h> and not <Uefi.h>.  Also <Base.h> should be listed first.
  e) RngLibTimer.c - I see use of calls to MicroSecondDelay() for 
     small values such as 1, 2, 4 without any comments that explain 
     why this call is made and how the values were selected.  One 
     aspect of this is that depending on the rate of the counter
     used by the GetPerformanceCounter(), the use of these small
     values to MicroSecondDelay() may all produce results where the
     counter is only incremented by 1.  This algorithms is more
     effective if the rate of the counter is much larger than 1MHz.
     Should the number microseconds that are used be based on 
     the results from GetPerofrmanceCounterProperties()?

2) CryptoPkg/CryptoPkg.dsc.  The update to this DSC file adds a 
   dependency on the SecurityPkg.  The SecurityPkg can depend on
   the CryptoPkg, but the CryptoPkg can not depend on the SecurityPkg.
   For a package verification build, perhaps we should always use
   BaseRngLibNull.  Platform DSC files can choose to use the real
   RngLib instances.

3) rand_pool.c/RandGetBytes() - Typo in function header.  Return
   values are TRUE and FALSE.

Best regards,

Mike

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On
> Behalf Of Matthew Carlson
> Sent: Thursday, July 30, 2020 10:21 AM
> To: devel@edk2.groups.io
> Subject: [edk2-devel] [Patch v2 0/2] Use RngLib instead
> of TimerLib for OpensslLib
> 
> From: Matthew Carlson <macarl@microsoft.com>
> 
> This fixes bugzilla 1871.
> See PR here: https://github.com/tianocore/edk2/pull/831
> 
> Matthew Carlson (2):
>   CryptoPkg: OpensslLib: Use RngLib to generate entropy
> in rand_pool
>   MdePkg: TimerRngLib: Added RngLib that uses TimerLib
> 
>  CryptoPkg/Library/OpensslLib/rand_pool.c           |
> 202 ++------------------
>  CryptoPkg/Library/OpensslLib/rand_pool_noise.c     |
> 29 ---
>  CryptoPkg/Library/OpensslLib/rand_pool_noise_tsc.c |
> 43 -----
>  MdePkg/Library/BaseRngLibTimer/RngLibTimer.c       |
> 153 +++++++++++++++
>  CryptoPkg/CryptoPkg.dsc                            |
> 2 +
>  CryptoPkg/Library/OpensslLib/OpensslLib.inf        |
> 15 +-
>  CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf  |
> 15 +-
>  CryptoPkg/Library/OpensslLib/rand_pool_noise.h     |
> 29 ---
>  MdePkg/Library/BaseRngLibTimer/BaseRngLibTimer.inf |
> 37 ++++
>  MdePkg/Library/BaseRngLibTimer/BaseRngLibTimer.uni |
> 17 ++
>  MdePkg/MdePkg.dsc                                  |
> 1 +
>  11 files changed, 230 insertions(+), 313 deletions(-)
>  delete mode 100644
> CryptoPkg/Library/OpensslLib/rand_pool_noise.c
>  delete mode 100644
> CryptoPkg/Library/OpensslLib/rand_pool_noise_tsc.c
>  create mode 100644
> MdePkg/Library/BaseRngLibTimer/RngLibTimer.c
>  delete mode 100644
> CryptoPkg/Library/OpensslLib/rand_pool_noise.h
>  create mode 100644
> MdePkg/Library/BaseRngLibTimer/BaseRngLibTimer.inf
>  create mode 100644
> MdePkg/Library/BaseRngLibTimer/BaseRngLibTimer.uni
> 
> --
> 2.27.0.windows.1
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#63528): https://edk2.groups.io/g/devel/message/63528
Mute This Topic: https://groups.io/mt/75890825/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Hey Mike!

Thanks for the comments. I've addressed the feedback below.

1. All good comments. I think I've addressed them all. New patch series should be coming soon.

2. This is a problem for BaseCryptLibOnProtocol. We can't use a null lib for RngLib since it explodes when it tries to seed OpenSSL with entropy. I think for now, let's stick with established dependencies and figure this out at a later date. I added an RngLib that uses the DXE RNG Protocol.

3. I don't see a typo?

Calls RandomNumber64 to fill
a buffer of arbitrary size with random bytes.

@param [in]   Length        Size of the buffer, in bytes,  to fill with.
@param [out]  RandBuffer    Pointer to the buffer to store the random result.

@retval True        Random bytes generation succeeded.
@retval False       Failed to request random bytes.

--
- Matthew Carlson

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#63591): https://edk2.groups.io/g/devel/message/63591
Mute This Topic: https://groups.io/mt/75890825/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Hi Matt,

I know you can use the Null lib and have the module be function.  The Package DSC is for build verification.  Does not have to be functional.  We already have examples in the Security using a Null BaseCryptLib to improve build verification performance, but of course those security modules as built would not run.

I know one goal is to build functional binaries from the CryptoPkg.  Perhaps that should be a different DSC file?

The typo is True -> TRUE and False -> FALSE

Mike

From: macarl via [] <macarl=microsoft.com@[]>
Sent: Friday, July 31, 2020 1:16 PM
To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io
Subject: Re: [edk2-devel] [Patch v2 0/2] Use RngLib instead of TimerLib for OpensslLib


Hey Mike!

Thanks for the comments. I've addressed the feedback below.

1. All good comments. I think I've addressed them all. New patch series should be coming soon.

2. This is a problem for BaseCryptLibOnProtocol. We can't use a null lib for RngLib since it explodes when it tries to seed OpenSSL with entropy. I think for now, let's stick with established dependencies and figure this out at a later date. I added an RngLib that uses the DXE RNG Protocol.

3. I don't see a typo?

  Calls RandomNumber64 to fill

  a buffer of arbitrary size with random bytes.



  @param[in]   Length        Size of the buffer, in bytes,  to fill with.

  @param[out]  RandBuffer    Pointer to the buffer to store the random result.



  @retval True        Random bytes generation succeeded.

  @retval False       Failed to request random bytes.



--
- Matthew Carlson

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#63597): https://edk2.groups.io/g/devel/message/63597
Mute This Topic: https://groups.io/mt/75890825/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

I included a null lib and I was including the wrong version of Rng anyway.

I think that would make sense to split them into two different DSC files.

Typo fixed, good spot!
--
- Matthew Carlson

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#63601): https://edk2.groups.io/g/devel/message/63601
Mute This Topic: https://groups.io/mt/75890825/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-