Re: [edk2] [PATCH v2 3/4] MdePkg/UefiLib: Simplify protocol un/installation abstraction
Ashish: UefiLib implementation simplification doesn't require to change library APIs. UninstallApi() interfaces are not required to be updated. Below Install API can still be kept. I don't think we need to keep the same interfaces for Install and Uninstall APIs. EfiLibUninstallAllDriverProtocols2 ( IN EFI_DRIVER_BINDING_PROTOCOL *DriverBinding, IN CONST EFI_COMPONENT_NAME_PROTOCOL*ComponentName,OPTIONAL IN CONST EFI_COMPONENT_NAME2_PROTOCOL *ComponentName2, OPTIONAL IN CONST EFI_DRIVER_CONFIGURATION_PROTOCOL *DriverConfiguration, OPTIONAL IN CONST EFI_DRIVER_CONFIGURATION2_PROTOCOL *DriverConfiguration2, OPTIONAL IN CONST EFI_DRIVER_DIAGNOSTICS_PROTOCOL*DriverDiagnostics,OPTIONAL IN CONST EFI_DRIVER_DIAGNOSTICS2_PROTOCOL *DriverDiagnostics2OPTIONAL ); Thanks Liming >-Original Message- >From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of >Ashish Singhal >Sent: Saturday, January 05, 2019 7:07 AM >To: edk2-devel@lists.01.org >Cc: Ashish Singhal >Subject: [edk2] [PATCH v2 3/4] MdePkg/UefiLib: Simplify protocol >un/installation abstraction > >Add a helper function to operate upon protocol installation and >uninstallation instead of every function doing it by itself. > >Contributed-under: TianoCore Contribution Agreement 1.1 >Signed-off-by: Ashish Singhal >--- > MdePkg/Include/Library/UefiLib.h | 26 +- > MdePkg/Library/UefiLib/UefiDriverModel.c | 1980 -- > 2 files changed, 270 insertions(+), 1736 deletions(-) > >diff --git a/MdePkg/Include/Library/UefiLib.h >b/MdePkg/Include/Library/UefiLib.h >index 08222d4..fbc9739 100644 >--- a/MdePkg/Include/Library/UefiLib.h >+++ b/MdePkg/Include/Library/UefiLib.h >@@ -1323,7 +1323,10 @@ EfiLibInstallDriverBinding ( > If DriverBinding is NULL, then ASSERT(). > If DriverBinding can not be uninstalled, then ASSERT(). > >+ @param ImageHandle The image handle of the driver. >+ @param SystemTable The EFI System Table that was passed to the >driver's entry point. > @param DriverBindingA Driver Binding Protocol instance that this > driver >produced. >+ @param DriverBindingHandle The handle that DriverBinding is to be >installed onto. > > @retval EFI_SUCCESS The protocol uninstallation successfully >completed. > @retval OthersStatus from gBS- >>UninstallMultipleProtocolInterfaces(). >@@ -1332,7 +1335,10 @@ EfiLibInstallDriverBinding ( > EFI_STATUS > EFIAPI > EfiLibUninstallDriverBinding ( >- IN EFI_DRIVER_BINDING_PROTOCOL *DriverBinding >+ IN CONST EFI_HANDLE ImageHandle, >+ IN CONST EFI_SYSTEM_TABLE *SystemTable, >+ IN EFI_DRIVER_BINDING_PROTOCOL *DriverBinding, >+ IN EFI_HANDLE DriverBindingHandle > ); > > >@@ -1382,7 +1388,10 @@ EfiLibInstallAllDriverProtocols ( > If DriverBinding is NULL, then ASSERT(). > If the uninstallation fails, then ASSERT(). > >+ @param ImageHandle The image handle of the driver. >+ @param SystemTable The EFI System Table that was passed to the >driver's entry point. > @param DriverBindingA Driver Binding Protocol instance that this > driver >produced. >+ @param DriverBindingHandle The handle that DriverBinding is to be >installed onto. > @param ComponentNameA Component Name Protocol instance that >this driver produced. > @param DriverConfiguration A Driver Configuration Protocol instance that >this driver produced. > @param DriverDiagnosticsA Driver Diagnostics Protocol instance that > this >driver produced. >@@ -1394,7 +1403,10 @@ EfiLibInstallAllDriverProtocols ( > EFI_STATUS > EFIAPI > EfiLibUninstallAllDriverProtocols ( >+ IN CONST EFI_HANDLE ImageHandle, >+ IN CONST EFI_SYSTEM_TABLE *SystemTable, > IN EFI_DRIVER_BINDING_PROTOCOL *DriverBinding, >+ IN EFI_HANDLE DriverBindingHandle, > IN CONST EFI_COMPONENT_NAME_PROTOCOL*ComponentName, >OPTIONAL > IN CONST EFI_DRIVER_CONFIGURATION_PROTOCOL *DriverConfiguration, >OPTIONAL > IN CONST EFI_DRIVER_DIAGNOSTICS_PROTOCOL*DriverDiagnostics >OPTIONAL >@@ -1442,7 +1454,10 @@ EfiLibInstallDriverBindingComponentName2 ( > If DriverBinding is NULL, then ASSERT(). > If the uninstallation fails, then ASSERT(). > >+ @param ImageHandle The image handle of the driver. >+ @param SystemTable The EFI System Table that was passed to the >driver's entry point. > @param DriverBindingA Driver Binding Protocol instance that this > driver >produced. >+ @param DriverBindingHandle The handle that DriverBinding is to be >installed onto. > @param ComponentNameA Component Name Protocol instance that >this driver produced. > @param ComponentName2 A Component Name 2 Protocol instance that >this driver produced. > >@@ -1453,7 +1468,10 @@ EfiLibInst
[edk2] [Patch v2 0/2] Avoid AP calls PeiServices table.
AP should not use PeiServices. The patch serial fix one issue related to this. This serial also include one patch used to refine the debug message. Eric Dong (2): UefiCpuPkg/RegisterCpuFeaturesLib: Enhance debug message. UefiCpuPkg/RegisterCpuFeaturesLib: Avoid AP calls PeiService. .../RegisterCpuFeaturesLib/CpuFeaturesInitialize.c | 39 +- .../DxeRegisterCpuFeaturesLib.c| 57 +++- .../PeiRegisterCpuFeaturesLib.c| 62 +- .../RegisterCpuFeaturesLib/RegisterCpuFeatures.h | 16 +- 4 files changed, 96 insertions(+), 78 deletions(-) -- 2.15.0.windows.1 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [Patch v2 1/2] UefiCpuPkg/RegisterCpuFeaturesLib: Enhance debug message.
Enhance debug message format to let them easy to read. Cc: Ruiyu Ni Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Eric Dong --- .../RegisterCpuFeaturesLib/CpuFeaturesInitialize.c | 17 +++-- 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c index 0a74d448c8..624ddee055 100644 --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c @@ -473,8 +473,9 @@ DumpRegisterTableOnProcessor ( case Msr: DEBUG (( DebugPrintErrorLevel, -"Processor: %d: MSR: %x, Bit Start: %d, Bit Length: %d, Value: %lx\r\n", +"Processor: %04d: Index %04d, MSR : %08x, Bit Start: %02d, Bit Length: %02d, Value: %016lx\r\n", ProcessorNumber, +FeatureIndex, RegisterTableEntry->Index, RegisterTableEntry->ValidBitStart, RegisterTableEntry->ValidBitLength, @@ -484,8 +485,9 @@ DumpRegisterTableOnProcessor ( case ControlRegister: DEBUG (( DebugPrintErrorLevel, -"Processor: %d:CR: %x, Bit Start: %d, Bit Length: %d, Value: %lx\r\n", +"Processor: %04d: Index %04d, CR : %08x, Bit Start: %02d, Bit Length: %02d, Value: %016lx\r\n", ProcessorNumber, +FeatureIndex, RegisterTableEntry->Index, RegisterTableEntry->ValidBitStart, RegisterTableEntry->ValidBitLength, @@ -495,8 +497,9 @@ DumpRegisterTableOnProcessor ( case MemoryMapped: DEBUG (( DebugPrintErrorLevel, -"Processor: %d: MMIO: %lx, Bit Start: %d, Bit Length: %d, Value: %lx\r\n", +"Processor: %04d: Index %04d, MMIO : %08lx, Bit Start: %02d, Bit Length: %02d, Value: %016lx\r\n", ProcessorNumber, +FeatureIndex, RegisterTableEntry->Index | LShiftU64 (RegisterTableEntry->HighIndex, 32), RegisterTableEntry->ValidBitStart, RegisterTableEntry->ValidBitLength, @@ -506,8 +509,9 @@ DumpRegisterTableOnProcessor ( case CacheControl: DEBUG (( DebugPrintErrorLevel, -"Processor: %d: CACHE: %x, Bit Start: %d, Bit Length: %d, Value: %lx\r\n", +"Processor: %04d: Index %04d, CACHE: %08lx, Bit Start: %02d, Bit Length: %02d, Value: %016lx\r\n", ProcessorNumber, +FeatureIndex, RegisterTableEntry->Index, RegisterTableEntry->ValidBitStart, RegisterTableEntry->ValidBitLength, @@ -517,8 +521,9 @@ DumpRegisterTableOnProcessor ( case Semaphore: DEBUG (( DebugPrintErrorLevel, -"Processor: %d: Semaphore: Scope Value: %s\r\n", +"Processor: %04d: Index %04d, SEMAP: %s\r\n", ProcessorNumber, +FeatureIndex, mDependTypeStr[MIN ((UINT32)RegisterTableEntry->Value, InvalidDepType)] )); break; @@ -833,7 +838,7 @@ ProgramProcessorRegister ( ApLocation->Thread; DEBUG (( DEBUG_INFO, -"Processor = %lu, Entry Index %lu, Type = %s!\n", +"Processor = %08lu, Index %08lu, Type = %s!\n", (UINT64)ThreadIndex, (UINT64)Index, mRegisterTypeStr[MIN ((REGISTER_TYPE)RegisterTableEntry->RegisterType, InvalidReg)] -- 2.15.0.windows.1 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [Patch 2/2] UefiCpuPkg/RegisterCpuFeaturesLib: Avoid AP calls PeiService.
V2: 1. Initialize CpuFeaturesData->MpService in CpuInitDataInitialize and make this function been called at the begin of the initialization. 2. let all other functions use CpuFeaturesData->MpService install of locate the protocol itself. V1: GetProcessorIndex function calls GetMpPpi to get the MP Ppi. Ap will calls GetProcessorIndex function which final let AP calls PeiService. This patch avoid GetProcessorIndex call PeiService. BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1411 Cc: Ruiyu Ni Cc: Laszlo Ersek Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Eric Dong --- .../RegisterCpuFeaturesLib/CpuFeaturesInitialize.c | 22 +--- .../DxeRegisterCpuFeaturesLib.c| 57 +++- .../PeiRegisterCpuFeaturesLib.c| 62 +- .../RegisterCpuFeaturesLib/RegisterCpuFeatures.h | 16 +- 4 files changed, 85 insertions(+), 72 deletions(-) diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c index 624ddee055..5866e022f0 100644 --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c @@ -138,7 +138,7 @@ FillProcessorInfo ( **/ VOID CpuInitDataInitialize ( - IN UINTN NumberOfCpus + VOID ) { EFI_STATUS Status; @@ -157,12 +157,22 @@ CpuInitDataInitialize ( ACPI_CPU_DATA*AcpiCpuData; CPU_STATUS_INFORMATION *CpuStatus; UINT32 *ValidCoreCountPerPackage; + UINTNNumberOfCpus; + UINTNNumberOfEnabledProcessors; Core= 0; Package = 0; Thread = 0; CpuFeaturesData = GetCpuFeaturesData (); + + // + // Initialize CpuFeaturesData->MpService as early as possile, so later function can use it. + // + CpuFeaturesData->MpService = GetMpService (); + + GetNumberOfProcessor (&NumberOfCpus, &NumberOfEnabledProcessors); + CpuFeaturesData->InitOrder = AllocateZeroPool (sizeof (CPU_FEATURES_INIT_ORDER) * NumberOfCpus); ASSERT (CpuFeaturesData->InitOrder != NULL); @@ -409,7 +419,7 @@ CollectProcessorData ( CPU_FEATURES_DATA*CpuFeaturesData; CpuFeaturesData = (CPU_FEATURES_DATA *)Buffer; - ProcessorNumber = GetProcessorIndex (); + ProcessorNumber = GetProcessorIndex (CpuFeaturesData); CpuInfo = &CpuFeaturesData->InitOrder[ProcessorNumber].CpuInfo; // // collect processor information @@ -1105,15 +1115,11 @@ CpuFeaturesDetect ( VOID ) { - UINTN NumberOfCpus; - UINTN NumberOfEnabledProcessors; CPU_FEATURES_DATA *CpuFeaturesData; CpuFeaturesData = GetCpuFeaturesData(); - GetNumberOfProcessor (&NumberOfCpus, &NumberOfEnabledProcessors); - - CpuInitDataInitialize (NumberOfCpus); + CpuInitDataInitialize (); // // Wakeup all APs for data collection. @@ -1125,6 +1131,6 @@ CpuFeaturesDetect ( // CollectProcessorData (CpuFeaturesData); - AnalysisProcessorFeatures (NumberOfCpus); + AnalysisProcessorFeatures (CpuFeaturesData->NumberOfCpus); } diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.c b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.c index 926698dc95..2d2095e437 100644 --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.c +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.c @@ -20,7 +20,6 @@ #include "RegisterCpuFeatures.h" CPU_FEATURES_DATA mCpuFeaturesData = {0}; -EFI_MP_SERVICES_PROTOCOL *mCpuFeaturesMpServices = NULL; /** Worker function to get CPU_FEATURES_DATA pointer. @@ -40,44 +39,44 @@ GetCpuFeaturesData ( @return Pointer to EFI_MP_SERVICES_PROTOCOL. **/ -EFI_MP_SERVICES_PROTOCOL * -GetMpProtocol ( +VOID * +GetMpService ( VOID ) { - EFI_STATUS Status; - - if (mCpuFeaturesMpServices == NULL) { -// -// Get MP Services Protocol -// -Status = gBS->LocateProtocol ( - &gEfiMpServiceProtocolGuid, - NULL, - (VOID **)&mCpuFeaturesMpServices - ); -ASSERT_EFI_ERROR (Status); - } + EFI_STATUSStatus; + EFI_MP_SERVICES_PROTOCOL *MpServices; - ASSERT (mCpuFeaturesMpServices != NULL); - return mCpuFeaturesMpServices; + // + // Get MP Services Protocol + // + Status = gBS->LocateProtocol ( +&gEfiMpServiceProtocolGuid, +NULL, +(VOID **)&MpServices +); + ASSERT_EFI_ERROR (Status); + + return MpServices; } /** Worker function to return processor index. + @param CpuFeaturesDataCpu Feature Data structure. + @return The processor index. **/ UINTN GetProcessorIndex
Re: [edk2] [PATCH v2 4/4] NetworkPkg/IScsiDxe: Update UEFILib Usage
Reviewed-by: Siyuan Fu > -Original Message- > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Ashish > Singhal > Sent: Saturday, January 5, 2019 7:07 AM > To: edk2-devel@lists.01.org > Cc: Ashish Singhal > Subject: [edk2] [PATCH v2 4/4] NetworkPkg/IScsiDxe: Update UEFILib Usage > > Update interfaces as exposed by UEFILib for protocol > installation and uninstallation abstraction. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Ashish Singhal > --- > NetworkPkg/IScsiDxe/IScsiDriver.c | 6 ++ > 1 file changed, 6 insertions(+) > > diff --git a/NetworkPkg/IScsiDxe/IScsiDriver.c > b/NetworkPkg/IScsiDxe/IScsiDriver.c > index 8747de7..fa0ea00 100644 > --- a/NetworkPkg/IScsiDxe/IScsiDriver.c > +++ b/NetworkPkg/IScsiDxe/IScsiDriver.c > @@ -1863,14 +1863,20 @@ Error3: > > Error2: >EfiLibUninstallDriverBindingComponentName2 ( > +ImageHandle, > +SystemTable, > &gIScsiIp6DriverBinding, > +NULL, > &gIScsiComponentName, > &gIScsiComponentName2 > ); > > Error1: >EfiLibUninstallDriverBindingComponentName2 ( > +ImageHandle, > +SystemTable, > &gIScsiIp4DriverBinding, > +NULL, > &gIScsiComponentName, > &gIScsiComponentName2 > ); > -- > 2.7.4 > > ___ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2 2/4] NetworkPkg/IScsiDxe: Use UEFILib APIs to uninstall protocols.
Hi, Ashish The changes to NetworkPkg is good to me. Please add the package maintainer's name to the cc list of the patch mail in future, thanks. Reviewed-by: Fu Siyuan > -Original Message- > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Ashish > Singhal > Sent: Saturday, January 5, 2019 7:07 AM > To: edk2-devel@lists.01.org > Cc: Ashish Singhal > Subject: [edk2] [PATCH v2 2/4] NetworkPkg/IScsiDxe: Use UEFILib APIs to > uninstall protocols. > > During cleanup in case of initialization failure, some driver > bindings are not installed. Using abstractions in UEFILib takes > care of it. > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1428 > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Ashish Singhal > --- > NetworkPkg/IScsiDxe/IScsiDriver.c | 31 +++ > 1 file changed, 11 insertions(+), 20 deletions(-) > > diff --git a/NetworkPkg/IScsiDxe/IScsiDriver.c > b/NetworkPkg/IScsiDxe/IScsiDriver.c > index 91176e6..8747de7 100644 > --- a/NetworkPkg/IScsiDxe/IScsiDriver.c > +++ b/NetworkPkg/IScsiDxe/IScsiDriver.c > @@ -1,6 +1,7 @@ > /** @file >The entry point of IScsi driver. > > +Copyright (c) 2019, NVIDIA Corporation. All rights reserved. > Copyright (c) 2004 - 2018, Intel Corporation. All rights reserved. > (C) Copyright 2017 Hewlett Packard Enterprise Development LP > > @@ -1861,28 +1862,18 @@ Error3: > ); > > Error2: > - gBS->UninstallMultipleProtocolInterfaces ( > - gIScsiIp6DriverBinding.DriverBindingHandle, > - &gEfiDriverBindingProtocolGuid, > - &gIScsiIp6DriverBinding, > - &gEfiComponentName2ProtocolGuid, > - &gIScsiComponentName2, > - &gEfiComponentNameProtocolGuid, > - &gIScsiComponentName, > - NULL > - ); > + EfiLibUninstallDriverBindingComponentName2 ( > +&gIScsiIp6DriverBinding, > +&gIScsiComponentName, > +&gIScsiComponentName2 > +); > > Error1: > - gBS->UninstallMultipleProtocolInterfaces ( > - ImageHandle, > - &gEfiDriverBindingProtocolGuid, > - &gIScsiIp4DriverBinding, > - &gEfiComponentName2ProtocolGuid, > - &gIScsiComponentName2, > - &gEfiComponentNameProtocolGuid, > - &gIScsiComponentName, > - NULL > - ); > + EfiLibUninstallDriverBindingComponentName2 ( > +&gIScsiIp4DriverBinding, > +&gIScsiComponentName, > +&gIScsiComponentName2 > +); > >return Status; > } > -- > 2.7.4 > > ___ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2 3/4] MdePkg/UefiLib: Simplify protocol un/installation abstraction
Hello Liming, I am not touching APIs for Install and am OK keeping Uninstall API same as what I had in patch 1/4. I thought it would be easier for the developer to keep the interface similar to install but I do not have a strong preference either way. If you are OK with the Uninstall API as in patch 1/4, I am OK submitting a new patch where I can squash 1/4 and 3/4 together into a single commit and keep API as in 1/4. Thanks Ashish -Original Message- From: Gao, Liming Sent: Sunday, January 6, 2019 5:33 PM To: Ashish Singhal ; edk2-devel@lists.01.org Subject: RE: [edk2] [PATCH v2 3/4] MdePkg/UefiLib: Simplify protocol un/installation abstraction Ashish: UefiLib implementation simplification doesn't require to change library APIs. UninstallApi() interfaces are not required to be updated. Below Install API can still be kept. I don't think we need to keep the same interfaces for Install and Uninstall APIs. EfiLibUninstallAllDriverProtocols2 ( IN EFI_DRIVER_BINDING_PROTOCOL *DriverBinding, IN CONST EFI_COMPONENT_NAME_PROTOCOL*ComponentName,OPTIONAL IN CONST EFI_COMPONENT_NAME2_PROTOCOL *ComponentName2, OPTIONAL IN CONST EFI_DRIVER_CONFIGURATION_PROTOCOL *DriverConfiguration, OPTIONAL IN CONST EFI_DRIVER_CONFIGURATION2_PROTOCOL *DriverConfiguration2, OPTIONAL IN CONST EFI_DRIVER_DIAGNOSTICS_PROTOCOL*DriverDiagnostics,OPTIONAL IN CONST EFI_DRIVER_DIAGNOSTICS2_PROTOCOL *DriverDiagnostics2OPTIONAL ); Thanks Liming >-Original Message- >From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of >Ashish Singhal >Sent: Saturday, January 05, 2019 7:07 AM >To: edk2-devel@lists.01.org >Cc: Ashish Singhal >Subject: [edk2] [PATCH v2 3/4] MdePkg/UefiLib: Simplify protocol >un/installation abstraction > >Add a helper function to operate upon protocol installation and >uninstallation instead of every function doing it by itself. > >Contributed-under: TianoCore Contribution Agreement 1.1 >Signed-off-by: Ashish Singhal >--- > MdePkg/Include/Library/UefiLib.h | 26 +- > MdePkg/Library/UefiLib/UefiDriverModel.c | 1980 -- > 2 files changed, 270 insertions(+), 1736 deletions(-) > >diff --git a/MdePkg/Include/Library/UefiLib.h >b/MdePkg/Include/Library/UefiLib.h >index 08222d4..fbc9739 100644 >--- a/MdePkg/Include/Library/UefiLib.h >+++ b/MdePkg/Include/Library/UefiLib.h >@@ -1323,7 +1323,10 @@ EfiLibInstallDriverBinding ( > If DriverBinding is NULL, then ASSERT(). > If DriverBinding can not be uninstalled, then ASSERT(). > >+ @param ImageHandle The image handle of the driver. >+ @param SystemTable The EFI System Table that was passed to the >driver's entry point. > @param DriverBindingA Driver Binding Protocol instance that this > driver >produced. >+ @param DriverBindingHandle The handle that DriverBinding is to be >installed onto. > > @retval EFI_SUCCESS The protocol uninstallation successfully >completed. > @retval OthersStatus from gBS- >>UninstallMultipleProtocolInterfaces(). >@@ -1332,7 +1335,10 @@ EfiLibInstallDriverBinding ( > EFI_STATUS > EFIAPI > EfiLibUninstallDriverBinding ( >- IN EFI_DRIVER_BINDING_PROTOCOL *DriverBinding >+ IN CONST EFI_HANDLE ImageHandle, >+ IN CONST EFI_SYSTEM_TABLE *SystemTable, >+ IN EFI_DRIVER_BINDING_PROTOCOL *DriverBinding, >+ IN EFI_HANDLE DriverBindingHandle > ); > > >@@ -1382,7 +1388,10 @@ EfiLibInstallAllDriverProtocols ( > If DriverBinding is NULL, then ASSERT(). > If the uninstallation fails, then ASSERT(). > >+ @param ImageHandle The image handle of the driver. >+ @param SystemTable The EFI System Table that was passed to the >driver's entry point. > @param DriverBindingA Driver Binding Protocol instance that this > driver >produced. >+ @param DriverBindingHandle The handle that DriverBinding is to be >installed onto. > @param ComponentNameA Component Name Protocol instance that >this driver produced. > @param DriverConfiguration A Driver Configuration Protocol instance that >this driver produced. > @param DriverDiagnosticsA Driver Diagnostics Protocol instance that > this >driver produced. >@@ -1394,7 +1403,10 @@ EfiLibInstallAllDriverProtocols ( > EFI_STATUS > EFIAPI > EfiLibUninstallAllDriverProtocols ( >+ IN CONST EFI_HANDLE ImageHandle, >+ IN CONST EFI_SYSTEM_TABLE *SystemTable, > IN EFI_DRIVER_BINDING_PROTOCOL *DriverBinding, >+ IN EFI_HANDLE DriverBindingHandle, > IN CONST EFI_COMPONENT_NAME_PROTOCOL*ComponentName, >OPTIONAL > IN CONST EFI_DRIVER_CONFIGURATION_PROTOCOL *DriverConfiguration, >OPTIONAL > IN CONST EFI_DRIVER_DIAGNOSTICS_PROTOCOL*DriverDiagnostics >OPTIONAL >@@ -1442,7 +1454,10 @@ EfiLibInstallDriverBindingComponentName2 ( > I
[edk2] [PATCH] MdePkg/UefiLib: Add a new API GetVariable3
From: jiansonx BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1396 Add a new API GetVariable3, which can return the attributes of a variable during reading it. Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Jiansong Xu Cc: Liming Gao GetVariable. + +**/ +EFI_STATUS +EFIAPI +GetVariable3( + IN CONST CHAR16 *Name, + IN CONST EFI_GUID *Guid, + OUT VOID **Value, + OUT UINTN *Size OPTIONAL, + OUT UINT32 *Attr OPTIONAL + ); + /** Returns a pointer to an allocated buffer that contains the best matching language from a set of supported languages. diff --git a/MdePkg/Library/UefiLib/UefiLib.c b/MdePkg/Library/UefiLib/UefiLib.c index 39d0ce2b21..475017690c 100644 --- a/MdePkg/Library/UefiLib/UefiLib.c +++ b/MdePkg/Library/UefiLib/UefiLib.c @@ -1439,6 +1439,87 @@ GetVariable2 ( return Status; } +/** Return the attributes of the variable. + + Returns the status whether get the variable success. The function retrieves + variable through the UEFI Runtime Service GetVariable(). The + returned buffer is allocated using AllocatePool(). The caller is responsible + for freeing this buffer with FreePool(). The attributes are returned if + the caller provides a valid Attribute parameter. + + If Name is NULL, then ASSERT(). + If Guid is NULL, then ASSERT(). + If Value is NULL, then ASSERT(). + + @param[in] Name The pointer to a Null-terminated Unicode string. + @param[in] Guid The pointer to an EFI_GUID structure + @param[out] Value The buffer point saved the variable info. + @param[out] Size The buffer size of the variable. + @param[out] Attr The pointer to the variable attributes as found in var store + + @retval EFI_OUT_OF_RESOURCES Allocate buffer failed. + @retval EFI_SUCCESS Find the specified variable. + @retval Others Errors Return errors from call to gRT->GetVariable. + +**/ +EFI_STATUS +EFIAPI +GetVariable3( + IN CONST CHAR16 *Name, + IN CONST EFI_GUID *Guid, + OUT VOID **Value, + OUT UINTN *Size OPTIONAL, + OUT UINT32 *Attr OPTIONAL + ) +{ + EFI_STATUS Status; + UINTN BufferSize; + + ASSERT(Name != NULL && Guid != NULL && Value != NULL); + + // + // Try to get the variable size. + // + BufferSize = 0; + *Value = NULL; + if (Size != NULL) { +*Size = 0; + } + + if (Attr != NULL) { +*Attr = 0; + } + + Status = gRT->GetVariable((CHAR16 *)Name, (EFI_GUID *)Guid, Attr, &BufferSize, *Value); + if (Status != EFI_BUFFER_TOO_SMALL) { +return Status; + } + + // + // Allocate buffer to get the variable. + // + *Value = AllocatePool(BufferSize); + ASSERT(*Value != NULL); + if (*Value == NULL) { +return EFI_OUT_OF_RESOURCES; + } + + // + // Get the variable data. + // + Status = gRT->GetVariable((CHAR16 *)Name, (EFI_GUID *)Guid, Attr, &BufferSize, *Value); + if (EFI_ERROR(Status)) { +FreePool(*Value); +*Value = NULL; + } + + if (Size != NULL) { +*Size = BufferSize; + } + + return Status; +} + /** Returns a pointer to an allocated buffer that contains the contents of a variable retrieved through the UEFI Runtime Service GetVariable(). This -- 2.16.2.windows.1 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH v3 2/2] NetworkPkg/IScsiDxe: Use UEFILib APIs to uninstall protocols.
During cleanup in case of initialization failure, some driver bindings are not installed. Using abstractions in UEFILib takes care of it. REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1428 Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Ashish Singhal --- NetworkPkg/IScsiDxe/IScsiDriver.c | 31 +++ 1 file changed, 11 insertions(+), 20 deletions(-) diff --git a/NetworkPkg/IScsiDxe/IScsiDriver.c b/NetworkPkg/IScsiDxe/IScsiDriver.c index 91176e6..8747de7 100644 --- a/NetworkPkg/IScsiDxe/IScsiDriver.c +++ b/NetworkPkg/IScsiDxe/IScsiDriver.c @@ -1,6 +1,7 @@ /** @file The entry point of IScsi driver. +Copyright (c) 2019, NVIDIA Corporation. All rights reserved. Copyright (c) 2004 - 2018, Intel Corporation. All rights reserved. (C) Copyright 2017 Hewlett Packard Enterprise Development LP @@ -1861,28 +1862,18 @@ Error3: ); Error2: - gBS->UninstallMultipleProtocolInterfaces ( - gIScsiIp6DriverBinding.DriverBindingHandle, - &gEfiDriverBindingProtocolGuid, - &gIScsiIp6DriverBinding, - &gEfiComponentName2ProtocolGuid, - &gIScsiComponentName2, - &gEfiComponentNameProtocolGuid, - &gIScsiComponentName, - NULL - ); + EfiLibUninstallDriverBindingComponentName2 ( +&gIScsiIp6DriverBinding, +&gIScsiComponentName, +&gIScsiComponentName2 +); Error1: - gBS->UninstallMultipleProtocolInterfaces ( - ImageHandle, - &gEfiDriverBindingProtocolGuid, - &gIScsiIp4DriverBinding, - &gEfiComponentName2ProtocolGuid, - &gIScsiComponentName2, - &gEfiComponentNameProtocolGuid, - &gIScsiComponentName, - NULL - ); + EfiLibUninstallDriverBindingComponentName2 ( +&gIScsiIp4DriverBinding, +&gIScsiComponentName, +&gIScsiComponentName2 +); return Status; } -- 2.7.4 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH v3 0/2] Provide UEFILib functions for protocol uninstallation.
An issue was seen in IScsiDxe in NetworkPkg where driver cleanup after initialization failure was not done right. Bug 1428 was filed in this regard. As per discussions with Mike, it was also discussed that having UEFILib provide protocol uninstallation abstraction would help to avoid these issues in the future. Bug 1429 was found to track this. The first 2 patches take care of this. Patch number 1 also simplifies the UEFILib protocol installation and uninstallation abstraction by adding a helper function doing operations instead of every public function. Ashish Singhal (2): MdePkg/UefiLib: Abstract driver model protocol uninstallation NetworkPkg/IScsiDxe: Use UEFILib APIs to uninstall protocols. MdePkg/Include/Library/UefiLib.h | 103 +++ MdePkg/Library/UefiLib/UefiDriverModel.c | 1186 -- NetworkPkg/IScsiDxe/IScsiDriver.c| 31 +- 3 files changed, 435 insertions(+), 885 deletions(-) -- 2.7.4 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH v3 1/2] MdePkg/UefiLib: Abstract driver model protocol uninstallation
Provided functions in UEFILib that abstract driver model protocol uninstallation. This helps drivers to install and uninstall protocols using a library to keep things seemless. Also, add a helper function to operate upon protocol installation and uninstallation instead of every function doing it by itself. REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1429 Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Ashish Singhal --- MdePkg/Include/Library/UefiLib.h | 103 +++ MdePkg/Library/UefiLib/UefiDriverModel.c | 1186 -- 2 files changed, 424 insertions(+), 865 deletions(-) diff --git a/MdePkg/Include/Library/UefiLib.h b/MdePkg/Include/Library/UefiLib.h index 468bffc..08222d4 100644 --- a/MdePkg/Include/Library/UefiLib.h +++ b/MdePkg/Include/Library/UefiLib.h @@ -12,6 +12,7 @@ of size reduction when compiler optimization is disabled. If MDEPKG_NDEBUG is defined, then debug and assert related macros wrapped by it are the NULL implementations. +Copyright (c) 2019, NVIDIA Corporation. All rights reserved. Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved. This program and the accompanying materials are licensed and made available under the terms and conditions of the BSD License that accompanies this distribution. @@ -1283,6 +1284,7 @@ AsciiPrintXY ( ... ); + /** Installs and completes the initialization of a Driver Binding Protocol instance. @@ -1316,6 +1318,25 @@ EfiLibInstallDriverBinding ( /** + Uninstalls a Driver Binding Protocol instance. + + If DriverBinding is NULL, then ASSERT(). + If DriverBinding can not be uninstalled, then ASSERT(). + + @param DriverBindingA Driver Binding Protocol instance that this driver produced. + + @retval EFI_SUCCESS The protocol uninstallation successfully completed. + @retval OthersStatus from gBS->UninstallMultipleProtocolInterfaces(). + +**/ +EFI_STATUS +EFIAPI +EfiLibUninstallDriverBinding ( + IN EFI_DRIVER_BINDING_PROTOCOL *DriverBinding + ); + + +/** Installs and completes the initialization of a Driver Binding Protocol instance and optionally installs the Component Name, Driver Configuration and Driver Diagnostics Protocols. @@ -1354,6 +1375,31 @@ EfiLibInstallAllDriverProtocols ( ); +/** + Uninstalls a Driver Binding Protocol instance and optionally uninstalls the + Component Name, Driver Configuration and Driver Diagnostics Protocols. + + If DriverBinding is NULL, then ASSERT(). + If the uninstallation fails, then ASSERT(). + + @param DriverBindingA Driver Binding Protocol instance that this driver produced. + @param ComponentNameA Component Name Protocol instance that this driver produced. + @param DriverConfiguration A Driver Configuration Protocol instance that this driver produced. + @param DriverDiagnosticsA Driver Diagnostics Protocol instance that this driver produced. + + @retval EFI_SUCCESS The protocol uninstallation successfully completed. + @retval OthersStatus from gBS->UninstallMultipleProtocolInterfaces(). + +**/ +EFI_STATUS +EFIAPI +EfiLibUninstallAllDriverProtocols ( + IN EFI_DRIVER_BINDING_PROTOCOL *DriverBinding, + IN CONST EFI_COMPONENT_NAME_PROTOCOL*ComponentName, OPTIONAL + IN CONST EFI_DRIVER_CONFIGURATION_PROTOCOL *DriverConfiguration, OPTIONAL + IN CONST EFI_DRIVER_DIAGNOSTICS_PROTOCOL*DriverDiagnosticsOPTIONAL + ); + /** Installs Driver Binding Protocol with optional Component Name and Component Name 2 Protocols. @@ -1391,6 +1437,29 @@ EfiLibInstallDriverBindingComponentName2 ( /** + Uninstalls Driver Binding Protocol with optional Component Name and Component Name 2 Protocols. + + If DriverBinding is NULL, then ASSERT(). + If the uninstallation fails, then ASSERT(). + + @param DriverBindingA Driver Binding Protocol instance that this driver produced. + @param ComponentNameA Component Name Protocol instance that this driver produced. + @param ComponentName2 A Component Name 2 Protocol instance that this driver produced. + + @retval EFI_SUCCESS The protocol installation successfully completed. + @retval OthersStatus from gBS->UninstallMultipleProtocolInterfaces(). + +**/ +EFI_STATUS +EFIAPI +EfiLibUninstallDriverBindingComponentName2 ( + IN EFI_DRIVER_BINDING_PROTOCOL *DriverBinding, + IN CONST EFI_COMPONENT_NAME_PROTOCOL*ComponentName, OPTIONAL + IN CONST EFI_COMPONENT_NAME2_PROTOCOL *ComponentName2 OPTIONAL + ); + + +/** Installs Driver Binding Protocol with optional Component Name, Component Name 2, Driver Configuration, Driver Configuration 2, Driver Diagnostics, and Driver Diagnostics 2 Protocols. @@ -1434,6 +1503,40 @@ EfiLibInstallAllDriverProtocols2 ( IN CONST EFI_DRIVER_DIAGNOSTICS2_PROTOCOL *DriverDiagnostics2
[edk2] [PATCH] BaseTools/tools_def ARM GCC5: disable LTO for ASLC invocations
GCC for 32-bit ARM chokes on .aslc files when running with LTO enabled. Since LTO has no benefit whatsoever here, just disable it. Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Ard Biesheuvel --- BaseTools/Conf/tools_def.template | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template index f7eb87af14c2..e68cfd9a4997 100755 --- a/BaseTools/Conf/tools_def.template +++ b/BaseTools/Conf/tools_def.template @@ -5145,7 +5145,7 @@ RELEASE_GCC5_X64_DLINK_FLAGS = DEF(GCC5_X64_DLINK_FLAGS) -flto -Os *_GCC5_ARM_RC_PATH = ENV(GCC5_ARM_PREFIX)objcopy *_GCC5_ARM_ARCHCC_FLAGS = -mthumb -*_GCC5_ARM_ASLCC_FLAGS = DEF(GCC_ASLCC_FLAGS) +*_GCC5_ARM_ASLCC_FLAGS = DEF(GCC_ASLCC_FLAGS) -fno-lto *_GCC5_ARM_ASLDLINK_FLAGS= DEF(GCC5_ARM_ASLDLINK_FLAGS) *_GCC5_ARM_ASM_FLAGS = DEF(GCC5_ARM_ASM_FLAGS) *_GCC5_ARM_DLINK2_FLAGS = DEF(GCC5_ARM_DLINK2_FLAGS) -- 2.20.1 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH] ArmPkg/ArmMmuLib ARM: disregard high memory when setting permissions
Ignore calls to ArmSetMemoryAttributes () when the region described is outside of the 32-bit addressable range. This memory is not mapped in the first place, and the current code does not deal with the high bits correctly, resulting in hangs. Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Ard Biesheuvel --- ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c | 4 1 file changed, 4 insertions(+) diff --git a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c index 3b3b20aa9b78..bffab83d4fd0 100644 --- a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c +++ b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c @@ -744,6 +744,10 @@ ArmSetMemoryAttributes ( UINT64ChunkLength; BOOLEAN FlushTlbs; + if (BaseAddress > (UINT64)MAX_ADDRESS - Length + 1) { +return EFI_UNSUPPORTED; + } + if (Length == 0) { return EFI_SUCCESS; } -- 2.20.1 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH 4/5] ArmPkg/ArmMmuLib AARCH64: add support for read-only page tables
As a hardening measure, implement support for remapping all page tables read-only at a certain point during the boot (end of DXE is the most appropriate trigger). This should make it a lot more difficult to take advantage of write exploits to defeat authentication checks, since the attacker can no longer manipulate the page tables directly. To allow the page tables to still be manipulated, make use of the existing code to manipulate live entries: this drops into assembler with interrupts off, and disables the MMU for a brief moment to avoid causing TLB conflicts. Since page tables are writable with the MMU off, we can reuse this code to still manipulate the page tables after we updated the CPU mappings to be read-only. Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Ard Biesheuvel --- ArmPkg/Include/Library/ArmMmuLib.h | 6 + ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 119 ++-- ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c | 8 ++ 3 files changed, 123 insertions(+), 10 deletions(-) diff --git a/ArmPkg/Include/Library/ArmMmuLib.h b/ArmPkg/Include/Library/ArmMmuLib.h index d2725810f1c6..f0832b91bf17 100644 --- a/ArmPkg/Include/Library/ArmMmuLib.h +++ b/ArmPkg/Include/Library/ArmMmuLib.h @@ -70,4 +70,10 @@ ArmSetMemoryAttributes ( IN UINT64Attributes ); +VOID +EFIAPI +MapAllPageTablesReadOnly ( + VOID + ); + #endif diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c index b59c081a7e49..cefaad9961ea 100644 --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c @@ -28,6 +28,8 @@ // We use this index definition to define an invalid block entry #define TT_ATTR_INDX_INVALID((UINT32)~0) +STATIC BOOLEAN mReadOnlyPageTables; + STATIC UINT64 ArmMemoryAttributeToPageAttribute ( @@ -137,6 +139,9 @@ ReplaceLiveEntry ( IN UINT64 Address ) { + if (*Entry == Value) { +return; + } if (!ArmMmuEnabled ()) { *Entry = Value; } else { @@ -181,7 +186,8 @@ GetBlockEntryListFromAddress ( IN UINT64RegionStart, OUT UINTN*TableLevel, IN OUT UINT64*BlockEntrySize, - OUT UINT64 **LastBlockEntry + OUT UINT64 **LastBlockEntry, + OUT BOOLEAN *NewPageTablesAllocated ) { UINTN RootTableLevel; @@ -292,6 +298,8 @@ GetBlockEntryListFromAddress ( return NULL; } +*NewPageTablesAllocated = TRUE; + // Populate the newly created lower level table SubTableBlockEntry = TranslationTable; for (Index = 0; Index < TT_ENTRY_COUNT; Index++) { @@ -316,10 +324,18 @@ GetBlockEntryListFromAddress ( return NULL; } +*NewPageTablesAllocated = TRUE; + ZeroMem (TranslationTable, TT_ENTRY_COUNT * sizeof(UINT64)); // Fill the new BlockEntry with the TranslationTable -*BlockEntry = ((UINTN)TranslationTable & TT_ADDRESS_MASK_DESCRIPTION_TABLE) | TT_TYPE_TABLE_ENTRY; +if (!mReadOnlyPageTables) { + *BlockEntry = (UINTN)TranslationTable | TT_TYPE_TABLE_ENTRY; +} else { + ReplaceLiveEntry (BlockEntry, +(UINTN)TranslationTable | TT_TYPE_TABLE_ENTRY, +RegionStart); +} } } } @@ -345,7 +361,8 @@ UpdateRegionMapping ( IN UINT64 RegionStart, IN UINT64 RegionLength, IN UINT64 Attributes, - IN UINT64 BlockEntryMask + IN UINT64 BlockEntryMask, + OUT BOOLEAN *ReadOnlyRemapDone ) { UINT32 Type; @@ -353,6 +370,7 @@ UpdateRegionMapping ( UINT64 *LastBlockEntry; UINT64 BlockEntrySize; UINTN TableLevel; + BOOLEAN NewPageTablesAllocated; // Ensure the Length is aligned on 4KB boundary if ((RegionLength == 0) || ((RegionLength & (SIZE_4KB - 1)) != 0)) { @@ -360,11 +378,13 @@ UpdateRegionMapping ( return EFI_INVALID_PARAMETER; } + NewPageTablesAllocated = FALSE; do { // Get the first Block Entry that matches the Virtual Address and also the information on the Table Descriptor // such as the the size of the Block Entry and the address of the last BlockEntry of the Table Descriptor BlockEntrySize = RegionLength; -BlockEntry = GetBlockEntryListFromAddress (RootTable, RegionStart, &TableLevel, &BlockEntrySize, &LastBlockEntry); +BlockEntry = GetBlockEntryListFromAddress (RootTable, RegionStart, + &TableLevel, &BlockEntrySize, &LastBlockEntry, &NewPageTablesAllocated); if (BlockEntry == NULL) { // GetBlockEntryListFromAddress() return NULL when it fails to allocate new pages from the Translation Tables return EFI_OUT_OF_RESOURCES; @@ -378,10 +398,16 @@ UpdateRegionMapping ( do { // Fill the Block Entry with attribute and output block address - *BlockEntry &= BlockEntryMask; - *BlockEntry |= (RegionStart & TT_ADDRESS_MASK_BLOCK_ENTRY) | Attribut
[edk2] [PATCH 2/5] ArmPkg/ArmMmuLib AARCH64: get rid of needless TLB invalidation
Currently, we always invalidate the TLBs entirely after making any modification to the page tables. Now that we have introduced strict memory permissions in quite a number of places, such modifications occur much more often, and it is better for performance to flush only those TLB entries that are actually affected by the changes. Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Ard Biesheuvel --- ArmPkg/Include/Library/ArmMmuLib.h | 3 ++- ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S| 6 +++--- ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 16 +++- ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S | 14 -- 4 files changed, 20 insertions(+), 19 deletions(-) diff --git a/ArmPkg/Include/Library/ArmMmuLib.h b/ArmPkg/Include/Library/ArmMmuLib.h index fb7fd006417c..d2725810f1c6 100644 --- a/ArmPkg/Include/Library/ArmMmuLib.h +++ b/ArmPkg/Include/Library/ArmMmuLib.h @@ -59,7 +59,8 @@ VOID EFIAPI ArmReplaceLiveTranslationEntry ( IN UINT64 *Entry, - IN UINT64 Value + IN UINT64 Value, + IN UINT64 Address ); EFI_STATUS diff --git a/ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S b/ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S index b7173e00b039..175fb58206b6 100644 --- a/ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S +++ b/ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S @@ -124,15 +124,15 @@ ASM_FUNC(ArmSetMAIR) // IN VOID *MVA// X1 // ); ASM_FUNC(ArmUpdateTranslationTableEntry) - dc civac, x0 // Clean and invalidate data line - dsb sy + dsb nshst + lsr x1, x1, #12 EL1_OR_EL2_OR_EL3(x0) 1: tlbivaae1, x1 // TLB Invalidate VA , EL1 b 4f 2: tlbivae2, x1 // TLB Invalidate VA , EL2 b 4f 3: tlbivae3, x1 // TLB Invalidate VA , EL3 -4: dsb sy +4: dsb nsh isb ret diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c index d66df3e17a02..e1fabfcbea14 100644 --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c @@ -129,13 +129,14 @@ STATIC VOID ReplaceLiveEntry ( IN UINT64 *Entry, - IN UINT64 Value + IN UINT64 Value, + IN UINT64 Address ) { if (!ArmMmuEnabled ()) { *Entry = Value; } else { -ArmReplaceLiveTranslationEntry (Entry, Value); +ArmReplaceLiveTranslationEntry (Entry, Value, Address); } } @@ -296,7 +297,8 @@ GetBlockEntryListFromAddress ( // Fill the BlockEntry with the new TranslationTable ReplaceLiveEntry (BlockEntry, - ((UINTN)TranslationTable & TT_ADDRESS_MASK_DESCRIPTION_TABLE) | TableAttributes | TT_TYPE_TABLE_ENTRY); + (UINTN)TranslationTable | TableAttributes | TT_TYPE_TABLE_ENTRY, + RegionStart); } } else { if (IndexLevel != PageLevel) { @@ -375,6 +377,8 @@ UpdateRegionMapping ( *BlockEntry &= BlockEntryMask; *BlockEntry |= (RegionStart & TT_ADDRESS_MASK_BLOCK_ENTRY) | Attributes | Type; + ArmUpdateTranslationTableEntry (BlockEntry, (VOID *)RegionStart); + // Go to the next BlockEntry RegionStart += BlockEntrySize; RegionLength -= BlockEntrySize; @@ -487,9 +491,6 @@ ArmSetMemoryAttributes ( return Status; } - // Invalidate all TLB entries so changes are synced - ArmInvalidateTlb (); - return EFI_SUCCESS; } @@ -512,9 +513,6 @@ SetMemoryRegionAttribute ( return Status; } - // Invalidate all TLB entries so changes are synced - ArmInvalidateTlb (); - return EFI_SUCCESS; } diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S index 90192df24f55..d40c19b2e3e5 100644 --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S @@ -32,11 +32,12 @@ dmb sy dcivac, x0 - // flush the TLBs + // flush translations for the target address from the TLBs + lsr x2, x2, #12 .if \el == 1 - tlbi vmalle1 + tlbi vaae1, x2 .else - tlbi alle\el + tlbi vae\el, x2 .endif dsb sy @@ -48,12 +49,13 @@ //VOID //ArmReplaceLiveTranslationEntry ( // IN UINT64 *Entry, -// IN UINT64 Value +// IN UINT64 Value, +// IN UINT64 Address // ) ASM_FUNC(ArmReplaceLiveTranslationEntry) // disable interrupts - mrs x2, daif + mrs x4, daif msr daifset, #0xf isb @@ -69,7 +71,7 @@ ASM_FUNC(ArmReplaceLiveTranslationEntry) b 4f 3:__replace_entry 3 -4:msr daif, x2 +4:msr daif, x4 ret ASM_GLOBAL ASM_PFX(ArmReplaceLiveTranslationEntrySize) -- 2.20.1 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH 0/5] memory/MMU hardening for AArch64
Now that we are getting more serious about implementing secure boot on ARM systems, by putting the code that manipulated the variable store in a secure partition, it makes sense to give some attention to the non-secure side as well, since having secure authenticated variables is moot if we can just nop out the authentication check in the image loader. Patch #1 fixes an issue in ArmMmuLib that is triggered when HeapGuard is enabled. Patch #2 optimizes TLB management so that we don't flush all of it every time. This is a performance optimization as well as a hardening measure, since it makes it more difficult to trigger a flush of all TLBs, which is needed when abusing a write exploit to change memory permissions. Patch #3 is a prerequisite for enabling StackGuard and HeapGuard, which make use of the EFI_MEMORY_RP attribute and this wasn't wired up yet. Patch #4 adds support to ArmMmuLib to remap all page tables read-only, so that they are no longer vulnerable to rogue writes. Patch #5 enables the feature added in #4 at EndOfDxe. Ard Biesheuvel (5): ArmPkg/ArmMmuLib AARCH64: fix out of bounds access ArmPkg/ArmMmuLib AARCH64: get rid of needless TLB invalidation ArmPkg/ArmMmuLib AARCH64: implement support for EFI_MEMORY_RP permissions ArmPkg/ArmMmuLib AARCH64: add support for read-only page tables ArmPkg/CpuDxe: switch to read-only page tables at EndOfDxe ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c | 5 +- ArmPkg/Drivers/CpuDxe/CpuDxe.c | 23 +++ ArmPkg/Drivers/CpuDxe/CpuDxe.inf | 1 + ArmPkg/Include/Library/ArmMmuLib.h | 9 +- ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S| 6 +- ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 149 +--- ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S | 14 +- ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c | 8 ++ 8 files changed, 181 insertions(+), 34 deletions(-) -- 2.20.1 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH 1/5] ArmPkg/ArmMmuLib AARCH64: fix out of bounds access
Take care not to dereference BlockEntry if it may be pointing past the end of the page table we are manipulating. It is only a read, and thus harmless, but HeapGuard triggers on it so let's fix it. Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Ard Biesheuvel --- ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c index e41044142ef4..d66df3e17a02 100644 --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c @@ -382,7 +382,7 @@ UpdateRegionMapping ( // Break the inner loop when next block is a table // Rerun GetBlockEntryListFromAddress to avoid page table memory leak - if (TableLevel != 3 && + if (TableLevel != 3 && BlockEntry <= LastBlockEntry && (*BlockEntry & TT_TYPE_MASK) == TT_TYPE_TABLE_ENTRY) { break; } -- 2.20.1 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH 3/5] ArmPkg/ArmMmuLib AARCH64: implement support for EFI_MEMORY_RP permissions
Wire up the access flag (AF) page table attribute to the EFI_MEMORY_RP permission attribute, so that attempts to read from such a region will trigger an access flag fault. Note that this is a stronger notion than just read protection, since it now implies that any write or execute attempt is trapped as well. However, this does not really matter in practice since we never assume that a read protected page is writable or executable, and StackGuard and HeapGuard (which are the primary users of this facility) certainly don't care. Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Ard Biesheuvel --- ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c | 5 +++-- ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 14 +++--- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c index 3e216c7cb235..e62e3fa87112 100644 --- a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c +++ b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c @@ -223,8 +223,9 @@ EfiAttributeToArmAttribute ( ArmAttributes = TT_ATTR_INDX_MASK; } - // Set the access flag to match the block attributes - ArmAttributes |= TT_AF; + if ((EfiAttributes & EFI_MEMORY_RP) == 0) { +ArmAttributes |= TT_AF; + } // Determine protection attributes if (EfiAttributes & EFI_MEMORY_RO) { diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c index e1fabfcbea14..b59c081a7e49 100644 --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c @@ -102,6 +102,10 @@ PageAttributeToGcdAttribute ( GcdAttributes |= EFI_MEMORY_XP; } + if ((PageAttributes & TT_AF) == 0) { +GcdAttributes |= EFI_MEMORY_RP; + } + return GcdAttributes; } @@ -451,7 +455,11 @@ GcdAttributeToPageAttribute ( PageAttributes |= TT_AP_RO_RO; } - return PageAttributes | TT_AF; + if ((GcdAttributes & EFI_MEMORY_RP) == 0) { +PageAttributes |= TT_AF; + } + + return PageAttributes; } EFI_STATUS @@ -474,9 +482,9 @@ ArmSetMemoryAttributes ( // No memory type was set in Attributes, so we are going to update the // permissions only. // -PageAttributes &= TT_AP_MASK | TT_UXN_MASK | TT_PXN_MASK; +PageAttributes &= TT_AP_MASK | TT_UXN_MASK | TT_PXN_MASK | TT_AF; PageAttributeMask = ~(TT_ADDRESS_MASK_BLOCK_ENTRY | TT_AP_MASK | - TT_PXN_MASK | TT_XN_MASK); + TT_PXN_MASK | TT_XN_MASK | TT_AF); } TranslationTable = ArmGetTTBR0BaseAddress (); -- 2.20.1 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH 5/5] ArmPkg/CpuDxe: switch to read-only page tables at EndOfDxe
Register for the EndOfDxe event, and use it to invoke the new ArmMmuLib code that remaps all page tables as read-only. This should limit the impact of arbitrary write exploits, since they can no longer be abused to modify tightened memory permissions. Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Ard Biesheuvel --- ArmPkg/Drivers/CpuDxe/CpuDxe.c | 23 ArmPkg/Drivers/CpuDxe/CpuDxe.inf | 1 + 2 files changed, 24 insertions(+) diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.c b/ArmPkg/Drivers/CpuDxe/CpuDxe.c index 5e923d45b715..11f4a2ccf5c8 100644 --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.c +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.c @@ -238,6 +238,17 @@ InitializeDma ( CpuArchProtocol->DmaBufferAlignment = ArmCacheWritebackGranule (); } +STATIC +VOID +EFIAPI +OnEndOfDxe ( + IN EFI_EVENTEvent, + IN VOID *Context + ) +{ + MapAllPageTablesReadOnly (); +} + EFI_STATUS CpuDxeInitialize ( IN EFI_HANDLE ImageHandle, @@ -246,6 +257,7 @@ CpuDxeInitialize ( { EFI_STATUS Status; EFI_EVENTIdleLoopEvent; + EFI_EVENTEndOfDxeEvent; InitializeExceptions (&mCpu); @@ -285,5 +297,16 @@ CpuDxeInitialize ( ); ASSERT_EFI_ERROR (Status); + + Status = gBS->CreateEventEx ( + EVT_NOTIFY_SIGNAL, + TPL_CALLBACK, + OnEndOfDxe, + NULL, + &gEfiEndOfDxeEventGroupGuid, + &EndOfDxeEvent + ); + ASSERT_EFI_ERROR (Status); + return Status; } diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.inf b/ArmPkg/Drivers/CpuDxe/CpuDxe.inf index c32d2cb9c7d4..0788a2ab27c0 100644 --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.inf +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.inf @@ -63,6 +63,7 @@ [Guids] gEfiDebugImageInfoTableGuid + gEfiEndOfDxeEventGroupGuid gArmMpCoreInfoGuid gIdleLoopEventGuid gEfiVectorHandoffTableGuid -- 2.20.1 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [Patch v2 1/2] UefiCpuPkg/RegisterCpuFeaturesLib: Enhance debug message.
On 1/7/2019 9:05 AM, Eric Dong wrote: Enhance debug message format to let them easy to read. Cc: Ruiyu Ni Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Eric Dong --- .../RegisterCpuFeaturesLib/CpuFeaturesInitialize.c | 17 +++-- 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c index 0a74d448c8..624ddee055 100644 --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c @@ -473,8 +473,9 @@ DumpRegisterTableOnProcessor ( case Msr: DEBUG (( DebugPrintErrorLevel, -"Processor: %d: MSR: %x, Bit Start: %d, Bit Length: %d, Value: %lx\r\n", +"Processor: %04d: Index %04d, MSR : %08x, Bit Start: %02d, Bit Length: %02d, Value: %016lx\r\n", ProcessorNumber, +FeatureIndex, RegisterTableEntry->Index, RegisterTableEntry->ValidBitStart, RegisterTableEntry->ValidBitLength, @@ -484,8 +485,9 @@ DumpRegisterTableOnProcessor ( case ControlRegister: DEBUG (( DebugPrintErrorLevel, -"Processor: %d:CR: %x, Bit Start: %d, Bit Length: %d, Value: %lx\r\n", +"Processor: %04d: Index %04d, CR : %08x, Bit Start: %02d, Bit Length: %02d, Value: %016lx\r\n", ProcessorNumber, +FeatureIndex, RegisterTableEntry->Index, RegisterTableEntry->ValidBitStart, RegisterTableEntry->ValidBitLength, @@ -495,8 +497,9 @@ DumpRegisterTableOnProcessor ( case MemoryMapped: DEBUG (( DebugPrintErrorLevel, -"Processor: %d: MMIO: %lx, Bit Start: %d, Bit Length: %d, Value: %lx\r\n", +"Processor: %04d: Index %04d, MMIO : %08lx, Bit Start: %02d, Bit Length: %02d, Value: %016lx\r\n", ProcessorNumber, +FeatureIndex, RegisterTableEntry->Index | LShiftU64 (RegisterTableEntry->HighIndex, 32), RegisterTableEntry->ValidBitStart, RegisterTableEntry->ValidBitLength, @@ -506,8 +509,9 @@ DumpRegisterTableOnProcessor ( case CacheControl: DEBUG (( DebugPrintErrorLevel, -"Processor: %d: CACHE: %x, Bit Start: %d, Bit Length: %d, Value: %lx\r\n", +"Processor: %04d: Index %04d, CACHE: %08lx, Bit Start: %02d, Bit Length: %02d, Value: %016lx\r\n", ProcessorNumber, +FeatureIndex, RegisterTableEntry->Index, RegisterTableEntry->ValidBitStart, RegisterTableEntry->ValidBitLength, @@ -517,8 +521,9 @@ DumpRegisterTableOnProcessor ( case Semaphore: DEBUG (( DebugPrintErrorLevel, -"Processor: %d: Semaphore: Scope Value: %s\r\n", +"Processor: %04d: Index %04d, SEMAP: %s\r\n", ProcessorNumber, +FeatureIndex, mDependTypeStr[MIN ((UINT32)RegisterTableEntry->Value, InvalidDepType)] )); break; @@ -833,7 +838,7 @@ ProgramProcessorRegister ( ApLocation->Thread; DEBUG (( DEBUG_INFO, -"Processor = %lu, Entry Index %lu, Type = %s!\n", +"Processor = %08lu, Index %08lu, Type = %s!\n", (UINT64)ThreadIndex, (UINT64)Index, mRegisterTypeStr[MIN ((REGISTER_TYPE)RegisterTableEntry->RegisterType, InvalidReg)] Reviewed-by: Ray Ni -- Thanks, Ray ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel