From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from EUR04-DB3-obe.outbound.protection.outlook.com (EUR04-DB3-obe.outbound.protection.outlook.com [40.107.6.79]) by mx.groups.io with SMTP id smtpd.web10.5168.1688034521734482381 for ; Thu, 29 Jun 2023 03:28:42 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@armh.onmicrosoft.com header.s=selector2-armh-onmicrosoft-com header.b=BPKz3Id0; spf=pass (domain: arm.com, ip: 40.107.6.79, mailfrom: sami.mujawar@arm.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=armh.onmicrosoft.com; s=selector2-armh-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=Z8eQU3/7YYM0EiBnFjqLo+vXhhny9Oo85u7VH8GSGV0=; b=BPKz3Id0hBkeJvQZMza1aNya5MwPYhtPJ65trP5YLgcBzOceit2IEaBTbMtRVqDyYBH4vMVJfq5f+LSGcwEO2i+nebYYzjLl/E8zyjBYo9aE8ShRq8vNL50IQ+L7o8qXhRY9qTMikDBdf0YGrdYY33juAQDnG6SWs8wAEUColgc= Received: from AS8P189CA0038.EURP189.PROD.OUTLOOK.COM (2603:10a6:20b:458::13) by GVXPR08MB7680.eurprd08.prod.outlook.com (2603:10a6:150:6e::9) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.6500.35; Thu, 29 Jun 2023 10:28:34 +0000 Received: from AM7EUR03FT046.eop-EUR03.prod.protection.outlook.com (2603:10a6:20b:458:cafe::39) by AS8P189CA0038.outlook.office365.com (2603:10a6:20b:458::13) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.6544.20 via Frontend Transport; Thu, 29 Jun 2023 10:28:34 +0000 X-MS-Exchange-Authentication-Results: spf=pass (sender IP is 63.35.35.123) smtp.mailfrom=arm.com; dkim=pass (signature was verified) header.d=armh.onmicrosoft.com;dmarc=pass action=none header.from=arm.com; Received-SPF: Pass (protection.outlook.com: domain of arm.com designates 63.35.35.123 as permitted sender) receiver=protection.outlook.com; client-ip=63.35.35.123; helo=64aa7808-outbound-1.mta.getcheckrecipient.com; pr=C Received: from 64aa7808-outbound-1.mta.getcheckrecipient.com (63.35.35.123) by AM7EUR03FT046.mail.protection.outlook.com (100.127.140.78) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.6544.22 via Frontend Transport; Thu, 29 Jun 2023 10:28:34 +0000 Received: ("Tessian outbound d6c4ee3ba1eb:v142"); Thu, 29 Jun 2023 10:28:34 +0000 X-CheckRecipientChecked: true X-CR-MTA-CID: f101c5daa925be86 X-CR-MTA-TID: 64aa7808 Received: from 8b900378f1d7.1 by 64aa7808-outbound-1.mta.getcheckrecipient.com id EC625F94-EF02-4CF3-9286-D971BE3C7B19.1; Thu, 29 Jun 2023 10:28:27 +0000 Received: from EUR05-DB8-obe.outbound.protection.outlook.com by 64aa7808-outbound-1.mta.getcheckrecipient.com with ESMTPS id 8b900378f1d7.1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384); Thu, 29 Jun 2023 10:28:27 +0000 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=a9adGLFb36ONLm+vwiUCbw6HEQ+3NHMYmDkAoLvz54Lln+1/CbVn2WWyu7ph968TPvekW7zH1KvPNpkqV19CVVWTYxS/EbntQEWa6VXQHcoBwqkb1Wtm74MiWf9pOUdhST1E0XbCqJVFsk1HbaSAHyVzUFIkPlpbyyD5gs50IC8kFwgxRvFN0KAprNQw4mPCB0ND7aSJxxvvpoS45kkUkz71EaPdurZ+iIriEy+2VJvtK7flmu6Dqn4JCtyGk3x9f31LKoEpy0/80XPbiFTNl/eBfavBhGmhOLS0uIjemjeHZQfBcwI/PBfexwTE54B0fUQnzKtkC9zWNCH+SZbrGg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=Z8eQU3/7YYM0EiBnFjqLo+vXhhny9Oo85u7VH8GSGV0=; b=nOuQwZYJ/hz7MAtxR9YBmok0Ug8wtvfraf7kTS/r/CXMWbyRft87zOUvCYQuClhQ2lmX+Fr7Nij4lIy/NKXcDhbviCgnwQ3/uGCGVoyZs8yZkPgI65gfnKf1HLRA2QuXkWfr8UGsMhQ4+06BV01+Qutpru9KGZd6+4gOi3csYhrRsBPNS+EL9zYHdUFWGcWjO6e4u4hdCKuORQ9x/zq/tXcG+pYd+EZ/g1A8anXAP3FuvuKK0q6asvUtv1IfySpcEVHb8vSxjia1P/EjyV9Er6pfnqjbU99IAF9H0R5cqeqPIvLTl2bnMaTVUvsiru6lxsabpLl7lyjMGEKC4AQUxw== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=arm.com; dmarc=pass action=none header.from=arm.com; dkim=pass header.d=arm.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=armh.onmicrosoft.com; s=selector2-armh-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=Z8eQU3/7YYM0EiBnFjqLo+vXhhny9Oo85u7VH8GSGV0=; b=BPKz3Id0hBkeJvQZMza1aNya5MwPYhtPJ65trP5YLgcBzOceit2IEaBTbMtRVqDyYBH4vMVJfq5f+LSGcwEO2i+nebYYzjLl/E8zyjBYo9aE8ShRq8vNL50IQ+L7o8qXhRY9qTMikDBdf0YGrdYY33juAQDnG6SWs8wAEUColgc= Authentication-Results-Original: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=arm.com; Received: from AS8PR08MB6806.eurprd08.prod.outlook.com (2603:10a6:20b:39b::12) by DU0PR08MB9050.eurprd08.prod.outlook.com (2603:10a6:10:47a::17) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.6521.26; Thu, 29 Jun 2023 10:28:25 +0000 Received: from AS8PR08MB6806.eurprd08.prod.outlook.com ([fe80::8ef4:aa57:6248:7850]) by AS8PR08MB6806.eurprd08.prod.outlook.com ([fe80::8ef4:aa57:6248:7850%4]) with mapi id 15.20.6544.019; Thu, 29 Jun 2023 10:28:25 +0000 Message-ID: <89f19363-1b7b-b576-865b-9910e6e2d18d@arm.com> Date: Thu, 29 Jun 2023 11:28:24 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.12.0 Subject: Re: [PATCH v1 6/8] SecurityPkg/RngDxe: Use GetRngGuid() when probing RngLib To: pierre.gondois@arm.com, devel@edk2.groups.io Cc: Michael D Kinney , Liming Gao , Zhiguang Liu , Jiewen Yao , Jian J Wang , Ard Biesheuvel , Jose Marinho , Samer El-Haj-Mahmoud , "nd@arm.com" References: <20230509074042.1523428-1-pierre.gondois@arm.com> <20230509074042.1523428-7-pierre.gondois@arm.com> From: "Sami Mujawar" In-Reply-To: <20230509074042.1523428-7-pierre.gondois@arm.com> X-ClientProxiedBy: LNXP265CA0093.GBRP265.PROD.OUTLOOK.COM (2603:10a6:600:76::33) To AS8PR08MB6806.eurprd08.prod.outlook.com (2603:10a6:20b:39b::12) MIME-Version: 1.0 X-MS-TrafficTypeDiagnostic: AS8PR08MB6806:EE_|DU0PR08MB9050:EE_|AM7EUR03FT046:EE_|GVXPR08MB7680:EE_ X-MS-Office365-Filtering-Correlation-Id: 19aeae30-c3d9-4f4b-6cfe-08db788b97b1 x-checkrecipientrouted: true NoDisclaimer: true X-MS-Exchange-SenderADCheck: 1 X-MS-Exchange-AntiSpam-Relay: 0 X-Microsoft-Antispam-Untrusted: BCL:0; X-Microsoft-Antispam-Message-Info-Original: 4lit0HJB1VECbvSed8cKorZKX4enTq47qmU/XNq9eaQX7j2tDxgjC+tr3qCEPrNHoCDHL99sYKY4IxHqcmrd5+Jz7eJ3jKOEHN+x36ass0Sd+ePM6hfR7JigiTYT2SmcSxhXSROm0bFWfVQMyfXXBWwCMZCMgYd29V8t6LWAr9pk1+1kyOfZmeeVnn6W7GJgH64QW+cmiCwL/PkWiRWNQwq7Xf4LYaw+0mPX2qJjiu8f/KnTQFUWdl+xiXtSfaLWwvMFaJVB7ESJCvvQ8QrcGgpAaUBxlVNjNJqO8q4eqN/EyKEUe95aLZAzGIX1LVGiWzaNzFKYOnSmHRgA34ZnqzyzuoeDqa6dcsU8RNnyhEeUkyCNz8yxmsOPF3XtV7veqvv0pNSvuLzYN+t4TyKGfaSQDHFnjBuNreyotbUTkxmKI1iENl8Q4RvUe2x26nNkuFfTDSDklhgvWV9T28UoBQ3aNbFiAik8OrAP5mYBA+6TnNpwuGOCJDvIfN0B/tXBwtGOhHht+VdirD8s9lrbLs+psOdaKMDThHfPP1p6jJ5F3K93RIjPezNkuvcY6nSMRUEbjgvVuM41nUwYcaz0GpK8DX9umHsPjyY13tOAGJJGJE584ZHeh3AN1X7zU9AVWsjV7Yo8jWElNcBhDfakQA== X-Forefront-Antispam-Report-Untrusted: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:AS8PR08MB6806.eurprd08.prod.outlook.com;PTR:;CAT:NONE;SFS:(13230028)(4636009)(366004)(396003)(346002)(376002)(136003)(39860400002)(451199021)(26005)(54906003)(2906002)(30864003)(33964004)(6486002)(186003)(2616005)(38100700002)(83380400001)(53546011)(6506007)(86362001)(966005)(19627235002)(31696002)(41300700001)(478600001)(66556008)(66476007)(316002)(4326008)(36756003)(8936002)(66946007)(6512007)(31686004)(44832011)(5660300002)(8676002)(15650500001)(166002)(43740500002)(45980500001);DIR:OUT;SFP:1101; X-MS-Exchange-Transport-CrossTenantHeadersStamped: DU0PR08MB9050 Original-Authentication-Results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=arm.com; Return-Path: Sami.Mujawar@arm.com X-EOPAttributedMessage: 0 X-MS-Exchange-Transport-CrossTenantHeadersStripped: AM7EUR03FT046.eop-EUR03.prod.protection.outlook.com X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id-Prvs: 600a120a-e194-43d1-5858-08db788b9239 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: fJWhsodcMu3QhQqluotmuNsE9qKVMyeh16KQUNxSq8Efi8TJvai4wDvnqfsAPaQR2UKasXpmEEzYzS3Gxc2APfsEFUI3uL6PScleoJloS1j96DcwT5rrXdvRgAjYD+9eFZEj5gjs0r+XkW49OEU8DRhVmNqBrqUZThvXeO5CYREKAxJ0GEi/fyklcLN7pce+i8kAkb+BYHF3PUGWKTE8Rp4up49Gj6NlH7VsKqrefQ4gsRlnMC85m35S9XEoIeng64BynOmpJ9gW6jpvCgvjb2ltNycPoIpuG2wU88HOq+0/2YTkAVGlUkJAo+YIKAo6pSVowDifw2DLczVYzPNTtU5pPFtH+orrpqqVIO2HEXoCSBJ339kiVd6dkw9KOh+z89R6SuXICWlGgU+jkFVsigSMsdzWNbqNWmVCIHl0tcCGVVQXmi5HJY4MY3CH2vTuTCS7ATCGVmsBk4e4QoJ8eg3Hc0KYrUkidIwtX4ybqJV7mIrCjCE1OHC31v2a05paJyREVhAb85xavGfKpuLXX/nnAOUMcxHajV0HsgsHTrSr6ZE91nD285GMlberXm000Sdb//rY/EC5qFVBBhs6rHn025rDARn674ZGxBTmAGRiVMNn+aEqveFwecLTmY6mhk7nKnzo49iETA1wip+h4pGmbRjgK59mpQJ1P53d+2N5/nKTEvu6OakA/PN1FRmMXzdAPvJn8u1PsA7ozuMcs6L+0kKMHsiqyXvnPU/w19/W4Gwddm65j2vQVHBODQ96 X-Forefront-Antispam-Report: CIP:63.35.35.123;CTRY:IE;LANG:en;SCL:1;SRV:;IPV:CAL;SFV:NSPM;H:64aa7808-outbound-1.mta.getcheckrecipient.com;PTR:ec2-63-35-35-123.eu-west-1.compute.amazonaws.com;CAT:NONE;SFS:(13230028)(4636009)(136003)(396003)(376002)(346002)(39860400002)(451199021)(46966006)(40470700004)(36840700001)(6512007)(82310400005)(36860700001)(19627235002)(47076005)(966005)(6486002)(478600001)(54906003)(186003)(336012)(26005)(2616005)(83380400001)(53546011)(2906002)(6506007)(15650500001)(33964004)(44832011)(86362001)(41300700001)(70206006)(36756003)(356005)(30864003)(5660300002)(31696002)(4326008)(82740400003)(81166007)(316002)(70586007)(40460700003)(8936002)(166002)(8676002)(40480700001)(31686004)(43740500002);DIR:OUT;SFP:1101; X-OriginatorOrg: arm.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Jun 2023 10:28:34.3050 (UTC) X-MS-Exchange-CrossTenant-Network-Message-Id: 19aeae30-c3d9-4f4b-6cfe-08db788b97b1 X-MS-Exchange-CrossTenant-Id: f34e5979-57d9-4aaa-ad4d-b122a662184d X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=f34e5979-57d9-4aaa-ad4d-b122a662184d;Ip=[63.35.35.123];Helo=[64aa7808-outbound-1.mta.getcheckrecipient.com] X-MS-Exchange-CrossTenant-AuthSource: AM7EUR03FT046.eop-EUR03.prod.protection.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Anonymous X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem X-MS-Exchange-Transport-CrossTenantHeadersStamped: GVXPR08MB7680 Content-Type: multipart/alternative; boundary="------------glBP6q4tBcKx57zzf8ddxrrm" --------------glBP6q4tBcKx57zzf8ddxrrm Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Hi Pierre, Thank you for this patch. I think we should treat algorithms that have a zero GUID as unsafe. I have made some suggestions on those lines marked inline as [SAMI]. Regards, Sami Mujawar On 09/05/2023 08:40 am, pierre.gondois@arm.com wrote: > From: Pierre Gondois > > BZ:https://bugzilla.tianocore.org/show_bug.cgi?id=4151 > > The EFI_RNG_PROTOCOL can rely on the RngLib. The RngLib has multiple > implementations, some of them are unsafe (e.g. BaseRngLibTimerLib). > To allow the RngDxe to detect when such implementation is used, > a GetRngGuid() function was added in a previous patch. > > The EFI_RNG_PROTOCOL can advertise multiple algorithms through > Guids. The PcdCpuRngSupportedAlgorithm is currently used to > advertise the RngLib in the Arm implementation. > > The issues of doing that are: > - the RngLib implementation might not use CPU instructions, > cf. the BaseRngLibTimerLib > - most platforms don't set PcdCpuRngSupportedAlgorithm > > A GetRngGuid() was added to the RngLib in a previous patch, > allowing to identify the algorithm implemented by the RngLib. > Make use of this function. > > Signed-off-by: Pierre Gondois > --- > .../RngDxe/AArch64/AArch64Algo.c | 24 +++++++++---------- > .../RandomNumberGenerator/RngDxe/ArmRngDxe.c | 6 ++++- > .../RandomNumberGenerator/RngDxe/RngDxe.inf | 5 ++-- > 3 files changed, 19 insertions(+), 16 deletions(-) > > diff --git a/SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c b/SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c > index e8be217f8a8c..a1ff7bd58fda 100644 > --- a/SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c > +++ b/SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c > @@ -10,6 +10,7 @@ > #include > #include > #include > +#include > > #include "RngDxeInternals.h" > > @@ -28,9 +29,10 @@ GetAvailableAlgorithms ( > VOID > ) > { > - UINT64 DummyRand; > - UINT16 MajorRevision; > - UINT16 MinorRevision; > + EFI_STATUS Status; > + UINT16 MajorRevision; > + UINT16 MinorRevision; > + GUID RngGuid; > > // Rng algorithms 2 times, one for the allocation, one to populate. > mAvailableAlgoArray = AllocateZeroPool (RNG_AVAILABLE_ALGO_MAX); > @@ -38,24 +40,22 @@ GetAvailableAlgorithms ( > return EFI_OUT_OF_RESOURCES; > } > > - // Check RngGetBytes() before advertising PcdCpuRngSupportedAlgorithm. > - if (!EFI_ERROR (RngGetBytes (sizeof (DummyRand), (UINT8 *)&DummyRand))) { > + // Identify RngLib algorithm. > + Status = GetRngGuid (&RngGuid); > + if (!EFI_ERROR (Status)) { > CopyMem ( > &mAvailableAlgoArray[mAvailableAlgoArrayCount], > - PcdGetPtr (PcdCpuRngSupportedAlgorithm), > - sizeof (EFI_RNG_ALGORITHM) > + RngGuid, > + sizeof (RngGuid) > ); > mAvailableAlgoArrayCount++; > > - DEBUG_CODE_BEGIN (); > - if (IsZeroGuid (PcdGetPtr (PcdCpuRngSupportedAlgorithm))) { > + if (IsZeroGuid (&RngGuid)) { > DEBUG (( > DEBUG_WARN, > - "PcdCpuRngSupportedAlgorithm should be a non-zero GUID\n" > + "RngLib should have a non-zero GUID\n" > )); > } > - > - DEBUG_CODE_END (); > } > > // Raw algorithm (Trng) ... [SAMI] I think this code should do the following: ---   BOOLEAN UnSafeAlgo = FALSE;   mAvailableAlgoArrayCount = 0; // Identify RngLib algorithm.   Status = GetRngGuid (&RngGuid); if(!EFI_ERROR (Status)) { if((IsZeroGuid (&RngGuid)) ||         (CompareGuid (&RngGuid, &gEfiRngAlgorithmUnSafe))) { // Treat zero GUID as an unsafe algorithm       DEBUG ((         DEBUG_WARN, "RngLib uses an Unsafe algorithm and " "must not be used for production builds.\n"         )); // Set the UnSafeAlgo flag to indicate an unsafe algorithm was found // so that it can be added at the end of the algorithm list.       UnSafeAlgo = TRUE;     } else{       CopyMem (         &mAvailableAlgoArray[mAvailableAlgoArrayCount],         &RngGuid, sizeof(RngGuid)         );       mAvailableAlgoArrayCount++;     }   } // Raw algorithm (Trng) if(!EFI_ERROR (GetArmTrngVersion (&MajorRevision, &MinorRevision))) {     CopyMem (       &mAvailableAlgoArray[mAvailableAlgoArrayCount],       &gEfiRngAlgorithmRaw, sizeof(EFI_RNG_ALGORITHM)       );     mAvailableAlgoArrayCount++;   } // Add unsafe algorithms at the end of the list. if(UnSafeAlgo) {     CopyMem (       &mAvailableAlgoArray[mAvailableAlgoArrayCount],       &gEfiRngAlgorithmUnSafe, sizeof(EFI_RNG_ALGORITHM)       );     mAvailableAlgoArrayCount++;   } --- Doing this will not need the call to RngFindDefaultAlgo () introduced in the next patch "[PATCH v1 7/8] SecurityPkg/RngDxe: Select safe default Rng algorithm", which can be dropped. [SAMI] > diff --git a/SecurityPkg/RandomNumberGenerator/RngDxe/ArmRngDxe.c b/SecurityPkg/RandomNumberGenerator/RngDxe/ArmRngDxe.c > index ce49ff7ae661..78a18c5e1177 100644 > --- a/SecurityPkg/RandomNumberGenerator/RngDxe/ArmRngDxe.c > +++ b/SecurityPkg/RandomNumberGenerator/RngDxe/ArmRngDxe.c > @@ -78,6 +78,7 @@ RngGetRNG ( > { > EFI_STATUS Status; > UINTN Index; > + GUID RngGuid; > > if ((This == NULL) || (RNGValueLength == 0) || (RNGValue == NULL)) { > return EFI_INVALID_PARAMETER; > @@ -102,7 +103,10 @@ RngGetRNG ( > } > > FoundAlgo: > - if (CompareGuid (RNGAlgorithm, PcdGetPtr (PcdCpuRngSupportedAlgorithm))) { > + Status = GetRngGuid (&RngGuid); > + if (!EFI_ERROR (Status) && > + CompareGuid (RNGAlgorithm, &RngGuid)) > + { > Status = RngGetBytes (RNGValueLength, RNGValue); > return Status; > } > diff --git a/SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf b/SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf > index d6c2d30195bf..aa5743387ed9 100644 > --- a/SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf > +++ b/SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf > @@ -75,13 +75,12 @@ [Guids] > gEfiRngAlgorithmX9313DesGuid ## SOMETIMES_PRODUCES ## GUID # Unique ID of the algorithm for RNG > gEfiRngAlgorithmX931AesGuid ## SOMETIMES_PRODUCES ## GUID # Unique ID of the algorithm for RNG > gEfiRngAlgorithmRaw ## SOMETIMES_PRODUCES ## GUID # Unique ID of the algorithm for RNG > + gEfiRngAlgorithmArmRndr ## SOMETIMES_PRODUCES ## GUID # Unique ID of the algorithm for RNG [SAMI] Can the [Guids] section be split to relect the usage across architectures, please? > + gEfiRngAlgorithmUnSafe ## SOMETIMES_PRODUCES ## GUID # Unique ID of the algorithm for RNG > > [Protocols] > gEfiRngProtocolGuid ## PRODUCES > > -[Pcd.AARCH64] > - gEfiMdePkgTokenSpaceGuid.PcdCpuRngSupportedAlgorithm ## CONSUMES > - > [Depex] > TRUE > --------------glBP6q4tBcKx57zzf8ddxrrm Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable

