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

["Wang, Jian J" <jian.j.wang@intel.com>]

Laszlo,


> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
> Sent: Tuesday, November 12, 2019 3:50 PM
> To: Wang, Jian J <jian.j.wang@intel.com>; devel@edk2.groups.io
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Zhang, Chao B
> <chao.b.zhang@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>;
> Gao, Liming <liming.gao@intel.com>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>; Ni, Ray <ray.ni@intel.com>
> Subject: Re: [edk2-devel] [PATCH] SecurityPkg/RngLibNull: add null version of
> RngLib
> 
> 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".
> 

I have no strong opinion on this.

Liming, do you have any comments?

> 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".
> 

Good explanation. Thanks. And I agree with your version of STR_MODULE_DESCRIPTION.
I'll update it in v2.

Regards,
Jian

> Thanks,
> Laszlo
> 
> 
>