Re: [edk2-devel] [PATCH v2 11/12] OvmfPkg: Add VariablePolicy engine to OvmfPkg platform

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

On 05/12/20 08:46, Michael Kubacki wrote:
> From: Bret Barkelew <brbarkel@microsoft.com>
>
> https://bugzilla.tianocore.org/show_bug.cgi?id=2522
>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
> ---
>  OvmfPkg/OvmfPkgIa32.dsc    | 8 ++++++++
>  OvmfPkg/OvmfPkgIa32X64.dsc | 8 ++++++++
>  OvmfPkg/OvmfPkgX64.dsc     | 8 ++++++++
>  3 files changed, 24 insertions(+)

(1) Repeating my request from under the blurb, please cover
"OvmfXen.dsc" too.

>
> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index 41ac3202961b..7c7b33a8bec3 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -3,6 +3,7 @@
>  #
>  #  Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
>  #  (C) Copyright 2016 Hewlett Packard Enterprise Development LP<BR>
> +#  Copyright (c) Microsoft Corporation.
>  #
>  #  SPDX-License-Identifier: BSD-2-Clause-Patent
>  #
> @@ -196,6 +197,8 @@
>    AuthVariableLib|MdeModulePkg/Library/AuthVariableLibNull/AuthVariableLibNull.inf
>  !endif
>    VarCheckLib|MdeModulePkg/Library/VarCheckLib/VarCheckLib.inf
> +  VariablePolicyLib|MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.inf
> +  VariablePolicyHelperLib|MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelperLib.inf
>
>
>    #
> @@ -334,6 +337,7 @@
>    BaseCryptLib|CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
>    PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
>    QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
> +  VariablePolicyLib|MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLibRuntimeDxe.inf
>
>  [LibraryClasses.common.UEFI_DRIVER]
>    PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> @@ -492,6 +496,9 @@
>    gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVolatileVariableSize|0x40000
>  !endif
>
> +  # Optional: Omit if VariablePolicy should be always-on.
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdAllowVariablePolicyEnforcementDisable|TRUE
> +
>    gEfiMdeModulePkgTokenSpaceGuid.PcdVpdBaseAddress|0x0
>
>    gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|0x07

(2) Based on the wiki article section here (which I find very useful):

  https://github.com/tianocore/tianocore.github.io/wiki/VariablePolicy-Protocol---Enhanced-Method-for-Managing-Variables#pcdallowvariablepolicyenforcementdisable

I'd like us to drop this hunk. The default should be secure.

Now, I realize that people might want to set this PCD to TRUE in OVMF,
for testing various things. Maybe the unit tests / functional tests
introduced in this series even *depend* on the PCD being TRUE (I can't
tell, I haven't checked). That's OK; for accommodating that, we have two
options:

(2a) build OVMF with the appropriate --pcd=... switch passed to "build",

(2b) for controlling the PCD dynamically (on the QEMU command line):

- the PCD would have permit the dynamic access method in the DEC file,

- the modules consuming the PCD would have to do so in their entry point
  functions / library constructors, and use the cached copy thenceforth,

- the OVMF DSC files would have to get a dynamic default (value FALSE),

- and OVMF would need another NULL class library for setting the PCD
  from fw_cfg. Please refer to
  <https://bugzilla.tianocore.org/show_bug.cgi?id=2681> for details on
  that.

The patch looks OK to me otherwise.

Thanks!
Laszlo

> @@ -945,6 +952,7 @@
>    MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf {
>      <LibraryClasses>
>        NULL|MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf
> +      NULL|MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.inf
>    }
>    MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.inf
>
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index c2f11aee2cec..8d5c6b3fc4b6 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -3,6 +3,7 @@
>  #
>  #  Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
>  #  (C) Copyright 2016 Hewlett Packard Enterprise Development LP<BR>
> +#  Copyright (c) Microsoft Corporation.
>  #
>  #  SPDX-License-Identifier: BSD-2-Clause-Patent
>  #
> @@ -200,6 +201,8 @@
>    AuthVariableLib|MdeModulePkg/Library/AuthVariableLibNull/AuthVariableLibNull.inf
>  !endif
>    VarCheckLib|MdeModulePkg/Library/VarCheckLib/VarCheckLib.inf
> +  VariablePolicyLib|MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.inf
> +  VariablePolicyHelperLib|MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelperLib.inf
>
>
>    #
> @@ -338,6 +341,7 @@
>    BaseCryptLib|CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
>    PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
>    QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
> +  VariablePolicyLib|MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLibRuntimeDxe.inf
>
>  [LibraryClasses.common.UEFI_DRIVER]
>    PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> @@ -496,6 +500,9 @@
>    gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVolatileVariableSize|0x40000
>  !endif
>
> +  # Optional: Omit if VariablePolicy should be always-on.
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdAllowVariablePolicyEnforcementDisable|TRUE
> +
>    gEfiMdeModulePkgTokenSpaceGuid.PcdVpdBaseAddress|0x0
>
>    gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|0x07
> @@ -959,6 +966,7 @@
>    MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf {
>      <LibraryClasses>
>        NULL|MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf
> +      NULL|MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.inf
>    }
>    MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.inf
>
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index 643e6041ad53..960d43eb1e84 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -3,6 +3,7 @@
>  #
>  #  Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
>  #  (C) Copyright 2016 Hewlett Packard Enterprise Development LP<BR>
> +#  Copyright (c) Microsoft Corporation.
>  #
>  #  SPDX-License-Identifier: BSD-2-Clause-Patent
>  #
> @@ -200,6 +201,8 @@
>    AuthVariableLib|MdeModulePkg/Library/AuthVariableLibNull/AuthVariableLibNull.inf
>  !endif
>    VarCheckLib|MdeModulePkg/Library/VarCheckLib/VarCheckLib.inf
> +  VariablePolicyLib|MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.inf
> +  VariablePolicyHelperLib|MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelperLib.inf
>
>
>    #
> @@ -338,6 +341,7 @@
>    BaseCryptLib|CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
>    PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
>    QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
> +  VariablePolicyLib|MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLibRuntimeDxe.inf
>
>  [LibraryClasses.common.UEFI_DRIVER]
>    PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> @@ -496,6 +500,9 @@
>    gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVolatileVariableSize|0x40000
>  !endif
>
> +  # Optional: Omit if VariablePolicy should be always-on.
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdAllowVariablePolicyEnforcementDisable|TRUE
> +
>    gEfiMdeModulePkgTokenSpaceGuid.PcdVpdBaseAddress|0x0
>
>    gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|0x07
> @@ -956,6 +963,7 @@
>    MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf {
>      <LibraryClasses>
>        NULL|MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf
> +      NULL|MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.inf
>    }
>    MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.inf
>
>