From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.61]) by mx.groups.io with SMTP id smtpd.web09.8650.1573545021665095790 for ; Mon, 11 Nov 2019 23:50:21 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=C1NnWrrc; spf=pass (domain: redhat.com, ip: 205.139.110.61, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1573545020; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=c8xrdc6j9+rSRnMHuHy1UfAj4W9mb7RogfJ3NLcSO5Y=; b=C1NnWrrc0dXP7Vg+yFcKwb4VOlPsacyVtTNLWt7HkUqGl/U4UMcMc9xd4Bq2bfjWe46Et9 vwAF7vtUD2lqq3NLLBy78ird8C2pRzPAcF3hVm40Z/yQByHtDApbc2D2rkVx8VEx7eH4yR z6aZMBevj8e7inkbhLZ6KHNTnZdA0xo= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-276-P0okPPsWN5GkWRyUZCAjFg-1; Tue, 12 Nov 2019 02:50:17 -0500 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id F253A1005500; Tue, 12 Nov 2019 07:50:15 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-207.ams2.redhat.com [10.36.116.207]) by smtp.corp.redhat.com (Postfix) with ESMTP id E87286266F; Tue, 12 Nov 2019 07:50:13 +0000 (UTC) Subject: Re: [PATCH] SecurityPkg/RngLibNull: add null version of RngLib To: Jian J Wang , devel@edk2.groups.io Cc: Jiewen Yao , Chao Zhang , Michael D Kinney , Liming Gao , Ard Biesheuvel , Ray Ni References: <20191112055545.3948-1-jian.j.wang@intel.com> From: "Laszlo Ersek" Message-ID: <77a7c143-8547-2bc6-4a87-d0afbf2529e5@redhat.com> Date: Tue, 12 Nov 2019 08:50:12 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20191112055545.3948-1-jian.j.wang@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-MC-Unique: P0okPPsWN5GkWRyUZCAjFg-1 X-Mimecast-Spam-Score: 0 Content-Language: en-US Content-Type: text/plain; charset=WINDOWS-1252 Content-Transfer-Encoding: quoted-printable 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. >=20 > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3D1871 > Cc: Jiewen Yao > Cc: Chao Zhang > Cc: Michael D Kinney > Cc: Liming Gao > Cc: Laszlo Ersek > Cc: Ard Biesheuvel > Cc: Ray Ni > Signed-off-by: Jian J Wang > --- > .../RngLibNull/RngLibNull.c | 95 +++++++++++++++++++ > .../RngLibNull/RngLibNull.inf | 31 ++++++ > .../RngLibNull/RngLibNull.uni | 14 +++ > 3 files changed, 140 insertions(+) > create mode 100644 SecurityPkg/RandomNumberGenerator/RngLibNull/RngLibNu= ll.c > create mode 100644 SecurityPkg/RandomNumberGenerator/RngLibNull/RngLibNu= ll.inf > create mode 100644 SecurityPkg/RandomNumberGenerator/RngLibNull/RngLibNu= ll.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.
> +SPDX-License-Identifier: BSD-2-Clause-Patent > + > +**/ > + > +#include > +#include > +#include > + > +/** > + 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.
> +# > +# SPDX-License-Identifier: BSD-2-Clause-Patent > +# > +## > + > +[Defines] > + INF_VERSION =3D 0x00010005 > + BASE_NAME =3D RngLibNull > + MODULE_UNI_FILE =3D RngLibNull.uni > + FILE_GUID =3D CD8991F8-2061-4084-8C9E-9C6F352DC58= D > + MODULE_TYPE =3D BASE > + VERSION_STRING =3D 1.0 > + LIBRARY_CLASS =3D RngLib > + > +# > +# VALID_ARCHITECTURES =3D 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.
> +// > +// SPDX-License-Identifier: BSD-2-Clause-Patent > +// > +// **/ > + > + > +#string STR_MODULE_ABSTRACT #language en-US "Null Instance o= f RNG Library" > + > +#string STR_MODULE_DESCRIPTION #language en-US "Caution: This i= s a null version of RNG library and SHOULD NOT be used on any product ever.= " > + >=20 (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 (=3D 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