Hi Pierre,

Thank you for this patch.

I think we should treat algorithms that have a zero GUID as unsafe. I have made some suggestions on those lines marked inline as [SAMI].

Regards,

Sami Mujawar

On 09/05/2023 08:40 am, pierre.gondois@arm.com wrote:
From: Pierre Gondois <pierr=
e.gondois@arm.com>

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=
=3D4151

The EFI_RNG_PROTOCOL can rely on the RngLib. The RngLib has multiple
implementations, some of them are unsafe (e.g. BaseRngLibTimerLib).
To allow the RngDxe to detect when such implementation is used,
a GetRngGuid() function was added in a previous patch.

The EFI_RNG_PROTOCOL can advertise multiple algorithms through
Guids. The PcdCpuRngSupportedAlgorithm is currently used to
advertise the RngLib in the Arm implementation.

The issues of doing that are:
- the RngLib implementation might not use CPU instructions,
  cf. the BaseRngLibTimerLib
- most platforms don't set PcdCpuRngSupportedAlgorithm

A GetRngGuid() was added to the RngLib in a previous patch,
allowing to identify the algorithm implemented by the RngLib.
Make use of this function.

Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
---
 .../RngDxe/AArch64/AArch64Algo.c              | 24 +++++++++----------
 .../RandomNumberGenerator/RngDxe/ArmRngDxe.c  |  6 ++++-
 .../RandomNumberGenerator/RngDxe/RngDxe.inf   |  5 ++--
 3 files changed, 19 insertions(+), 16 deletions(-)

