Re: [edk2-devel] [PATCH 1/2] OvmfPkg: Add VirtHstiDxe driver

["Yao, Jiewen" <jiewen.yao@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 9110 bytes --]

I agree that not all bits make sense to virtual machine.
However, I do see some bits should be there if we really want to add HSTI to report security propery.

Please take a look at the HSTI spec - https://learn.microsoft.com/en-us/windows-hardware/test/hlk/testref/hardware-security-testability-specification
For example:
Do you use RSA 2048 and SHA256 only (or higher but not lower than this)
Compatibility Support Modules (CSM)
Firmware Code must be present in protected storage
Secure firmware update process
Do you have backdoors to override SecureBoot
Protection from internal and external DMA


Another question: I notice you report platform as “Intel(R) 9-Series v1”.
Is that right configuration for current OVMF?
I think there is some configuration detection, such as https://github.com/tianocore/edk2/blob/master/OvmfPkg/PlatformPei/Platform.c.


All in all, I don’t think it is a right way to provide an *empty* one just to pass the SVVP.
That totally looses the value to having HSTI in the SVVP program.

I recommend we provide a real HSTI based on the OVMF threat model (without and with configuration computing) and current real implementation.

Thank you
Yao, Jiewen


From: Konstantin Kostiuk <kkostiuk@redhat.com>
Sent: Thursday, March 14, 2024 7:43 PM
To: Yao, Jiewen <jiewen.yao@intel.com>
Cc: devel@edk2.groups.io; Yan Vugenfirer <yvugenfi@redhat.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>; Gerd Hoffmann <kraxel@redhat.com>
Subject: Re: [PATCH 1/2] OvmfPkg: Add VirtHstiDxe driver



On Thu, Mar 14, 2024 at 12:28 PM Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>> wrote:
Question: What is the value to provide an *empty* HSTI table?

IMHO, If the goal is to perform some security check, I think we need provide a *real* HSTI table.

HSTI is very vendor-specific and depends on features that a vendor supports. Looking at
the HSTI spec a lot of the bits don't make sense for virtual machines. Some feature depends on
hardware configuration and this check is a dummy in a virtual environment.

So, the main goal is to pass Microsoft SVVP with OVMF+QEMU.

Best Regards,
Konstantin Kostiuk.


Thank you
Yao, Jiewen

