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 <mac...@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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
