Re: [edk2-devel] [PATCH 08/11] OvmfPkg: specify RngLib instances in dsc files

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

Jian, Ard,

commenting for both OvmfPkg and ArmVirtPkg:

On 11/14/19 03:17, Wang, Jian J wrote:
> Per BZ1871, OpensslLib will depend on RngLib instead of TimerLib.
> Update OVMF dsc files to accommodate the coming changes. It's supposed
> that only TlsDxe needs random number. The DxeRngLibRngProtocol is added
> for it. For all other drivers, BaseRngLibNull is used by default.
> 
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1871
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> ---
>  OvmfPkg/OvmfPkgIa32.dsc    | 5 +++++
>  OvmfPkg/OvmfPkgIa32X64.dsc | 5 +++++
>  OvmfPkg/OvmfPkgX64.dsc     | 5 +++++
>  OvmfPkg/OvmfXen.dsc        | 5 +++++
>  4 files changed, 20 insertions(+)
> 
> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index d350b75630..5a709a95b2 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -217,6 +217,7 @@
>  
>  [LibraryClasses.common]
>    BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> +  RngLib|MdePkg/Library/BaseRngLibNull/BaseRngLibNull.inf
>  
>  [LibraryClasses.common.SEC]
>    TimerLib|OvmfPkg/Library/AcpiTimerLib/BaseRomAcpiTimerLib.inf
> @@ -786,6 +787,10 @@
>      <LibraryClasses>
>        NULL|OvmfPkg/Library/TlsAuthConfigLib/TlsAuthConfigLib.inf
>    }
> +  NetworkPkg/TlsDxe/TlsDxe.inf {
> +    <LibraryClasses>
> +      RngLib|SecurityPkg/RandomNumberGenerator/DxeRngLibRngProtocol/DxeRngLibRngProtocol.inf
> +  }
>  !endif
>    OvmfPkg/VirtioNetDxe/VirtioNet.inf
>  

This is not right for either OvmfPkg or ArmVirtPkg, because:

- the virtual hardware is dynamic and might change from boot to boot,

- EFI_RNG_PROTOCOL, implemented by VirtioRngDxe, only supports
EFI_RNG_ALGORITHM_RAW.

I propose the following approach.

(1) Jian, please do introduce all of the RngLib instances that you are
introducing in this v1 series, namely:

- the null lib instance,
- the RDSEED lib instance,
- the DXE / protocol lib instance that directly requires NIST/ANSI
conformant algorithm support from the RNG protocol.

(2) Jian, please add a *further* lib instance: a time based one. This
instance should basically extract the current functionality from
"rand_pool_noise.c" and "rand_pool_noise_tsc.c". (The code that you are
removing in patch #10.)

It's also fine with me if, rather than factoring out the
rand_pool_noise*.c logic, you add a new "CPU jitter" based RngLib
instance. (Ard might disagree with me about this alternative -- the
point is that we should have a library instance that provides *some*
(even if barely tolerable) randomness, but with no dependency on
*optional* hardware. In other words, this lib instance should depend on
*guaranteed* platform hardware only. TSC, TimerLib, etc.)

(3) For OvmfPkg and ArmVirtPkg, please write patches that:

- provide a default resolution to the Null instance,

- resolve RngLib to the time-based instance, for TlsDxe *only*.

At that point, we will have made ArmVirtPkg and OvmfPkg *independent* of
the rest of this series -- ArmVirtPkg and OvmfPkg will not block this
patch series, they will also not suffer any regressions, and we can go
ahead and implement a separate RngLib instance for TlsDxe ourselves.


Now, let me lay out my proposal for *that* RngLib instance. (I'm willing
to write it a good chunk of it, but I will need help from Ard minimally
in the crypto code.)

- The CONSTRUCTOR function should register an End-of-Dxe notification
function. In that function, (a) a boolean flag (such as "mEndOfDxe")
should be flipped from FALSE to TRUE, and (b) a pointer to
EFI_RNG_PROTOCOL should be saved (if such a protocol exists), such as
"mRngProtocol".

- each RngLib API in this lib instance should hang, if "mEndOfDxe" is
FALSE at the time of the call (--> CpuDeadLoop() etc)

- otherwise, if "mRngProtocol" is not NULL, it should be used to fetch
*raw* entropy. That raw entropy should be mixed sufficiently for NIST /
ANSI conformance. (I hope this statement makes sense.) This is where I'd
absolutely need help from Ard.

- otherwise, on IA32/X64 only, if CPUID indicates that RDSEED is
supported by the processor, call RDSEED.

- otherwise, on IA32/X64 only, if CPUID indicates that RDRAND is
supported by the processor, call RDRAND.

- otherwise, on all arches, scrape some timer-based entropy, "from the
bottom of the barrel" (possibly in an arch-specific way, e.g. we could
use TSC on IA32/X64).

- The platform BDS code for OVMF and ArmVirtQemu* should bind the
virtio-rng device to VirtioRngDxe *before* signaling End-of-Dxe.


The basic idea is that this RngLib instance should give the platform a
*chance* to produce an EFI_RNG_PROTOCOL instance until End-of-Dxe. If
that happens, then continue consuming raw entropy from *that* source,
and mix it manually for NIST / ANSI conformance. Otherwise, gracefully
degrade to the next strongest entropy source that's dynamically detected
on the platform (RDSEED or RDRAND, per CPUID -- on IA32/X64 only --, and
then timer-based -- on all platforms).

Thanks
Laszlo