> -----Original Message-----
> From: Konstantin Kostiuk <kkostiuk@redhat.com<mailto:kkostiuk@redhat.com>>
> Sent: Thursday, March 14, 2024 6:25 PM
> To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
> Cc: Yan Vugenfirer <yvugenfi@redhat.com<mailto:yvugenfi@redhat.com>>; Ard Biesheuvel
> <ardb+tianocore@kernel.org<mailto:ardb%2Btianocore@kernel.org>>; Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Gerd
> Hoffmann <kraxel@redhat.com<mailto:kraxel@redhat.com>>
> Subject: [PATCH 1/2] OvmfPkg: Add VirtHstiDxe driver
>
> The driver provides empty HSTI table.
>
> Signed-off-by: Konstantin Kostiuk <kkostiuk@redhat.com<mailto:kkostiuk@redhat.com>>
> ---
>  OvmfPkg/VirtHstiDxe/VirtHstiDxe.c   | 75 +++++++++++++++++++++++++++++
>  OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf | 64 ++++++++++++++++++++++++
>  2 files changed, 139 insertions(+)
>  create mode 100644 OvmfPkg/VirtHstiDxe/VirtHstiDxe.c
>  create mode 100644 OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf
>
> diff --git a/OvmfPkg/VirtHstiDxe/VirtHstiDxe.c
> b/OvmfPkg/VirtHstiDxe/VirtHstiDxe.c
> new file mode 100644
> index 0000000000..b9ed189f33
> --- /dev/null
> +++ b/OvmfPkg/VirtHstiDxe/VirtHstiDxe.c
> @@ -0,0 +1,75 @@
> +/** @file
>
> +  This file contains DXE driver for publishing empty HSTI table
>
> +
>
> +Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
>
> +Copyright (c) 2024, Red Hat. Inc
>
> +
>
> +SPDX-License-Identifier: BSD-2-Clause-Patent
>
> +
>
> +**/
>
> +
>
> +#include <PiDxe.h>
>
> +#include <Library/BaseLib.h>
>
> +#include <Library/DebugLib.h>
>
> +#include <Library/BaseMemoryLib.h>
>
> +#include <Library/MemoryAllocationLib.h>
>
> +#include <Library/UefiBootServicesTableLib.h>
>
> +#include <Library/UefiLib.h>
>
> +#include <IndustryStandard/Hsti.h>
>
> +#include <Library/HstiLib.h>
>
> +
>
> +#define HSTI_PLATFORM_NAME          L"Intel(R) 9-Series v1"
>
> +#define HSTI_SECURITY_FEATURE_SIZE  1
>
> +
>
> +ADAPTER_INFO_PLATFORM_SECURITY  mHstiBase = {
>
> +  PLATFORM_SECURITY_VERSION_VNEXTCS,
>
> +  PLATFORM_SECURITY_ROLE_PLATFORM_REFERENCE,
>
> +  { HSTI_PLATFORM_NAME },
>
> +  HSTI_SECURITY_FEATURE_SIZE,
>
> +};
>
> +
>
> +/**
>
> +  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 is executed successfully.
>
> +  @retval other           Some error occurs when executing this entry point.
>
> +**/
>
> +EFI_STATUS
>
> +EFIAPI
>
> +VirtHstiDxeEntrypoint (
>
> +  IN EFI_HANDLE        ImageHandle,
>
> +  IN EFI_SYSTEM_TABLE  *SystemTable
>
> +  )
>
> +{
>
> +  EFI_STATUS  Status;
>
> +
>
> +  // Allocate memory for HSTI struct
>
> +  // 3 * sizeof (UINT8) * HSTI_SECURITY_FEATURE_SIZE is for the 3 arrays
>
> +  //   UINT8   SecurityFeaturesRequired[];
>
> +  //   UINT8   SecurityFeaturesImplemented[];
>
> +  //   UINT8   SecurityFeaturesVerified[];
>
> +  // sizeof (CHAR16) is for the NULL terminator of ErrorString
>
> +  //   CHAR16 ErrorString[]
>
> +  UINTN  HstiSize = sizeof (ADAPTER_INFO_PLATFORM_SECURITY) +
>
> +                    3 * sizeof (UINT8) * HSTI_SECURITY_FEATURE_SIZE +
>
> +                    sizeof (CHAR16);
>
> +  VOID  *HstiStruct = AllocateZeroPool (HstiSize);
>
> +
>
> +  if (HstiStruct == NULL) {
>
> +    return EFI_OUT_OF_RESOURCES;
>
> +  }
>
> +
>
> +  CopyMem (HstiStruct, &mHstiBase, sizeof
> (ADAPTER_INFO_PLATFORM_SECURITY));
>
> +
>
> +  Status = HstiLibSetTable (HstiStruct, HstiSize);
>
> +  if (EFI_ERROR (Status)) {
>
> +    if (Status != EFI_ALREADY_STARTED) {
>
> +      ASSERT_EFI_ERROR (Status);
>
> +    }
>
> +  }
>
> +
>
> +  return EFI_SUCCESS;
>
> +}
>
> diff --git a/OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf
> b/OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf
> new file mode 100644
> index 0000000000..270aa60026
> --- /dev/null
> +++ b/OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf
> @@ -0,0 +1,64 @@
> +## @file
>
> +#  Component description file for Virt Hsti Driver
>
> +#
>
> +# Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
>
> +# Copyright (c) Microsoft Corporation.<BR>
>
> +# Copyright (c) 2024, Red Hat. Inc
>
> +#
>
> +# SPDX-License-Identifier: BSD-2-Clause-Patent
>
> +#
>
> +##
>
> +
>
> +[Defines]
>
> +  INF_VERSION                    = 0x00010005
>
> +  BASE_NAME                      = VirtHstiDxe
>
> +  FILE_GUID                      = 60740CF3-D428-4500-80E6-04A5798241ED
>
> +  MODULE_TYPE                    = DXE_DRIVER
>
> +  VERSION_STRING                 = 1.0
>
> +  ENTRY_POINT                    = VirtHstiDxeEntrypoint
>
> +
>
> +################################################################
> ################
>
> +#
>
> +# Sources Section - list of files that are required for the build to succeed.
>
> +#
>
> +################################################################
> ################
>
> +
>
> +[Sources]
>
> +  VirtHstiDxe.c
>
> +
>
> +################################################################
> ################
>
> +#
>
> +# Package Dependency Section - list of Package files that are required for
>
> +#                              this module.
>
> +#
>
> +################################################################
> ################
>
> +
>
> +[Packages]
>
> +  MdePkg/MdePkg.dec
>
> +
>
> +################################################################
> ################
>
> +#
>
> +# Library Class Section - list of Library Classes that are required for
>
> +#                         this module.
>
> +#
>
> +################################################################
> ################
>
> +
>
> +[LibraryClasses]
>
> +  UefiDriverEntryPoint
>
> +  UefiLib
>
> +  BaseLib
>
> +  BaseMemoryLib
>
> +  MemoryAllocationLib
>
> +  DebugLib
>
> +  HstiLib
>
> +  UefiBootServicesTableLib
>
> +
>
> +################################################################
> ################
>
> +#
>
> +# Protocol C Name Section - list of Protocol and Protocol Notify C Names
>
> +#                           that this module uses or produces.
>
> +#
>
> +################################################################
> ################
>
> +
>
> +[Depex]
>
> +  TRUE
>
> --
> 2.44.0


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116750): https://edk2.groups.io/g/devel/message/116750
Mute This Topic: https://groups.io/mt/104923813/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 17792 bytes --]