Re: [PATCH] SecurityPkg/RngLibNull: add null version of RngLib

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

On 11/12/19 06:55, Jian J Wang wrote:
> This is null version of RngLib which is used for those platforms or
> components which don't need random number.
> 
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1871
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Chao Zhang <chao.b.zhang@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Ray Ni <ray.ni@intel.com>
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> ---
>  .../RngLibNull/RngLibNull.c                   | 95 +++++++++++++++++++
>  .../RngLibNull/RngLibNull.inf                 | 31 ++++++
>  .../RngLibNull/RngLibNull.uni                 | 14 +++
>  3 files changed, 140 insertions(+)
>  create mode 100644 SecurityPkg/RandomNumberGenerator/RngLibNull/RngLibNull.c
>  create mode 100644 SecurityPkg/RandomNumberGenerator/RngLibNull/RngLibNull.inf
>  create mode 100644 SecurityPkg/RandomNumberGenerator/RngLibNull/RngLibNull.uni

(1) I don't see any reason why this library instance should not be added
under MdePkg/Library. The other library instance is already there (and
the lib class header too is from MdePkg):

  MdePkg/Library/BaseRngLib

(2) I think this library instance should be called "BaseRngLibNull", not
just "RngLibNull".

More below:

> diff --git a/SecurityPkg/RandomNumberGenerator/RngLibNull/RngLibNull.c b/SecurityPkg/RandomNumberGenerator/RngLibNull/RngLibNull.c
> new file mode 100644
> index 0000000000..13677abc84
> --- /dev/null
> +++ b/SecurityPkg/RandomNumberGenerator/RngLibNull/RngLibNull.c
> @@ -0,0 +1,95 @@
> +/** @file
> +  Null version of Random number generator services.
> +
> +Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
> +SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include <Library/BaseLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/RngLib.h>
> +
> +/**
> +  Generates a 16-bit random number.
> +
> +  if Rand is NULL, then ASSERT().
> +
> +  @param[out] Rand     Buffer pointer to store the 16-bit random value.
> +
> +  @retval TRUE         Random number generated successfully.
> +  @retval FALSE        Failed to generate the random number.
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +GetRandomNumber16 (
> +  OUT     UINT16                    *Rand
> +  )
> +{
> +  ASSERT (FALSE);
> +  return FALSE;
> +}
> +
> +/**
> +  Generates a 32-bit random number.
> +
> +  if Rand is NULL, then ASSERT().
> +
> +  @param[out] Rand     Buffer pointer to store the 32-bit random value.
> +
> +  @retval TRUE         Random number generated successfully.
> +  @retval FALSE        Failed to generate the random number.
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +GetRandomNumber32 (
> +  OUT     UINT32                    *Rand
> +  )
> +{
> +  ASSERT (FALSE);
> +  return FALSE;
> +}
> +
> +/**
> +  Generates a 64-bit random number.
> +
> +  if Rand is NULL, then ASSERT().
> +
> +  @param[out] Rand     Buffer pointer to store the 64-bit random value.
> +
> +  @retval TRUE         Random number generated successfully.
> +  @retval FALSE        Failed to generate the random number.
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +GetRandomNumber64 (
> +  OUT     UINT64                    *Rand
> +  )
> +{
> +  ASSERT (FALSE);
> +  return FALSE;
> +}
> +
> +/**
> +  Generates a 128-bit random number.
> +
> +  if Rand is NULL, then ASSERT().
> +
> +  @param[out] Rand     Buffer pointer to store the 128-bit random value.
> +
> +  @retval TRUE         Random number generated successfully.
> +  @retval FALSE        Failed to generate the random number.
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +GetRandomNumber128 (
> +  OUT     UINT64                    *Rand
> +  )
> +{
> +  ASSERT (FALSE);
> +  return FALSE;
> +}
> diff --git a/SecurityPkg/RandomNumberGenerator/RngLibNull/RngLibNull.inf b/SecurityPkg/RandomNumberGenerator/RngLibNull/RngLibNull.inf
> new file mode 100644
> index 0000000000..f6494cdb82
> --- /dev/null
> +++ b/SecurityPkg/RandomNumberGenerator/RngLibNull/RngLibNull.inf
> @@ -0,0 +1,31 @@
> +## @file
> +#  Null instance of RNG (Random Number Generator) Library.
> +#
> +#  Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
> +#
> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +##
> +
> +[Defines]
> +  INF_VERSION                    = 0x00010005
> +  BASE_NAME                      = RngLibNull
> +  MODULE_UNI_FILE                = RngLibNull.uni
> +  FILE_GUID                      = CD8991F8-2061-4084-8C9E-9C6F352DC58D
> +  MODULE_TYPE                    = BASE
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = RngLib
> +
> +#
> +#  VALID_ARCHITECTURES           = IA32 X64 ARM AARCH64
> +#
> +
> +[Sources]
> +  RngLibNull.c
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +
> +[LibraryClasses]
> +  BaseLib
> +  DebugLib
> diff --git a/SecurityPkg/RandomNumberGenerator/RngLibNull/RngLibNull.uni b/SecurityPkg/RandomNumberGenerator/RngLibNull/RngLibNull.uni
> new file mode 100644
> index 0000000000..40b2ec3fe1
> --- /dev/null
> +++ b/SecurityPkg/RandomNumberGenerator/RngLibNull/RngLibNull.uni
> @@ -0,0 +1,14 @@
> +// /** @file
> +// Null Instance of RNG (Random Number Generator) Library.
> +//
> +// Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
> +//
> +// SPDX-License-Identifier: BSD-2-Clause-Patent
> +//
> +// **/
> +
> +
> +#string STR_MODULE_ABSTRACT             #language en-US "Null Instance of RNG Library"
> +
> +#string STR_MODULE_DESCRIPTION          #language en-US "Caution: This is a null version of RNG library and SHOULD NOT be used on any product ever."
> +
> 

(3) I disagree with STR_MODULE_DESCRIPTION.

This library instance is appropriate even in production, namely for such
modules that *inherit* a dependency on RngLib -- for example, through
another library instance --, but, in practice, they never consume
randomness, and/or they never *must* consume randomness.

In other words, this library instance should be used with modules that
should, in practice, never *reach* any calls to GetRandomNumberXX()
APIs, but it is difficult to remove the call sites themselves -- for
example, because they are inherited (i.e., indirectly) through another
library class.

With that in mind, the ASSERT()s seem justified -- these functions
should never be reached.

Note: I'm not saying that the ASSERT()s are *required*. Luckily, all
these APIs are able to report failure, and so if all client code checks
the return values, no actual functionality will be misled. (The
functions in this lib instance all return FALSE, correctly.) But, the
ASSERT()s are good for pointing out the larger issue: if a module
actually calls these functions (because it needs actual randomness),
then the module / platform configuration (= DSC file) is broken.

In summary, STR_MODULE_DESCRIPTION should state, "this library instance
should be used with modules that inherit an (indirect) dependency on the
RngLib class, but never actually call RngLib APIs for consuming randomness".

Thanks,
Laszlo