diff --git a/SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c=
 b/SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c
index e8be217f8a8c..a1ff7bd58fda 100644
--- a/SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c
+++ b/SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c
@@ -10,6 +10,7 @@
 #include <Library/DebugLib.h>
 #include <Library/MemoryAllocationLib.h>
 #include <Library/ArmTrngLib.h>
+#include <Library/RngLib.h>
=20
 #include "RngDxeInternals.h"
=20
@@ -28,9 +29,10 @@ GetAvailableAlgorithms (
   VOID
   )
 {
-  UINT64  DummyRand;
-  UINT16  MajorRevision;
-  UINT16  MinorRevision;
+  EFI_STATUS  Status;
+  UINT16      MajorRevision;
+  UINT16      MinorRevision;
+  GUID        RngGuid;
=20
   // Rng algorithms 2 times, one for the allocation, one to populate.
   mAvailableAlgoArray =3D AllocateZeroPool (RNG_AVAILABLE_ALGO_MAX);
@@ -38,24 +40,22 @@ GetAvailableAlgorithms (
     return EFI_OUT_OF_RESOURCES;
   }
=20
-  // Check RngGetBytes() before advertising PcdCpuRngSupportedAlgorithm.
-  if (!EFI_ERROR (RngGetBytes (sizeof (DummyRand), (UINT8 *)&DummyRand=
))) {
<SNIP>
+  // Identify RngLib algorithm.
+  Status =3D GetRngGuid (&RngGuid);
+  if (!EFI_ERROR (Status)) {
     CopyMem (
       &mAvailableAlgoArray[mAvailableAlgoArrayCount],
-      PcdGetPtr (PcdCpuRngSupportedAlgorithm),
-      sizeof (EFI_RNG_ALGORITHM)
+      RngGuid,
+      sizeof (RngGuid)
       );
     mAvailableAlgoArrayCount++;
=20
-    DEBUG_CODE_BEGIN ();
-    if (IsZeroGuid (PcdGetPtr (PcdCpuRngSupportedAlgorithm))) {
+    if (IsZeroGuid (&RngGuid)) {
       DEBUG ((
         DEBUG_WARN,
-        "PcdCpuRngSupportedAlgorithm should be a non-zero GUID\n"=
;
+        "RngLib should have a non-zero GUID\n"
         ));
     }
-
-    DEBUG_CODE_END ();
   }
=20
   // Raw algorithm (Trng)

...

</SNIP>

[SAMI] I think this code should do the following:

---

  B= OOLEAN UnSafeAlgo =3D FALSE;
  mAvailableAlgoArrayCount =3D 0;
  // Iden= tify RngLib algorithm.
&nb= sp; Status =3D GetRngGuid (&RngGuid);
  if (!EFI_ERROR (Status)) {
    if ((IsZeroGuid (&RngGuid))= ||
      =   (CompareGuid (&RngGuid, &gEfiRngAlgorithmUnSafe))) {
      // Treat zero GUID as an unsafe algorithm
      DEBUG ((=
      &nbs= p; DEBUG_WARN,
  &nbs= p;     "RngLib uses a= n Unsafe algorithm and "
        &quo= t;must not be used for production builds.\n"
        ));
      // Set the UnSafeAlgo flag to indicate an unsafe algorithm w= as found
    &nb= sp; // so that it can be added at th= e end of the algorithm list.
      UnSafeAlgo =3D TRUE;
    } {
      CopyMem (
        &mAvailableAlgoA= rray[mAvailableAlgoArrayCount],
        &RngGuid,
        sizeof (RngGuid)
        );<= /span>
      mAvai= lableAlgoArrayCount++;
&nb= sp;   }
  }
  // Raw algorithm (Trng)
  if (!EFI_ERROR (GetArmTrngVersion (&MajorRevision, &= ;MinorRevision))) {
 =   CopyMem (
  &= nbsp;   &mAvailableAlgoArray[mAvailableAlgoArrayCount],
      &gEfiRngAlg= orithmRaw,
    &= nbsp; sizeof (EFI_RNG_ALGORITHM)
      );
    mAvailableAlgoArrayCount++;
  }
  // Add unsafe algorithms at the end of the list.
<= span style=3D"color: #000000;">  (UnSafeAlgo) {
    CopyMem (
<= span style=3D"color: #000000;">      &mAvailableAlgoArra= y[mAvailableAlgoArrayCount],
      &gEfiRngAlgorithmUnSafe,
      sizeof (EFI_RNG_ALGORITHM= )
    &nb= sp; );
    mAvai= lableAlgoArrayCount++;
&nb= sp; }
---

Doing this will not need the call to RngFindDefaultAlgo () introduced in the next patch "[PATCH v1 7/8] SecurityPkg/RngDxe: Select safe default Rng algorithm", which can be dropped.

[SAMI]

diff --git a/SecurityPkg/RandomNumberGenerator/RngDxe/ArmRngDxe.c b/Securit=
yPkg/RandomNumberGenerator/RngDxe/ArmRngDxe.c
index ce49ff7ae661..78a18c5e1177 100644
--- a/SecurityPkg/RandomNumberGenerator/RngDxe/ArmRngDxe.c
+++ b/SecurityPkg/RandomNumberGenerator/RngDxe/ArmRngDxe.c
@@ -78,6 +78,7 @@ RngGetRNG (
 {
   EFI_STATUS  Status;
   UINTN       Index;
+  GUID        RngGuid;
=20
   if ((This =3D=3D NULL) || (RNGValueLength =3D=3D 0) || (RNGValue =3D=3D =
NULL)) {
     return EFI_INVALID_PARAMETER;
@@ -102,7 +103,10 @@ RngGetRNG (
   }
=20
 FoundAlgo:
-  if (CompareGuid (RNGAlgorithm, PcdGetPtr (PcdCpuRngSupportedAlgorithm)))=
 {
+  Status =3D GetRngGuid (&RngGuid);
+  if (!EFI_ERROR (Status) &&
+      CompareGuid (RNGAlgorithm, &RngGuid))
+  {
     Status =3D RngGetBytes (RNGValueLength, RNGValue);
     return Status;
   }
diff --git a/SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf b/Security=
Pkg/RandomNumberGenerator/RngDxe/RngDxe.inf
index d6c2d30195bf..aa5743387ed9 100644
--- a/SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf
+++ b/SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf
@@ -75,13 +75,12 @@ [Guids]
   gEfiRngAlgorithmX9313DesGuid        ## SOMETIMES_PRODUCES    ## GUID    =
    # Unique ID of the algorithm for RNG
   gEfiRngAlgorithmX931AesGuid         ## SOMETIMES_PRODUCES    ## GUID    =
    # Unique ID of the algorithm for RNG
   gEfiRngAlgorithmRaw                 ## SOMETIMES_PRODUCES    ## GUID    =
    # Unique ID of the algorithm for RNG
+  gEfiRngAlgorithmArmRndr             ## SOMETIMES_PRODUCES    ## GUID    =
    # Unique ID of the algorithm for RNG
[SAMI] Can the [Guids] section be split to relect the usage across architectures, please? 
+  gEfiRngAlgorithmUnSafe              ## SOMETIMES_PRODUCES    ## GUID    =
    # Unique ID of the algorithm for RNG
=20
 [Protocols]
   gEfiRngProtocolGuid                ## PRODUCES
=20
-[Pcd.AARCH64]
-  gEfiMdePkgTokenSpaceGuid.PcdCpuRngSupportedAlgorithm      ## CONSUMES
-
 [Depex]
   TRUE
=20
--------------glBP6q4tBcKx57zzf8ddxrrm--