From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web10.8630.1597321174168629075 for ; Thu, 13 Aug 2020 05:19:34 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: ard.biesheuvel@arm.com) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id D1F891063; Thu, 13 Aug 2020 05:19:33 -0700 (PDT) Received: from [192.168.178.54] (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id ADB4E3F6CF; Thu, 13 Aug 2020 05:19:32 -0700 (PDT) Subject: Re: [PATCH v6 2/5] MdePkg: BaseRngLibDxe: Add RngLib that uses RngDxe To: matthewfcarlson@gmail.com, devel@edk2.groups.io Cc: Michael D Kinney , Liming Gao , Zhiguang Liu References: <20200812224338.287-1-matthewfcarlson@gmail.com> <20200812224338.287-3-matthewfcarlson@gmail.com> From: "Ard Biesheuvel" Message-ID: <1b47936f-ed13-778e-79ed-707e0ca3bdbc@arm.com> Date: Thu, 13 Aug 2020 14:19:31 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 MIME-Version: 1.0 In-Reply-To: <20200812224338.287-3-matthewfcarlson@gmail.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit On 8/13/20 12:43 AM, matthewfcarlson@gmail.com wrote: > From: Matthew Carlson > > This adds a RngLib that uses the RngProtocol to provide randomness. > This means that the RngLib is meant to be used with DXE_DRIVERS. > > Ref: https://github.com/tianocore/edk2/pull/845 > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1871 > > Cc: Ard Biesheuvel > Cc: Michael D Kinney > Cc: Liming Gao > Cc: Zhiguang Liu > Signed-off-by: Matthew Carlson > --- > MdePkg/Library/BaseRngLibDxe/RngDxeLib.c | 200 ++++++++++++++++++++ > MdePkg/Library/BaseRngLibDxe/BaseRngLibDxe.inf | 38 ++++ > MdePkg/MdePkg.dsc | 4 +- > 3 files changed, 241 insertions(+), 1 deletion(-) > > diff --git a/MdePkg/Library/BaseRngLibDxe/RngDxeLib.c b/MdePkg/Library/BaseRngLibDxe/RngDxeLib.c > new file mode 100644 > index 000000000000..8ee29329de13 > --- /dev/null > +++ b/MdePkg/Library/BaseRngLibDxe/RngDxeLib.c > @@ -0,0 +1,200 @@ > +/** @file > + Provides an implementation of the library class RngLib that uses the Rng protocol. > + > +Copyright (c) Microsoft Corporation. All rights reserved. > +SPDX-License-Identifier: BSD-2-Clause-Patent Please use matching indentation > + > +**/ > +#include > +#include > +#include > +#include > +#include > + > +/** > +Routine Description: > + > + Generates a random number via the NIST > + 800-9A algorithm. Refer to > + http://csrc.nist.gov/groups/STM/cavp/documents/drbg/DRBGVS.pdf > + for more information. > + > + Arguments: > + > + Buffer -- Buffer to receive the random number. > + BufferSize -- Number of bytes in Buffer. > + > +Return Value: > + > + EFI_SUCCESS or underlying failure code. > + > +**/ STATIC ? > +EFI_STATUS > +EFIAPI > +GenerateRandomNumberViaNist800Algorithm( space before ( > + OUT UINT8* Buffer, put * on the rhs > + IN UINTN BufferSize > + ) > +{ > + EFI_STATUS Status; > + EFI_RNG_PROTOCOL* RngProtocol; likewise > + > + RngProtocol = NULL; > + > + if (Buffer == NULL) { > + DEBUG((DEBUG_ERROR, "[%a] Buffer == NULL.\n", __FUNCTION__)); Could you drop the [] around the function name? This is rather unidiomatic for EDK2 > + return EFI_INVALID_PARAMETER; > + } > + > + Status = gBS->LocateProtocol(&gEfiRngProtocolGuid, NULL, (VOID **)&RngProtocol); Space before ( > + if (EFI_ERROR(Status) || RngProtocol == NULL) { Space before (. Also, I think the second condition could be an ASSERT() > + DEBUG((DEBUG_ERROR, "[%a] Could not locate RNG prototocol, Status = %r\n", __FUNCTION__, Status)); > + return Status; > + } > + > + Status = RngProtocol->GetRNG(RngProtocol, &gEfiRngAlgorithmSp80090Ctr256Guid, BufferSize, Buffer); > + DEBUG((DEBUG_INFO, "[%a] GetRNG algorithm CTR-256 - Status = %r\n", __FUNCTION__, Status)); > + if(!EFI_ERROR(Status)) { Space after 'if' and before (. Please do a pass over all the patches, I will stop pointing out the spacing around ( from this point. > + return Status; > + } > + > + Status = RngProtocol->GetRNG(RngProtocol, &gEfiRngAlgorithmSp80090Hmac256Guid, BufferSize, Buffer); > + DEBUG((DEBUG_INFO, "[%a] GetRNG algorithm HMAC-256 - Status = %r\n", __FUNCTION__, Status)); > + if(!EFI_ERROR(Status)) { > + return Status; > + } > + > + Status = RngProtocol->GetRNG(RngProtocol, &gEfiRngAlgorithmSp80090Hash256Guid, BufferSize, Buffer); > + DEBUG((DEBUG_INFO, "[%a] GetRNG algorithm Hash-256 - Status = %r\n", __FUNCTION__, Status)); > + if(!EFI_ERROR(Status)) { > + return Status; > + } I don't like this 'success handling' pattern tbh. Also, why are these algorithms singled out like this? EFI_RNG_PROTOCOL typically has a default algorithm, and even raw entropy is perfectly suitable for key generation (although perhaps slightly wasteful in some case) I am aware there is a check in the RdRand RngDxe that refuses to return 32 bytes from the raw algorithm, but this is simply a misinterpretation of the spec that we should fix at some point, > + // If we get to this point, we have failed > + DEBUG((DEBUG_ERROR, "[%a] GetRNG() failed, staus = %r\n", __FUNCTION__, Status)); > + > + return Status; > +}// GenerateRandomNumberViaNist800Algorithm() > + > + > +/** > + Generates a 16-bit random number. > + > + if Rand is NULL, return FALSE. > + Can't we simply stipulate that Rand != NULL? Is there ever a point to calling this function with a NULL buffer? > + @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 > + ) > +{ > + EFI_STATUS Status; > + > + if (Rand == NULL) > + { > + return FALSE; > + } > + > + Status = GenerateRandomNumberViaNist800Algorithm ((UINT8 *)Rand, 2); > + if (EFI_ERROR(Status)) > + { > + return FALSE; > + } > + return TRUE; > +} > + > +/** > + Generates a 32-bit random number. > + > + if Rand is NULL, return FALSE. > + > + @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 > + ) > +{ > + EFI_STATUS Status; > + > + if (Rand == NULL) { > + return FALSE; > + } > + > + Status = GenerateRandomNumberViaNist800Algorithm ((UINT8 *)Rand, 4); > + if (EFI_ERROR(Status)) { > + return FALSE; > + } > + return TRUE; > +} > + > +/** > + Generates a 64-bit random number. > + > + if Rand is NULL, return FALSE. > + > + @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 > + ) > +{ > + EFI_STATUS Status; > + > + if (Rand == NULL) > + { > + return FALSE; > + } > + > + Status = GenerateRandomNumberViaNist800Algorithm ((UINT8 *)Rand, 8); > + if (EFI_ERROR(Status)) { > + return FALSE; > + } > + return TRUE; > +} > + > +/** > + Generates a 128-bit random number. > + > + if Rand is NULL, return FALSE. > + > + @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 > + ) > +{ > + EFI_STATUS Status; > + > + if (Rand == NULL) { > + return FALSE; > + } > + > + Status = GenerateRandomNumberViaNist800Algorithm ((UINT8 *)Rand, 16); > + if (EFI_ERROR(Status)) { > + return FALSE; > + } > + return TRUE; > +} > diff --git a/MdePkg/Library/BaseRngLibDxe/BaseRngLibDxe.inf b/MdePkg/Library/BaseRngLibDxe/BaseRngLibDxe.inf > new file mode 100644 > index 000000000000..819a106b1376 > --- /dev/null > +++ b/MdePkg/Library/BaseRngLibDxe/BaseRngLibDxe.inf > @@ -0,0 +1,38 @@ > +# @file > +# Provides implementation of the library class RngLib that uses the RngProtocol > +# > +# @copyright > +# Copyright (c) Microsoft Corporation. All rights reserved. > +# SPDX-License-Identifier: BSD-2-Clause-Patent > +# > +# MU_CHANGE: New file Drop this please > +## > + > +[Defines] > + INF_VERSION = 0x00010017 > + BASE_NAME = BaseRngLibDxe > + FILE_GUID = FF9F84C5-A33E-44E3-9BB5-0D654B2D4149 > + MODULE_TYPE = DXE_DRIVER > + VERSION_STRING = 1.0 > + LIBRARY_CLASS = RngLib|DXE_DRIVER UEFI_APPLICATION UEFI_DRIVER > + > +[Packages] > + MdePkg/MdePkg.dec > + > +[Sources] > + RngDxeLib.c > + > +[LibraryClasses] > + DebugLib > + UefiBootServicesTableLib > + > +[Protocols] > + gEfiRngProtocolGuid ## CONSUMES > + > +[Depex] > + gEfiRngProtocolGuid > + > +[Guids] > + gEfiRngAlgorithmSp80090Ctr256Guid ## CONSUMES > + gEfiRngAlgorithmSp80090Hash256Guid ## CONSUMES > + gEfiRngAlgorithmSp80090Hmac256Guid ## CONSUMES I'm not sure these annotations are appropriate for the [Guids] section > diff --git a/MdePkg/MdePkg.dsc b/MdePkg/MdePkg.dsc > index d7ba3a730909..837a0047400e 100644 > --- a/MdePkg/MdePkg.dsc > +++ b/MdePkg/MdePkg.dsc > @@ -62,8 +62,10 @@ > MdePkg/Library/BasePostCodeLibPort80/BasePostCodeLibPort80.inf > MdePkg/Library/BasePrintLib/BasePrintLib.inf > MdePkg/Library/BaseReportStatusCodeLibNull/BaseReportStatusCodeLibNull.inf > - MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf > + MdePkg/Library/BaseRngLibDxe/BaseRngLibDxe.inf > MdePkg/Library/BaseRngLibNull/BaseRngLibNull.inf > + MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf > + > MdePkg/Library/BaseSerialPortLibNull/BaseSerialPortLibNull.inf > MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLib.inf > MdePkg/Library/BaseTimerLibNullTemplate/BaseTimerLibNullTemplate.inf >