Re: [EXTERNAL] Re: [edk2-devel] [PATCH v2 01/12] MdeModulePkg: Define the VariablePolicy protocol interface

["Laszlo Ersek" <lersek@redhat.com>]

On 05/13/20 06:51, Bret Barkelew wrote:
> I don’t entirely disagree with the name suggestion, but it’s pretty late in the process. If it’s not a hard-stop, I’d rather not.

I'll let the MdeModulePkg maintainers determine the severity.

> Other change has been made.

Thanks
Laszlo

> From: Laszlo Ersek via groups.io<mailto:lersek=redhat.com@groups.io>
> Sent: Tuesday, May 12, 2020 5:19 AM
> To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; michael.kubacki@outlook.com<mailto:michael.kubacki@outlook.com>
> Cc: Jian J Wang<mailto:jian.j.wang@intel.com>; Hao A Wu<mailto:hao.a.wu@intel.com>; liming.gao<mailto:liming.gao@intel.com>
> Subject: [EXTERNAL] Re: [edk2-devel] [PATCH v2 01/12] MdeModulePkg: Define the VariablePolicy protocol interface
> 
> On 05/12/20 08:46, Michael Kubacki wrote:
>> From: Bret Barkelew <brbarkel@microsoft.com>
>>
>> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2522&amp;data=02%7C01%7CBret.Barkelew%40microsoft.com%7Cf9d61ace6d7d42a2b9c008d7f66eb4b3%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637248827657827126&amp;sdata=s80j2lvjZROfSb9GR6g0NO0FwGN2c18v9Im8pmRRenE%3D&amp;reserved=0
>>
>> VariablePolicy is an updated interface to
>> replace VarLock and VarCheckProtocol.
>>
>> Add the VariablePolicy protocol interface
>> header and add to the MdeModulePkg.dec file.
>>
>> Cc: Jian J Wang <jian.j.wang@intel.com>
>> Cc: Hao A Wu <hao.a.wu@intel.com>
>> Cc: Liming Gao <liming.gao@intel.com>
>> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
>> ---
>>  MdeModulePkg/Include/Protocol/VariablePolicy.h | 157 ++++++++++++++++++++
>>  MdeModulePkg/MdeModulePkg.dec                  |  14 +-
>>  2 files changed, 170 insertions(+), 1 deletion(-)
>>
>> diff --git a/MdeModulePkg/Include/Protocol/VariablePolicy.h b/MdeModulePkg/Include/Protocol/VariablePolicy.h
>> new file mode 100644
>> index 000000000000..2cd025860554
>> --- /dev/null
>> +++ b/MdeModulePkg/Include/Protocol/VariablePolicy.h
>> @@ -0,0 +1,157 @@
>> +/** @file -- VariablePolicy.h
>> +
>> +This protocol allows communication with Variable Policy Engine.
>> +
>> +Copyright (c) Microsoft Corporation.
>> +SPDX-License-Identifier: BSD-2-Clause-Patent
>> +**/
>> +
>> +#ifndef __VARIABLE_POLICY_PROTOCOL__
>> +#define __VARIABLE_POLICY_PROTOCOL__
>> +
>> +#define VARIABLE_POLICY_PROTOCOL_REVISION   0x0000000000010000
>> +
>> +#define VARIABLE_POLICY_PROTOCOL_GUID \
>> +  { \
>> +    0x81D1675C, 0x86F6, 0x48DF, { 0xBD, 0x95, 0x9A, 0x6E, 0x4F, 0x09, 0x25, 0xC3 } \
>> +  }
>> +
>> +#define VARIABLE_POLICY_ENTRY_REVISION      0x00010000
>> +
>> +#pragma pack(push, 1)
>> +typedef struct {
>> +  UINT32   Version;
>> +  UINT16   Size;
>> +  UINT16   OffsetToName;
>> +  EFI_GUID Namespace;
>> +  UINT32   MinSize;
>> +  UINT32   MaxSize;
>> +  UINT32   AttributesMustHave;
>> +  UINT32   AttributesCantHave;
>> +  UINT8    LockPolicyType;
>> +  UINT8    Padding[3];
>> +  // UINT8    LockPolicy[];     // Variable Length Field
>> +  // CHAR16   Name[]            // Variable Length Field
>> +} VARIABLE_POLICY_ENTRY;
>> +
>> +#define     VARIABLE_POLICY_NO_MIN_SIZE             0
>> +#define     VARIABLE_POLICY_NO_MAX_SIZE             MAX_UINT32
>> +#define     VARIABLE_POLICY_NO_MUST_ATTR            0
>> +#define     VARIABLE_POLICY_NO_CANT_ATTR            0
>> +
>> +#define     VARIABLE_POLICY_TYPE_NO_LOCK            0
>> +#define     VARIABLE_POLICY_TYPE_LOCK_NOW           1
>> +#define     VARIABLE_POLICY_TYPE_LOCK_ON_CREATE     2
>> +#define     VARIABLE_POLICY_TYPE_LOCK_ON_VAR_STATE  3
>> +
>> +typedef struct {
>> +  EFI_GUID Namespace;
>> +  UINT8    Value;
>> +  UINT8    Padding;
>> +  // CHAR16   Name[];           // Variable Length Field
>> +} VARIABLE_LOCK_ON_VAR_STATE_POLICY;
>> +#pragma pack(pop)
>> +
>> +/**
>> +  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.
>> +
>> +**/
>> +typedef
>> +EFI_STATUS
>> +(EFIAPI *DISABLE_VARIABLE_POLICY)(
>> +  VOID
>> +  );
>> +
>> +/**
>> +  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.
>> +
>> +**/
>> +typedef
>> +EFI_STATUS
>> +(EFIAPI *IS_VARIABLE_POLICY_ENABLED)(
>> +  OUT BOOLEAN *State
>> +  );
>> +
>> +/**
>> +  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_ABORTED             A calculation error has prevented this function from completing.
>> +  @retval     EFI_OUT_OF_RESOURCES    Cannot grow the table to hold any more policies.
>> +
>> +**/
>> +typedef
>> +EFI_STATUS
>> +(EFIAPI *REGISTER_VARIABLE_POLICY)(
>> +  IN VARIABLE_POLICY_ENTRY *PolicyEntry
>> +  );
>> +
>> +/**
>> +  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.
>> +
>> +**/
>> +typedef
>> +EFI_STATUS
>> +(EFIAPI *DUMP_VARIABLE_POLICY)(
>> +  IN OUT UINT8  *Policy,
>> +  IN OUT UINT32 *Size
>> +  );
>> +
>> +/**
>> +  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.
>> +
>> +**/
>> +typedef
>> +EFI_STATUS
>> +(EFIAPI *LOCK_VARIABLE_POLICY)(
>> +  VOID
>> +  );
>> +
>> +typedef struct {
>> +  UINT64                     Revision;
>> +  DISABLE_VARIABLE_POLICY    DisableVariablePolicy;
>> +  IS_VARIABLE_POLICY_ENABLED IsVariablePolicyEnabled;
>> +  REGISTER_VARIABLE_POLICY   RegisterVariablePolicy;
>> +  DUMP_VARIABLE_POLICY       DumpVariablePolicy;
>> +  LOCK_VARIABLE_POLICY       LockVariablePolicy;
>> +} _VARIABLE_POLICY_PROTOCOL;
>> +
>> +typedef _VARIABLE_POLICY_PROTOCOL VARIABLE_POLICY_PROTOCOL;
>> +
>> +extern EFI_GUID gVariablePolicyProtocolGuid;
>> +
>> +#endif
>> diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
>> index 4f44af694862..f74fea00b6e7 100644
>> --- a/MdeModulePkg/MdeModulePkg.dec
>> +++ b/MdeModulePkg/MdeModulePkg.dec
>> @@ -8,7 +8,7 @@
>>  # Copyright (c) 2016, Linaro Ltd. All rights reserved.<BR>
>>  # (C) Copyright 2016 - 2019 Hewlett Packard Enterprise Development LP<BR>
>>  # Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR>
>> -# Copyright (c) 2016, Microsoft Corporation<BR>
>> +# Copyright (c) Microsoft Corporation.<BR>
>>  # SPDX-License-Identifier: BSD-2-Clause-Patent
>>  #
>>  ##
>> @@ -624,6 +624,9 @@
>>  #   0x80000006 | Incorrect error code provided.
>>  #
>>
>> +  ## Include/Protocol/VariablePolicy.h
>> +  gVariablePolicyProtocolGuid = { 0x81D1675C, 0x86F6, 0x48DF, { 0xBD, 0x95, 0x9A, 0x6E, 0x4F, 0x09, 0x25, 0xC3 } }
>> +
> 
> (1) Should be called gEdkiiVariablePolicyProtocolGuid, IMO.
> 
> Similarly, all VARIABLE_POLICY_PROTOCOL substrings should be
> EDKII_VARIABLE_POLICY_PROTOCOL, in the protocol header file, I believe.
> 
>>  [PcdsFeatureFlag]
>>    ## Indicates if the platform can support update capsule across a system reset.<BR><BR>
>>    #   TRUE  - Supports update capsule across a system reset.<BR>
>> @@ -1129,6 +1132,15 @@
>>    # @Prompt Variable storage size.
>>    gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0x10000|UINT32|0x30000005
>>
>> +  ## Toggle for whether the VariablePolicy engine should allow disabling.
>> +  # The engine is enabled at power-on, but the interface allows the platform to
>> +  # disable enforcement for servicing flexibility. If this PCD is disabled, it will block the ability to
>> +  # disable the enforcement and VariablePolicy enforcement will always be ON.
>> +  #   TRUE - VariablePolicy can be disabled by request through the interface (until interface is locked)
>> +  #   FALSE - VariablePolicy interface will not accept requests to disable and is ALWAYS ON
>> +  # @Prompt Allow VariablePolicy enforcement to be disabled.
>> +  gEfiMdeModulePkgTokenSpaceGuid.PcdAllowVariablePolicyEnforcementDisable|FALSE|BOOLEAN|0x30000020
>> +
>>    ## FFS filename to find the ACPI tables.
>>    # @Prompt FFS name of ACPI tables storage.
>>    gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiTableStorageFile|{ 0x25, 0x4e, 0x37, 0x7e, 0x01, 0x8e, 0xee, 0x4f, 0x87, 0xf2, 0x39, 0xc, 0x23, 0xc6, 0x6, 0xcd }|VOID*|0x30000016
>>
> 
> (2) This patch should update "MdeModulePkg.uni" in tandem with
> "MdeModulePkg.dec", I think.
> 
> Thanks
> Laszlo
> 
> 
> 
> 
>