On 06/02/20 20:17, Bret Barkelew wrote: > Actually, two things: > > 1. I was running on the wrong branch and > 2. I don’t know that I submitted this series to GitHub yet, so the CI > wouldn’t have caught it.
Ah OK -- I thought you had run the v4 series through a personal CI build (= PR without the "push" label) on github.com. (I don't closely monitor my github notifications folder just yet, i.e. before we transition to github.com for good, and so I couldn't have remembered either the presence or the absence of such a personal CI build / PR in the recent past.) Thanks! Laszlo > > I ran it through the Windows CI on my local machine before v4 patches, but VS > is less picky (for better or [probably] worse) about things like EFIAPI and > CONST. > > I have these patched and ready for v5. > > - Bret > > From: Bret Barkelew via > groups.io<mailto:[email protected]> > Sent: Tuesday, June 2, 2020 9:55 AM > To: [email protected]<mailto:[email protected]>; > [email protected]<mailto:[email protected]>; > [email protected]<mailto:[email protected]> > Cc: Jian J Wang<mailto:[email protected]>; Hao A > Wu<mailto:[email protected]>; liming.gao<mailto:[email protected]> > Subject: Re: [EXTERNAL] Re: [edk2-devel] [PATCH v4 09/14] MdeModulePkg: > Connect VariablePolicy business logic to VariableServices > > I’m also puzzled by the lack of error in CI. I just tried to leverage it to > iterate on the problem and resolve the issue you’re seeing, but it wasn’t of > any use. Will poke around a little, but also keen to hear from anyone with > more GCC5 experience. > > - Bret > > From: Laszlo Ersek via groups.io<mailto:[email protected]> > Sent: Tuesday, June 2, 2020 8:48 AM > To: [email protected]<mailto:[email protected]>; > [email protected]<mailto:[email protected]> > Cc: Jian J Wang<mailto:[email protected]>; Hao A > Wu<mailto:[email protected]>; liming.gao<mailto:[email protected]> > Subject: [EXTERNAL] Re: [edk2-devel] [PATCH v4 09/14] MdeModulePkg: Connect > VariablePolicy business logic to VariableServices > > Hi Bret, > > this patch causes a build failure with the GCC48 toolchain (actual > version: gcc 4.8.5-36 in RHEL7), and also with the GCC5 toolchain > (actual version: "Red Hat Cross 6.1.1-2"): > > On 06/01/20 18:33, Bret Barkelew wrote: >> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2522&data=02%7C01%7CBret.Barkelew%40microsoft.com%7C40552c2a4896475ed9da08d8070c6f51%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637267097285334746&sdata=UEd%2FyUeixp5cq2jXwUQ68tIws9pbGBl0m9cpnrijaGc%3D&reserved=0<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2522&data=02%7C01%7Cbret.barkelew%40microsoft.com%7C35bc0a0f5b704d509a4808d80715cd2c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637267137524542472&sdata=JpZVvrk4J3uqv8z5uCcR9DDLXSu0eL4Sm2UtffT9N3U%3D&reserved=0> >> >> VariablePolicy is an updated interface to >> replace VarLock and VarCheckProtocol. >> >> Add connective code to publish the VariablePolicy protocol >> and wire it to either the SMM communication interface >> or directly into the VariablePolicyLib business logic. >> >> Cc: Jian J Wang <[email protected]> >> Cc: Hao A Wu <[email protected]> >> Cc: Liming Gao <[email protected]> >> Cc: Bret Barkelew <[email protected]> >> Signed-off-by: Bret Barkelew <[email protected]> >> --- >> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c | 53 >> ++ >> MdeModulePkg/Universal/Variable/RuntimeDxe/VariablePolicySmmDxe.c | 642 >> ++++++++++++++++++++ >> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c | 14 + >> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf | 2 + >> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf | 3 + >> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.inf | 10 + >> 6 files changed, 724 insertions(+) >> >> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c >> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c >> index 7d2b6c8e1fad..d404d4763e54 100644 >> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c >> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c >> @@ -5,18 +5,34 @@ >> Copyright (C) 2013, Red Hat, Inc. >> Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR> >> (C) Copyright 2015 Hewlett Packard Enterprise Development LP<BR> >> +Copyright (c) Microsoft Corporation. >> SPDX-License-Identifier: BSD-2-Clause-Patent >> >> **/ >> >> #include "Variable.h" >> >> +#include <Protocol/VariablePolicy.h> >> +#include <Library/VariablePolicyLib.h> >> + >> +EFI_STATUS >> +EFIAPI >> +ProtocolIsVariablePolicyEnabled ( >> + OUT BOOLEAN *State >> + ); >> + >> EFI_HANDLE mHandle = NULL; >> EFI_EVENT mVirtualAddressChangeEvent = NULL; >> VOID *mFtwRegistration = NULL; >> VOID ***mVarCheckAddressPointer = NULL; >> UINTN mVarCheckAddressPointerCount = 0; >> EDKII_VARIABLE_LOCK_PROTOCOL mVariableLock = { >> VariableLockRequestToLock }; >> +EDKII_VARIABLE_POLICY_PROTOCOL mVariablePolicyProtocol = { >> EDKII_VARIABLE_POLICY_PROTOCOL_REVISION, >> + >> DisableVariablePolicy, >> + >> ProtocolIsVariablePolicyEnabled, >> + >> RegisterVariablePolicy, > > (1) "error: initialization from incompatible pointer type [-Werror]" > > That's because RegisterVariablePolicy() has the following prototype > (from "MdeModulePkg/Include/Library/VariablePolicyLib.h"): > > EFI_STATUS > EFIAPI > RegisterVariablePolicy ( > IN CONST VARIABLE_POLICY_ENTRY *NewPolicy > ); > > whereas "EDKII_VARIABLE_POLICY_PROTOCOL.RegisterVariablePolicy" has the > following type (from "MdeModulePkg/Include/Protocol/VariablePolicy.h"): > > typedef > EFI_STATUS > (EFIAPI *REGISTER_VARIABLE_POLICY)( > IN VARIABLE_POLICY_ENTRY *PolicyEntry > ); > > The latter does not take a pointer to CONST. > > Now, assuming that "CONST is good", I locally modified the > REGISTER_VARIABLE_POLICY typedef, just to see if that would allow the > build to complete. It fixes problem (1), but it triggers a different > problem: > > >> + >> DumpVariablePolicy, >> + >> LockVariablePolicy }; >> EDKII_VAR_CHECK_PROTOCOL mVarCheck = { >> VarCheckRegisterSetVariableCheckHandler, >> >> VarCheckVariablePropertySet, >> >> VarCheckVariablePropertyGet }; >> @@ -303,6 +319,8 @@ OnReadyToBoot ( >> } >> } >> >> + ASSERT_EFI_ERROR (LockVariablePolicy ()); >> + >> gBS->CloseEvent (Event); >> } >> >> @@ -466,6 +484,28 @@ FtwNotificationEvent ( >> } >> >> >> +/** >> + This API function returns whether or not the policy engine is >> + currently being enforced. >> + >> + @param[out] State Pointer to a return value for whether the >> policy enforcement >> + is currently enabled. >> + >> + @retval EFI_SUCCESS >> + @retval Others An error has prevented this command from >> completing. >> + >> +**/ >> +EFI_STATUS >> +EFIAPI >> +ProtocolIsVariablePolicyEnabled ( >> + OUT BOOLEAN *State >> + ) >> +{ >> + *State = IsVariablePolicyEnabled (); >> + return EFI_SUCCESS; >> +} >> + >> + >> /** >> Variable Driver main entry point. The Variable driver places the 4 EFI >> runtime services in the EFI System Table and installs arch protocols >> @@ -576,6 +616,19 @@ VariableServiceInitialize ( >> ); >> ASSERT_EFI_ERROR (Status); >> >> + // Register and initialize the VariablePolicy engine. >> + Status = InitVariablePolicyLib (VariableServiceGetVariable); >> + ASSERT_EFI_ERROR (Status); >> + Status = VarCheckRegisterSetVariableCheckHandler (ValidateSetVariable); >> + ASSERT_EFI_ERROR (Status); >> + Status = gBS->InstallMultipleProtocolInterfaces ( >> + &mHandle, >> + &gEdkiiVariablePolicyProtocolGuid, >> + &mVariablePolicyProtocol, >> + NULL >> + ); >> + ASSERT_EFI_ERROR (Status); >> + >> return EFI_SUCCESS; >> } >> >> diff --git >> a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariablePolicySmmDxe.c >> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariablePolicySmmDxe.c >> new file mode 100644 >> index 000000000000..3d799025983a >> --- /dev/null >> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariablePolicySmmDxe.c >> @@ -0,0 +1,642 @@ >> +/** @file -- VariablePolicySmmDxe.c >> +This protocol allows communication with Variable Policy Engine. >> + >> +Copyright (c) Microsoft Corporation. >> +SPDX-License-Identifier: BSD-2-Clause-Patent >> + >> +**/ >> + >> +#include <Library/BaseLib.h> >> +#include <Library/UefiLib.h> >> +#include <Library/DebugLib.h> >> +#include <Library/SafeIntLib.h> >> +#include <Library/UefiBootServicesTableLib.h> >> +#include <Library/BaseMemoryLib.h> >> +#include <Library/MemoryAllocationLib.h> >> + >> +#include <Protocol/VariablePolicy.h> >> +#include <Protocol/MmCommunication2.h> >> + >> +#include <Guid/VarCheckPolicyMmi.h> >> + >> +#include "Variable.h" >> + >> +EDKII_VARIABLE_POLICY_PROTOCOL mVariablePolicyProtocol; >> +EFI_MM_COMMUNICATION2_PROTOCOL *mMmCommunication; >> + >> +VOID *mMmCommunicationBuffer; >> +UINTN mMmCommunicationBufferSize; >> +EFI_LOCK mMmCommunicationLock; >> + >> +/** >> + Internal helper function to consolidate communication method. >> + >> + @param[in,out] CommBuffer >> + @param[in,out] CommSize Size of the CommBuffer. >> + >> + @retval EFI_STATUS Result from communication method. >> + >> +**/ >> +STATIC >> +EFI_STATUS >> +InternalMmCommunicate ( >> + IN OUT VOID *CommBuffer, >> + IN OUT UINTN *CommSize >> + ) >> +{ >> + EFI_STATUS Status; >> + if (CommBuffer == NULL || CommSize == NULL) { >> + return EFI_INVALID_PARAMETER; >> + } >> + Status = mMmCommunication->Communicate (mMmCommunication, CommBuffer, >> CommBuffer, CommSize); >> + return Status; >> +} >> + >> + >> +/** >> + This API function disables the variable policy enforcement. If it's >> + already been called once, will return EFI_ALREADY_STARTED. >> + >> + @retval EFI_SUCCESS >> + @retval EFI_ALREADY_STARTED Has already been called once this boot. >> + @retval EFI_WRITE_PROTECTED Interface has been locked until reboot. >> + @retval EFI_WRITE_PROTECTED Interface option is disabled by >> platform PCD. >> + >> +**/ >> +STATIC >> +EFI_STATUS >> +EFIAPI >> +ProtocolDisableVariablePolicy ( >> + VOID >> + ) >> +{ >> + EFI_STATUS Status; >> + EFI_MM_COMMUNICATE_HEADER *CommHeader; >> + VAR_CHECK_POLICY_COMM_HEADER *PolicyHeader; >> + UINTN BufferSize; >> + >> + // Check the PCD for convenience. >> + // This would also be rejected by the lib, but why go to MM if we don't >> have to? >> + if (!PcdGetBool (PcdAllowVariablePolicyEnforcementDisable)) { >> + return EFI_WRITE_PROTECTED; >> + } >> + >> + AcquireLockOnlyAtBootTime (&mMmCommunicationLock); >> + >> + // Set up the MM communication. >> + BufferSize = mMmCommunicationBufferSize; >> + CommHeader = mMmCommunicationBuffer; >> + PolicyHeader = (VAR_CHECK_POLICY_COMM_HEADER*)&CommHeader->Data; >> + CopyGuid( &CommHeader->HeaderGuid, &gVarCheckPolicyLibMmiHandlerGuid ); >> + CommHeader->MessageLength = BufferSize; >> + PolicyHeader->Signature = VAR_CHECK_POLICY_COMM_SIG; >> + PolicyHeader->Revision = VAR_CHECK_POLICY_COMM_REVISION; >> + PolicyHeader->Command = VAR_CHECK_POLICY_COMMAND_DISABLE; >> + >> + Status = InternalMmCommunicate (CommHeader, &BufferSize); >> + DEBUG(( DEBUG_VERBOSE, "%a - MmCommunication returned %r.\n", >> __FUNCTION__, Status )); >> + >> + ReleaseLockOnlyAtBootTime (&mMmCommunicationLock); >> + >> + return (EFI_ERROR( Status )) ? Status : PolicyHeader->Result; >> +} >> + >> + >> +/** >> + This API function returns whether or not the policy engine is >> + currently being enforced. >> + >> + @param[out] State Pointer to a return value for whether the >> policy enforcement >> + is currently enabled. >> + >> + @retval EFI_SUCCESS >> + @retval Others An error has prevented this command from >> completing. >> + >> +**/ >> +STATIC >> +EFI_STATUS >> +EFIAPI >> +ProtocolIsVariablePolicyEnabled ( >> + OUT BOOLEAN *State >> + ) >> +{ >> + EFI_STATUS Status; >> + EFI_MM_COMMUNICATE_HEADER *CommHeader; >> + VAR_CHECK_POLICY_COMM_HEADER *PolicyHeader; >> + VAR_CHECK_POLICY_COMM_IS_ENABLED_PARAMS *CommandParams; >> + UINTN BufferSize; >> + >> + if (State == NULL) { >> + return EFI_INVALID_PARAMETER; >> + } >> + >> + AcquireLockOnlyAtBootTime (&mMmCommunicationLock); >> + >> + // Set up the MM communication. >> + BufferSize = mMmCommunicationBufferSize; >> + CommHeader = mMmCommunicationBuffer; >> + PolicyHeader = (VAR_CHECK_POLICY_COMM_HEADER*)&CommHeader->Data; >> + CommandParams = (VAR_CHECK_POLICY_COMM_IS_ENABLED_PARAMS*)(PolicyHeader + >> 1); >> + CopyGuid( &CommHeader->HeaderGuid, &gVarCheckPolicyLibMmiHandlerGuid ); >> + CommHeader->MessageLength = BufferSize; >> + PolicyHeader->Signature = VAR_CHECK_POLICY_COMM_SIG; >> + PolicyHeader->Revision = VAR_CHECK_POLICY_COMM_REVISION; >> + PolicyHeader->Command = VAR_CHECK_POLICY_COMMAND_IS_ENABLED; >> + >> + Status = InternalMmCommunicate (CommHeader, &BufferSize); >> + DEBUG(( DEBUG_VERBOSE, "%a - MmCommunication returned %r.\n", >> __FUNCTION__, Status )); >> + >> + if (!EFI_ERROR( Status )) { >> + Status = PolicyHeader->Result; >> + *State = CommandParams->State; >> + } >> + >> + ReleaseLockOnlyAtBootTime (&mMmCommunicationLock); >> + >> + return Status; >> +} >> + >> + >> +/** >> + This API function validates and registers a new policy with >> + the policy enforcement engine. >> + >> + @param[in] NewPolicy Pointer to the incoming policy structure. >> + >> + @retval EFI_SUCCESS >> + @retval EFI_INVALID_PARAMETER NewPolicy is NULL or is internally >> inconsistent. >> + @retval EFI_ALREADY_STARTED An identical matching policy already >> exists. >> + @retval EFI_WRITE_PROTECTED The interface has been locked until >> the next reboot. >> + @retval EFI_UNSUPPORTED Policy enforcement has been disabled. >> No reason to add more policies. >> + @retval EFI_ABORTED A calculation error has prevented >> this function from completing. >> + @retval EFI_OUT_OF_RESOURCES Cannot grow the table to hold any >> more policies. >> + >> +**/ >> +STATIC >> +EFI_STATUS >> +EFIAPI >> +ProtocolRegisterVariablePolicy ( >> + IN VARIABLE_POLICY_ENTRY *NewPolicy >> + ) >> +{ >> + EFI_STATUS Status; >> + EFI_MM_COMMUNICATE_HEADER *CommHeader; >> + VAR_CHECK_POLICY_COMM_HEADER *PolicyHeader; >> + VOID *PolicyBuffer; >> + UINTN BufferSize; >> + UINTN RequiredSize; >> + >> + if (NewPolicy == NULL) { >> + return EFI_INVALID_PARAMETER; >> + } >> + >> + // First, make sure that the required size does not exceed the >> capabilities >> + // of the MmCommunication buffer. >> + RequiredSize = OFFSET_OF(EFI_MM_COMMUNICATE_HEADER, Data) + >> sizeof(VAR_CHECK_POLICY_COMM_HEADER); >> + Status = SafeUintnAdd( RequiredSize, NewPolicy->Size, &RequiredSize ); >> + if (EFI_ERROR( Status ) || RequiredSize > mMmCommunicationBufferSize) { >> + DEBUG(( DEBUG_ERROR, "%a - Policy too large for buffer! %r, %d > %d >> \n", __FUNCTION__, >> + Status, RequiredSize, mMmCommunicationBufferSize )); >> + return EFI_OUT_OF_RESOURCES; >> + } >> + >> + AcquireLockOnlyAtBootTime (&mMmCommunicationLock); >> + >> + // Set up the MM communication. >> + BufferSize = mMmCommunicationBufferSize; >> + CommHeader = mMmCommunicationBuffer; >> + PolicyHeader = (VAR_CHECK_POLICY_COMM_HEADER*)&CommHeader->Data; >> + PolicyBuffer = (VOID*)(PolicyHeader + 1); >> + CopyGuid( &CommHeader->HeaderGuid, &gVarCheckPolicyLibMmiHandlerGuid ); >> + CommHeader->MessageLength = BufferSize; >> + PolicyHeader->Signature = VAR_CHECK_POLICY_COMM_SIG; >> + PolicyHeader->Revision = VAR_CHECK_POLICY_COMM_REVISION; >> + PolicyHeader->Command = VAR_CHECK_POLICY_COMMAND_REGISTER; >> + >> + // Copy the policy into place. This copy is safe because we've already >> tested above. >> + CopyMem( PolicyBuffer, NewPolicy, NewPolicy->Size ); >> + >> + Status = InternalMmCommunicate (CommHeader, &BufferSize); >> + DEBUG(( DEBUG_VERBOSE, "%a - MmCommunication returned %r.\n", >> __FUNCTION__, Status )); >> + >> + ReleaseLockOnlyAtBootTime (&mMmCommunicationLock); >> + >> + return (EFI_ERROR( Status )) ? Status : PolicyHeader->Result; >> +} >> + >> + >> +/** >> + This helper function takes care of the overhead of formatting, sending, >> and interpreting >> + the results for a single DumpVariablePolicy request. >> + >> + @param[in] PageRequested The page of the paginated results from >> MM. 0 for metadata. >> + @param[out] TotalSize The total size of the entire buffer. >> Returned as part of metadata. >> + @param[out] PageSize The size of the current page being >> returned. Not valid as part of metadata. >> + @param[out] HasMore A flag indicating whether there are more >> pages after this one. >> + @param[out] Buffer The start of the current page from MM. >> + >> + @retval EFI_SUCCESS Output params have been updated >> (either metadata or dump page). >> + @retval EFI_INVALID_PARAMETER One of the output params is NULL. >> + @retval Others Response from MM handler. >> + >> +**/ >> +STATIC >> +EFI_STATUS >> +DumpVariablePolicyHelper ( >> + IN UINT32 PageRequested, >> + OUT UINT32 *TotalSize, >> + OUT UINT32 *PageSize, >> + OUT BOOLEAN *HasMore, >> + OUT UINT8 **Buffer >> + ) >> +{ >> + EFI_STATUS Status; >> + EFI_MM_COMMUNICATE_HEADER *CommHeader; >> + VAR_CHECK_POLICY_COMM_HEADER *PolicyHeader; >> + VAR_CHECK_POLICY_COMM_DUMP_PARAMS *CommandParams; >> + UINTN BufferSize; >> + >> + if (TotalSize == NULL || PageSize == NULL || HasMore == NULL || Buffer == >> NULL) { >> + return EFI_INVALID_PARAMETER; >> + } >> + >> + // Set up the MM communication. >> + BufferSize = mMmCommunicationBufferSize; >> + CommHeader = mMmCommunicationBuffer; >> + PolicyHeader = (VAR_CHECK_POLICY_COMM_HEADER*)&CommHeader->Data; >> + CommandParams = (VAR_CHECK_POLICY_COMM_DUMP_PARAMS*)(PolicyHeader + 1); >> + CopyGuid( &CommHeader->HeaderGuid, &gVarCheckPolicyLibMmiHandlerGuid ); >> + CommHeader->MessageLength = BufferSize; >> + PolicyHeader->Signature = VAR_CHECK_POLICY_COMM_SIG; >> + PolicyHeader->Revision = VAR_CHECK_POLICY_COMM_REVISION; >> + PolicyHeader->Command = VAR_CHECK_POLICY_COMMAND_DUMP; >> + >> + CommandParams->PageRequested = PageRequested; >> + >> + Status = InternalMmCommunicate (CommHeader, &BufferSize); >> + DEBUG(( DEBUG_VERBOSE, "%a - MmCommunication returned %r.\n", >> __FUNCTION__, Status )); >> + >> + if (!EFI_ERROR( Status )) { >> + Status = PolicyHeader->Result; >> + *TotalSize = CommandParams->TotalSize; >> + *PageSize = CommandParams->PageSize; >> + *HasMore = CommandParams->HasMore; >> + *Buffer = (UINT8*)(CommandParams + 1); >> + } >> + >> + return Status; >> +} >> + >> + >> +/** >> + This API function will dump the entire contents of the variable policy >> table. >> + >> + Similar to GetVariable, the first call can be made with a 0 size and it >> will return >> + the size of the buffer required to hold the entire table. >> + >> + @param[out] Policy Pointer to the policy buffer. Can be NULL if Size >> is 0. >> + @param[in,out] Size On input, the size of the output buffer. On >> output, the size >> + of the data returned. >> + >> + @retval EFI_SUCCESS Policy data is in the output buffer >> and Size has been updated. >> + @retval EFI_INVALID_PARAMETER Size is NULL, or Size is non-zero and >> Policy is NULL. >> + @retval EFI_BUFFER_TOO_SMALL Size is insufficient to hold policy. >> Size updated with required size. >> + >> +**/ >> +STATIC >> +EFI_STATUS >> +EFIAPI >> +ProtocolDumpVariablePolicy ( >> + OUT UINT8 *Policy OPTIONAL, >> + IN OUT UINT32 *Size >> + ) >> +{ >> + EFI_STATUS Status; >> + UINT8 *Source; >> + UINT8 *Destination; >> + UINT32 PolicySize; >> + UINT32 PageSize; >> + BOOLEAN HasMore; >> + UINT32 PageIndex; >> + >> + if (Size == NULL || (*Size > 0 && Policy == NULL)) { >> + return EFI_INVALID_PARAMETER; >> + } >> + >> + AcquireLockOnlyAtBootTime (&mMmCommunicationLock); >> + >> + // Repeat this whole process until we either have a failure case or get >> the entire buffer. >> + do { >> + // First, we must check the zero page to determine the buffer size and >> + // reset the internal state. >> + PolicySize = 0; >> + PageSize = 0; >> + HasMore = FALSE; >> + Status = DumpVariablePolicyHelper (0, &PolicySize, &PageSize, &HasMore, >> &Source); >> + if (EFI_ERROR (Status)) { >> + break; >> + } >> + >> + // If we're good, we can at least check the required size now. >> + if (*Size < PolicySize) { >> + *Size = PolicySize; >> + Status = EFI_BUFFER_TOO_SMALL; >> + break; >> + } >> + >> + // On further thought, let's update the size either way. >> + *Size = PolicySize; >> + // And get ready to ROCK. >> + Destination = Policy; >> + >> + // Keep looping and copying until we're either done or freak out. >> + for (PageIndex = 1; !EFI_ERROR (Status) && HasMore && PageIndex < >> MAX_UINT32; PageIndex++) { >> + Status = DumpVariablePolicyHelper (PageIndex, &PolicySize, &PageSize, >> &HasMore, &Source); >> + if (!EFI_ERROR (Status)) { >> + CopyMem (Destination, Source, PageSize); >> + Destination += PageSize; >> + } >> + } >> + >> + // Next, we check to see whether >> + } while (Status == EFI_TIMEOUT); >> + >> + ReleaseLockOnlyAtBootTime (&mMmCommunicationLock); >> + >> + // There's currently no use for this, but it shouldn't be hard to >> implement. >> + return Status; >> +} >> + >> + >> +/** >> + This API function locks the interface so that no more policy updates >> + can be performed or changes made to the enforcement until the next boot. >> + >> + @retval EFI_SUCCESS >> + @retval Others An error has prevented this command from >> completing. >> + >> +**/ >> +STATIC >> +EFI_STATUS >> +EFIAPI >> +ProtocolLockVariablePolicy ( >> + VOID >> + ) >> +{ >> + EFI_STATUS Status; >> + EFI_MM_COMMUNICATE_HEADER *CommHeader; >> + VAR_CHECK_POLICY_COMM_HEADER *PolicyHeader; >> + UINTN BufferSize; >> + >> + AcquireLockOnlyAtBootTime (&mMmCommunicationLock); >> + >> + // Set up the MM communication. >> + BufferSize = mMmCommunicationBufferSize; >> + CommHeader = mMmCommunicationBuffer; >> + PolicyHeader = (VAR_CHECK_POLICY_COMM_HEADER*)&CommHeader->Data; >> + CopyGuid( &CommHeader->HeaderGuid, &gVarCheckPolicyLibMmiHandlerGuid ); >> + CommHeader->MessageLength = BufferSize; >> + PolicyHeader->Signature = VAR_CHECK_POLICY_COMM_SIG; >> + PolicyHeader->Revision = VAR_CHECK_POLICY_COMM_REVISION; >> + PolicyHeader->Command = VAR_CHECK_POLICY_COMMAND_LOCK; >> + >> + Status = InternalMmCommunicate (CommHeader, &BufferSize); >> + DEBUG(( DEBUG_VERBOSE, "%a - MmCommunication returned %r.\n", >> __FUNCTION__, Status )); >> + >> + ReleaseLockOnlyAtBootTime (&mMmCommunicationLock); >> + >> + return (EFI_ERROR( Status )) ? Status : PolicyHeader->Result; >> +} >> + >> + >> +/** >> + This helper function locates the shared comm buffer and assigns it to >> input pointers. >> + >> + @param[in,out] BufferSize On input, the minimum buffer size >> required INCLUDING the MM communicate header. >> + On output, the size of the matching >> buffer found. >> + @param[out] LocatedBuffer A pointer to the matching buffer. >> + >> + @retval EFI_SUCCESS >> + @retval EFI_INVALID_PARAMETER One of the output pointers was NULL. >> + @retval EFI_OUT_OF_RESOURCES Not enough memory to allocate a comm >> buffer. >> + >> +**/ >> +STATIC >> +EFI_STATUS >> +InitMmCommonCommBuffer ( >> + IN OUT UINTN *BufferSize, >> + OUT VOID **LocatedBuffer >> + ) >> +{ >> + EFI_STATUS Status; >> + >> + Status = EFI_SUCCESS; >> + >> + // Make sure that we're working with good pointers. >> + if (BufferSize == NULL || LocatedBuffer == NULL) { >> + return EFI_INVALID_PARAMETER; >> + } >> + >> + // Allocate the runtime memory for the comm buffer. >> + *LocatedBuffer = AllocateRuntimePool (*BufferSize); >> + if (*LocatedBuffer == NULL) { >> + Status = EFI_OUT_OF_RESOURCES; >> + *BufferSize = 0; >> + } >> + >> + EfiInitializeLock (&mMmCommunicationLock, TPL_NOTIFY); >> + >> + return Status; >> +} >> + >> + >> +/** >> + This helper is responsible for telemetry and any other actions that >> + need to be taken if the VariablePolicy fails to lock. >> + >> + NOTE: It's possible that parts of this handling will need to become >> + part of a platform policy. >> + >> + @param[in] FailureStatus The failure that was reported by >> LockVariablePolicy >> + >> +**/ >> +STATIC >> +VOID >> +VariablePolicyHandleFailureToLock ( >> + IN EFI_STATUS FailureStatus >> + ) >> +{ >> + // For now, there's no agreed-upon policy for this. >> + return; >> +} >> + >> + >> +/** >> + ReadyToBoot Callback >> + Lock the VariablePolicy interface if it hasn't already been locked. >> + >> + @param[in] Event Event whose notification function is being invoked >> + @param[in] Context Pointer to the notification function's context >> + >> +**/ >> +STATIC >> +VOID >> +EFIAPI >> +LockPolicyInterfaceAtReadyToBoot ( >> + IN EFI_EVENT Event, >> + IN VOID *Context >> + ) >> +{ >> + EFI_STATUS Status; >> + >> + Status = ProtocolLockVariablePolicy(); >> + >> + if (EFI_ERROR( Status )) { >> + VariablePolicyHandleFailureToLock( Status ); >> + } >> + else { >> + gBS->CloseEvent( Event ); >> + } >> + >> +} >> + >> + >> +/** >> + Convert internal pointer addresses to virtual addresses. >> + >> + @param[in] Event Event whose notification function is being invoked. >> + @param[in] Context The pointer to the notification function's context, >> which >> + is implementation-dependent. >> +**/ >> +STATIC >> +VOID >> +EFIAPI >> +VariablePolicyVirtualAddressCallback ( >> + IN EFI_EVENT Event, >> + IN VOID *Context >> + ) >> +{ >> + EfiConvertPointer (0, (VOID **)&mMmCommunication); >> + EfiConvertPointer (0, (VOID **)&mMmCommunicationBuffer); >> +} >> + >> + >> +/** >> + The driver's entry point. >> + >> + @param[in] ImageHandle The firmware allocated handle for the EFI image. >> + @param[in] SystemTable A pointer to the EFI System Table. >> + >> + @retval EFI_SUCCESS The entry point executed successfully. >> + @retval other Some error occured when executing this entry >> point. >> + >> +**/ >> +EFI_STATUS >> +EFIAPI >> +VariablePolicySmmDxeMain ( >> + IN EFI_HANDLE ImageHandle, >> + IN EFI_SYSTEM_TABLE *SystemTable >> + ) >> +{ >> + EFI_STATUS Status; >> + BOOLEAN ProtocolInstalled; >> + BOOLEAN CallbackRegistered; >> + BOOLEAN VirtualAddressChangeRegistered; >> + EFI_EVENT ReadyToBootEvent; >> + EFI_EVENT VirtualAddressChangeEvent; >> + >> + Status = EFI_SUCCESS; >> + ProtocolInstalled = FALSE; >> + CallbackRegistered = FALSE; >> + VirtualAddressChangeRegistered = FALSE; >> + >> + // Update the minimum buffer size. >> + mMmCommunicationBufferSize = VAR_CHECK_POLICY_MM_COMM_BUFFER_SIZE; >> + // Locate the shared comm buffer to use for sending MM commands. >> + Status = InitMmCommonCommBuffer( &mMmCommunicationBufferSize, >> &mMmCommunicationBuffer ); >> + if (EFI_ERROR( Status )) { >> + DEBUG((DEBUG_ERROR, "%a - Failed to locate a viable MM comm buffer! >> %r\n", __FUNCTION__, Status)); >> + ASSERT_EFI_ERROR( Status ); >> + return Status; >> + } >> + >> + // Locate the MmCommunication protocol. >> + Status = gBS->LocateProtocol( &gEfiMmCommunication2ProtocolGuid, NULL, >> (VOID**)&mMmCommunication ); >> + if (EFI_ERROR( Status )) { >> + DEBUG((DEBUG_ERROR, "%a - Failed to locate MmCommunication protocol! >> %r\n", __FUNCTION__, Status)); >> + ASSERT_EFI_ERROR( Status ); >> + return Status; >> + } >> + >> + // Configure the VariablePolicy protocol structure. >> + mVariablePolicyProtocol.Revision = >> EDKII_VARIABLE_POLICY_PROTOCOL_REVISION; >> + mVariablePolicyProtocol.DisableVariablePolicy = >> ProtocolDisableVariablePolicy; >> + mVariablePolicyProtocol.IsVariablePolicyEnabled = >> ProtocolIsVariablePolicyEnabled; >> + mVariablePolicyProtocol.RegisterVariablePolicy = >> ProtocolRegisterVariablePolicy; > > (2) "error: assignment from incompatible pointer type [-Werror]" > > Because, with my local modification for (1), the LHS now takes a > pointer-to-CONST, but the RHS is declared (in the present patch) as > > STATIC > EFI_STATUS > EFIAPI > ProtocolRegisterVariablePolicy ( > IN VARIABLE_POLICY_ENTRY *NewPolicy > ) > > So I modified this too, to take a pointer-to-CONST. That allowed the > builds to complete. > > Therefore I suggest one of the following: > > (a) If CONST is not important in the RegisterVariablePolicy() lib class > API, then please remove CONST from there (and the implementation(s) of > that function). > > (b) Alternatively, if CONST is preferred in the library, then > > (b1) please squash the following hunk: > >> diff --git a/MdeModulePkg/Include/Protocol/VariablePolicy.h >> b/MdeModulePkg/Include/Protocol/VariablePolicy.h >> index 30d6c155ae6a..83b6a999df07 100644 >> --- a/MdeModulePkg/Include/Protocol/VariablePolicy.h >> +++ b/MdeModulePkg/Include/Protocol/VariablePolicy.h >> @@ -102,7 +102,7 @@ EFI_STATUS >> typedef >> EFI_STATUS >> (EFIAPI *REGISTER_VARIABLE_POLICY)( >> - IN VARIABLE_POLICY_ENTRY *PolicyEntry >> + IN CONST VARIABLE_POLICY_ENTRY *PolicyEntry >> ); >> >> /** > > into patch "MdeModulePkg: Define the VariablePolicy protocol interface". > > (b2) And please squash the following hunk: > >> diff --git >> a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariablePolicySmmDxe.c >> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariablePolicySmmDxe.c >> index 3d799025983a..e2d4cf4cec1a 100644 >> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariablePolicySmmDxe.c >> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariablePolicySmmDxe.c >> @@ -176,7 +176,7 @@ STATIC >> EFI_STATUS >> EFIAPI >> ProtocolRegisterVariablePolicy ( >> - IN VARIABLE_POLICY_ENTRY *NewPolicy >> + IN CONST VARIABLE_POLICY_ENTRY *NewPolicy >> ) >> { >> EFI_STATUS Status; > > into the present patch. > > I've used (b1)+(b2) locally, and those together allow the builds to > complete. > > > Now, I wonder why the GCC / OVMF build(s) in the github.com CI did not > report this problem. According to the C99 standard, "6.7.5.3 Function > declarators (including prototypes)", paragraph 15: > > For two function types to be compatible, both shall specify compatible > return types. [...] Moreover, the parameter type lists, if both are > present, shall agree in the number of parameters and in use of the > ellipsis terminator; corresponding parameters shall have compatible > types. [...] > > Furthermore, from "6.7.5.1 Pointer declarators", paragraph 2: > > For two pointer types to be compatible, both shall be identically > qualified and both shall be pointers to compatible types. > > Yet further, from "6.5.16.1 Simple assignment", paragraph 1: > > One of the following shall hold: > > [...] > > - both operands are pointers to qualified or unqualified versions of > compatible types, and the type pointed to by the left has all the > qualifiers of the type pointed to by the right; > > [...] > > So the thought process is: > > - "mVariablePolicyProtocol.RegisterVariablePolicy" has type > > EFI_STATUS (EFIAPI *)( VARIABLE_POLICY_ENTRY *) > > while a pointer to RegisterVariablePolicy(), from the library, has > type > > EFI_STATUS (EFIAPI *)(const VARIABLE_POLICY_ENTRY *) > > For the assignment, these need to be compatible, per 6.5.16.1p1. > > Are they compatible? > > - Per 6.7.5.1p2, that depends on whether the pointed-to function types > are compatible. > > Are the pointed-to function types compatible? > > - Per 6.7.5.3p15, that depends on whether (VARIABLE_POLICY_ENTRY *) and > (const VARIABLE_POLICY_ENTRY *) are compatible. > > Are those pointer types compatible? > > - They're not, based on 6.7.5.1p2 -- they are not identically qualified. > > So IMO (according to the language standard), the code should have been > flagged by MSVC too; but more importantly, I don't understand why the > GCC5 / Ubuntu builds succeeded in CI. > > Thanks! > Laszlo > > >> + mVariablePolicyProtocol.DumpVariablePolicy = >> ProtocolDumpVariablePolicy; >> + mVariablePolicyProtocol.LockVariablePolicy = >> ProtocolLockVariablePolicy; >> + >> + // Register all the protocols and return the status. >> + Status = gBS->InstallMultipleProtocolInterfaces( &ImageHandle, >> + >> &gEdkiiVariablePolicyProtocolGuid, &mVariablePolicyProtocol, >> + NULL ); >> + if (EFI_ERROR( Status )) { >> + DEBUG(( DEBUG_ERROR, "%a - Failed to install protocol! %r\n", >> __FUNCTION__, Status )); >> + goto Exit; >> + } >> + else { >> + ProtocolInstalled = TRUE; >> + } >> + >> + // >> + // Register a callback for ReadyToBoot so that the interface is at least >> locked before >> + // dispatching any bootloaders or UEFI apps. >> + Status = gBS->CreateEventEx( EVT_NOTIFY_SIGNAL, >> + TPL_CALLBACK, >> + LockPolicyInterfaceAtReadyToBoot, >> + NULL, >> + &gEfiEventReadyToBootGuid, >> + &ReadyToBootEvent ); >> + if (EFI_ERROR( Status )) { >> + DEBUG(( DEBUG_ERROR, "%a - Failed to create ReadyToBoot event! %r\n", >> __FUNCTION__, Status )); >> + goto Exit; >> + } >> + else { >> + CallbackRegistered = TRUE; >> + } >> + >> + // >> + // Register a VirtualAddressChange callback for the MmComm protocol and >> Comm buffer. >> + Status = gBS->CreateEventEx (EVT_NOTIFY_SIGNAL, >> + TPL_NOTIFY, >> + VariablePolicyVirtualAddressCallback, >> + NULL, >> + &gEfiEventVirtualAddressChangeGuid, >> + &VirtualAddressChangeEvent); >> + if (EFI_ERROR( Status )) { >> + DEBUG(( DEBUG_ERROR, "%a - Failed to create VirtualAddressChange event! >> %r\n", __FUNCTION__, Status )); >> + goto Exit; >> + } >> + else { >> + VirtualAddressChangeRegistered = TRUE; >> + } >> + >> + >> +Exit: >> + // >> + // If we're about to return a failed status (and unload this driver), we >> must first undo anything that >> + // has been successfully done. >> + if (EFI_ERROR( Status )) { >> + if (ProtocolInstalled) { >> + gBS->UninstallProtocolInterface( &ImageHandle, >> &gEdkiiVariablePolicyProtocolGuid, &mVariablePolicyProtocol ); >> + } >> + if (CallbackRegistered) { >> + gBS->CloseEvent( ReadyToBootEvent ); >> + } >> + if (VirtualAddressChangeRegistered) { >> + gBS->CloseEvent( VirtualAddressChangeEvent ); >> + } >> + } >> + >> + return Status; >> +} >> diff --git >> a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c >> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c >> index 663a1aaa128f..c47e614d81f4 100644 >> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c >> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c >> @@ -65,6 +65,17 @@ EFI_LOCK mVariableServicesLock; >> EDKII_VARIABLE_LOCK_PROTOCOL mVariableLock; >> EDKII_VAR_CHECK_PROTOCOL mVarCheck; >> >> +/** >> + The logic to initialize the VariablePolicy engine is in its own file. >> + >> +**/ >> +EFI_STATUS >> +EFIAPI >> +VariablePolicySmmDxeMain ( >> + IN EFI_HANDLE ImageHandle, >> + IN EFI_SYSTEM_TABLE *SystemTable >> + ); >> + >> /** >> Some Secure Boot Policy Variable may update following other variable >> changes(SecureBoot follows PK change, etc). >> Record their initial State when variable write service is ready. >> @@ -1796,6 +1807,9 @@ VariableSmmRuntimeInitialize ( >> &mVirtualAddressChangeEvent >> ); >> >> + // Initialize the VariablePolicy protocol and engine. >> + VariablePolicySmmDxeMain (ImageHandle, SystemTable); >> + >> return EFI_SUCCESS; >> } >> >> diff --git >> a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf >> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf >> index ceea5d1ff9ac..48ac167906f7 100644 >> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf >> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf >> @@ -10,6 +10,7 @@ >> # buffer overflow or integer overflow. >> # >> # Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR> >> +# Copyright (c) Microsoft Corporation. >> # SPDX-License-Identifier: BSD-2-Clause-Patent >> # >> ## >> @@ -69,6 +70,7 @@ [LibraryClasses] >> TpmMeasurementLib >> AuthVariableLib >> VarCheckLib >> + VariablePolicyLib >> >> [Protocols] >> gEfiFirmwareVolumeBlockProtocolGuid ## CONSUMES >> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf >> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf >> index bc3033588d40..bbc8d2080193 100644 >> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf >> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf >> @@ -19,6 +19,7 @@ >> # the authentication service provided in this driver will be broken, and >> the behavior is undefined. >> # >> # Copyright (c) 2010 - 2019, Intel Corporation. All rights reserved.<BR> >> +# Copyright (c) Microsoft Corporation. >> # SPDX-License-Identifier: BSD-2-Clause-Patent >> # >> ## >> @@ -78,6 +79,8 @@ [LibraryClasses] >> AuthVariableLib >> VarCheckLib >> UefiBootServicesTableLib >> + VariablePolicyLib >> + VariablePolicyHelperLib >> >> [Protocols] >> gEfiSmmFirmwareVolumeBlockProtocolGuid ## CONSUMES >> diff --git >> a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.inf >> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.inf >> index 01564e4c5068..f217530b2985 100644 >> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.inf >> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.inf >> @@ -14,6 +14,7 @@ >> # the authentication service provided in this driver will be broken, and >> the behavior is undefined. >> # >> # Copyright (c) 2010 - 2019, Intel Corporation. All rights reserved.<BR> >> +# Copyright (c) Microsoft Corporation.<BR> >> # SPDX-License-Identifier: BSD-2-Clause-Patent >> # >> ## >> @@ -42,6 +43,7 @@ [Sources] >> VariableParsing.c >> VariableParsing.h >> Variable.h >> + VariablePolicySmmDxe.c >> >> [Packages] >> MdePkg/MdePkg.dec >> @@ -56,6 +58,8 @@ [LibraryClasses] >> DxeServicesTableLib >> UefiDriverEntryPoint >> TpmMeasurementLib >> + SafeIntLib >> + PcdLib >> >> [Protocols] >> gEfiVariableWriteArchProtocolGuid ## PRODUCES >> @@ -67,11 +71,15 @@ [Protocols] >> gEfiSmmVariableProtocolGuid >> gEdkiiVariableLockProtocolGuid ## PRODUCES >> gEdkiiVarCheckProtocolGuid ## PRODUCES >> + gEdkiiVariablePolicyProtocolGuid ## PRODUCES >> >> [FeaturePcd] >> gEfiMdeModulePkgTokenSpaceGuid.PcdEnableVariableRuntimeCache ## >> CONSUMES >> gEfiMdeModulePkgTokenSpaceGuid.PcdVariableCollectStatistics ## >> CONSUMES >> >> +[Pcd] >> + gEfiMdeModulePkgTokenSpaceGuid.PcdAllowVariablePolicyEnforcementDisable >> ## CONSUMES >> + >> [Guids] >> ## PRODUCES ## GUID # Signature of Variable store header >> ## CONSUMES ## GUID # Signature of Variable store header >> @@ -99,6 +107,8 @@ [Guids] >> ## SOMETIMES_CONSUMES ## Variable:L"dbt" >> gEfiImageSecurityDatabaseGuid >> >> + gVarCheckPolicyLibMmiHandlerGuid >> + >> [Depex] >> gEfiMmCommunication2ProtocolGuid >> >> > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#60607): https://edk2.groups.io/g/devel/message/60607 Mute This Topic: https://groups.io/mt/74633930/21656 Group Owner: [email protected] Unsubscribe: https://edk2.groups.io/g/devel/unsub [[email protected]] -=-=-=-=-=-=-=-=-=-=-=-
