From: "Matthew Carlson" <matthewfcarlson@gmail.com>
To: "Kinney, Michael D" <michael.d.kinney@intel.com>
Cc: "devel@edk2.groups.io" <devel@edk2.groups.io>,
Ard Biesheuvel <ard.biesheuvel@arm.com>,
Anthony Perard <anthony.perard@citrix.com>,
"Yao, Jiewen" <jiewen.yao@intel.com>,
"Wang, Jian J" <jian.j.wang@intel.com>,
Julien Grall <julien@xen.org>,
"Justen, Jordan L" <jordan.l.justen@intel.com>,
Laszlo Ersek <lersek@redhat.com>,
"Gao, Liming" <liming.gao@intel.com>,
Leif Lindholm <leif@nuviainc.com>,
"Lu, XiaoyuX" <xiaoyux.lu@intel.com>,
"Liu, Zhiguang" <zhiguang.liu@intel.com>,
Sean Brogan <sean.brogan@microsoft.com>
Subject: Re: [edk2-devel] [PATCH v7 0/5] Use RngLib instead of TimerLib for OpensslLib
Date: Fri, 14 Aug 2020 14:01:50 -0700 [thread overview]
Message-ID: <CALSfKn=GKQYh==LXsPKYddVxU3V7q8YvAqSCzGmw4dTZB8a1Tg@mail.gmail.com> (raw)
In-Reply-To: <DM6PR11MB44588E2D63FAEFD575348D94D2400@DM6PR11MB4458.namprd11.prod.outlook.com>
[-- Attachment #1: Type: text/plain, Size: 10438 bytes --]
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 <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 <ard.biesheuvel@arm.com>; Anthony Perard <
> anthony.perard@citrix.com>; Yao, Jiewen
> > <jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Julien
> Grall <julien@xen.org>; Justen, Jordan L
> > <jordan.l.justen@intel.com>; Laszlo Ersek <lersek@redhat.com>; Gao,
> Liming <liming.gao@intel.com>; Leif Lindholm
> > <leif@nuviainc.com>; Kinney, Michael D <michael.d.kinney@intel.com>;
> Lu, XiaoyuX <xiaoyux.lu@intel.com>; Liu, Zhiguang
> > <zhiguang.liu@intel.com>; Sean Brogan <sean.brogan@microsoft.com>;
> Matthew Carlson <matthewfcarlson@gmail.com>
> > Subject: [edk2-devel] [PATCH v7 0/5] Use RngLib instead of TimerLib for
> OpensslLib
> >
> > From: Matthew Carlson <macarl@microsoft.com>
> >
> > 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 <ard.biesheuvel@arm.com>
> > Cc: Anthony Perard <anthony.perard@citrix.com>
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > Cc: Jian J Wang <jian.j.wang@intel.com>
> > Cc: Julien Grall <julien@xen.org>
> > Cc: Jordan Justen <jordan.l.justen@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Liming Gao <liming.gao@intel.com>
> > Cc: Leif Lindholm <leif@nuviainc.com>
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > Cc: Xiaoyu Lu <xiaoyux.lu@intel.com>
> > Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> > Cc: Sean Brogan <sean.brogan@microsoft.com>
> >
> > Signed-off-by: Matthew Carlson <matthewfcarlson@gmail.com>
> >
> >
> > 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
> >
> >
> >
>
>
[-- Attachment #2: Type: text/html, Size: 14565 bytes --]
prev parent reply other threads:[~2020-08-14 21:02 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-08-13 19:44 [PATCH v7 0/5] Use RngLib instead of TimerLib for OpensslLib Matthew Carlson
2020-08-13 19:44 ` [PATCH v7 1/5] MdePkg: TimerRngLib: Added RngLib that uses TimerLib Matthew Carlson
2020-08-13 19:44 ` [PATCH v7 2/5] MdePkg: BaseRngLibDxe: Add RngLib that uses RngDxe Matthew Carlson
2020-08-13 19:44 ` [PATCH v7 3/5] OvmfPkg: Add RngLib based on TimerLib for Crypto Matthew Carlson
2020-08-13 19:44 ` [PATCH v7 4/5] ArmVirtPkg: Add RngLib based on TimerLib for CryptoPkg Matthew Carlson
2020-08-13 19:44 ` [PATCH v7 5/5] CryptoPkg: OpensslLib: Use RngLib to generate entropy in rand_pool Matthew Carlson
2020-08-13 23:26 ` [edk2-devel] " Yao, Jiewen
2020-08-14 1:12 ` [edk2-devel] [PATCH v7 0/5] Use RngLib instead of TimerLib for OpensslLib Michael D Kinney
2020-08-14 21:01 ` Matthew Carlson [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CALSfKn=GKQYh==LXsPKYddVxU3V7q8YvAqSCzGmw4dTZB8a1Tg@mail.gmail.com' \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox