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.kinney@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 (1500000 / 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 (1500000, > 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.perard@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 which instance to use > and > > the implications of that instance. > > > > This patch series seeks to address five seperate issues. > > 1) Use RngLib interface to generate random entropy in rand_pool > > 2) Remove dependency on TimerLib in OpensslLib > > 3) Add a new version of RngLib implemented by TimerLib > > 4) Add a new version of RngLib implemented by EFI_RNG_PROTOCOL > > 5) Add RngLib to platforms in EDK2 such as ArmVirtPkg and OvmfPkg > > > > Since this changes the dependencies of OpenSSL, this has the potential > of being > > a breaking change for platforms in edk2-platforms. The easiest solution > is just > > to use the RngLib that uses the TimerLib as this closely mimics the > behavior of > > OpenSSL prior to this patch series. There is also a null version of > RngLib for > > CI environments that need this change > > (https://edk2.groups.io/g/devel/message/50432). Though it should be > pointed out > > that in CI environments, the null version of BaseCryptLib or OpenSSL > should be > > used. > > > > In addition, it has been suggested that > > 1) Add AsmRdSeed to BaseLib. > > 2) Update BaseRngLib to use AsmRdSeed() for the random number, > > if RdSeed is supported (CPUID BIT18) > > > > However, this is largely out of scope for this particular patch series > and > > will likely need to be in a follow-up series later. > > > > It is my understanding that the OpenSSL code uses the values provided as > a > > randomness pool rather than a seed or random numbers itself, so the > > requirements for randomness are not quite as stringent as other > applications. > > > > For the ArmVirtPkg and OvmfPkg platforms, the patch series here just > adds in > > the TimerLib based RngLib as that is similar to the functionality of > before. > > It is added as a common library so any custom RngLib defined in the DSC > > should take precedence over the TimerLibRngLib. > > > > Ref: https://github.com/tianocore/edk2/pull/845 > > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1871 > > > > Cc: Ard Biesheuvel > > Cc: Anthony Perard > > Cc: Jiewen Yao > > Cc: Jian J Wang > > Cc: Julien Grall > > Cc: Jordan Justen > > Cc: Laszlo Ersek > > Cc: Liming Gao > > Cc: Leif Lindholm > > Cc: Michael D Kinney > > Cc: Xiaoyu Lu > > Cc: Zhiguang Liu > > Cc: Sean Brogan > > > > Signed-off-by: Matthew Carlson > > > > > > Matthew Carlson (5): > > MdePkg: TimerRngLib: Added RngLib that uses TimerLib > > MdePkg: BaseRngLibDxe: Add RngLib that uses RngDxe > > OvmfPkg: Add RngLib based on TimerLib for Crypto > > ArmVirtPkg: Add RngLib based on TimerLib for CryptoPkg > > CryptoPkg: OpensslLib: Use RngLib to generate entropy in rand_pool > > > > CryptoPkg/Library/OpensslLib/rand_pool.c | 265 > +++++--------------- > > CryptoPkg/Library/OpensslLib/rand_pool_noise.c | 29 --- > > CryptoPkg/Library/OpensslLib/rand_pool_noise_tsc.c | 43 ---- > > MdePkg/Library/BaseRngLibTimerLib/RngLibTimer.c | 188 > ++++++++++++++ > > MdePkg/Library/DxeRngLib/DxeRngLib.c | 206 > +++++++++++++++ > > ArmVirtPkg/ArmVirt.dsc.inc | 1 + > > CryptoPkg/CryptoPkg.dsc | 1 + > > CryptoPkg/Library/OpensslLib/OpensslLib.inf | 15 +- > > CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf | 15 +- > > CryptoPkg/Library/OpensslLib/rand_pool_noise.h | 29 --- > > MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf | 36 +++ > > MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.uni | 15 ++ > > MdePkg/Library/DxeRngLib/DxeRngLib.inf | 37 +++ > > MdePkg/MdePkg.dsc | 5 +- > > OvmfPkg/Bhyve/BhyvePkgX64.dsc | 1 + > > OvmfPkg/OvmfPkgIa32.dsc | 1 + > > OvmfPkg/OvmfPkgIa32X64.dsc | 1 + > > OvmfPkg/OvmfPkgX64.dsc | 1 + > > OvmfPkg/OvmfXen.dsc | 1 + > > 19 files changed, 555 insertions(+), 335 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/BaseRngLibTimerLib/RngLibTimer.c > > create mode 100644 MdePkg/Library/DxeRngLib/DxeRngLib.c > > delete mode 100644 CryptoPkg/Library/OpensslLib/rand_pool_noise.h > > create mode 100644 > MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf > > create mode 100644 > MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.uni > > create mode 100644 MdePkg/Library/DxeRngLib/DxeRngLib.inf > > > > -- > > 2.27.0.windows.1 > > > > > > > >