Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update to svsm-preview-v4 + vTPM driver #2

Open
wants to merge 1,179 commits into
base: master
Choose a base branch
from

Conversation

cclaudio
Copy link
Member

This version is based on:

  • the svsm-preview-v4 branch from AMDESE/ovmf
  • one patch "disable brotli" cherry picked from current
  • TPM driver that supports the SVSM-VTPM

changab and others added 30 commits December 5, 2023 03:27
…tion

Introduce gEdkIIRedfishHostInterfaceReadyProtocolGuid
and produce it when Redfish Host Interface is installed
on system.

Signed-off-by: Abner Chang <[email protected]>
Cc: Nickle Wang <[email protected]>
Cc: Igor Kulchytskyy <[email protected]>
Cc: Mike Maslenkin <[email protected]>
Reviewed-by: Nickle Wang <[email protected]>
Acked-by: Mike Maslenkin <[email protected]>
Wait until Redfish Host Interface is installed on
the system then acquire Redfish service.

Signed-off-by: Abner Chang <[email protected]>
Cc: Nickle Wang <[email protected]>
Cc: Igor Kulchytskyy <[email protected]>
Cc: Mike Maslenkin <[email protected]>
Reviewed-by: Nickle Wang <[email protected]>
Acked-by: Mike Maslenkin <[email protected]>
…nction

Signed-off-by: Abner Chang <[email protected]>
Cc: Nickle Wang <[email protected]>
Cc: Igor Kulchytskyy <[email protected]>
Cc: Mike Maslenkin <[email protected]>
Reviewed-by: Nickle Wang <[email protected]>
Acked-by: Mike Maslenkin <[email protected]>
Signed-off-by: Abner Chang <[email protected]>
Cc: Nickle Wang <[email protected]>
Cc: Igor Kulchytskyy <[email protected]>
Cc: Mike Maslenkin <[email protected]>
Reviewed-by: Nickle Wang <[email protected]>
Acked-by: Mike Maslenkin <[email protected]>
Refine SMBIOS 42h code add mode debug message
for the error conditions.

Signed-off-by: Abner Chang <[email protected]>
Cc: Nickle Wang <[email protected]>
Cc: Igor Kulchytskyy <[email protected]>
Cc: Mike Maslenkin <[email protected]>
Reviewed-by: Nickle Wang <[email protected]>
Acked-by: Mike Maslenkin <[email protected]>
MAC address reference is incorrect when it is
copied to Host Interface DeviceDescriptor.

Signed-off-by: Abner Chang <[email protected]>
Cc: Nickle Wang <[email protected]>
Cc: Igor Kulchytskyy <[email protected]>
Cc: Mike Maslenkin <[email protected]>
Reviewed-by: Nickle Wang <[email protected]>
Acked-by: Mike Maslenkin <[email protected]>
The size of structure must be minus with byte that is
occupied by the initial array.

Signed-off-by: Abner Chang <[email protected]>
Cc: Nickle Wang <[email protected]>
Cc: Igor Kulchytskyy <[email protected]>
Cc: Mike Maslenkin <[email protected]>
Reviewed-by: Nickle Wang <[email protected]>
Acked-by: Mike Maslenkin <[email protected]>
Wrong memory allocation issue may result in memory
corruption.

Signed-off-by: Abner Chang <[email protected]>
Cc: Nickle Wang <[email protected]>
Cc: Igor Kulchytskyy <[email protected]>
Cc: Mike Maslenkin <[email protected]>
Reviewed-by: Nickle Wang <[email protected]>
Acked-by: Mike Maslenkin <[email protected]>
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4591

1. Refer NVME spec 2.0c chapter 5.24, add Sanitize Command
   related definition.
2. Refer NVME spec 2.0c chapter 5.16, add Get Log Page
   Command related definition for Sanitize status support.

Cc: Ray Ni <[email protected]>
Cc: Xiao X Chen <[email protected]>
Cc: Arthur Chen <[email protected]>
Cc: Liming Gao <[email protected]>
Cc: Zhiguang Liu <[email protected]>
Cc: Sean Brogan <[email protected]>
Cc: Michael D Kinney <[email protected]>
Signed-off-by: Tina Chen <[email protected]>
Reviewed-by: Michael D Kinney <[email protected]>
The local variable OneOfPagingEntry is used before initialized, this
may cause reserved bit in page table entry is set especially in PAE
paging mode. The bug is random because it depends on the value in
stack.

Reviewed-by: Ray Ni <[email protected]>
Cc: Rahul Kumar <[email protected]>
Cc: Gerd Hoffmann <[email protected]>
Cc: Laszlo Ersek <[email protected]>
Signed-off-by: Zhiguang Liu <[email protected]>
Refine test case:
1. Check PAE paging reserved bits is zero.
2. Set stack as random value.

Reviewed-by: Ray Ni <[email protected]>
Cc: Rahul Kumar <[email protected]>
Cc: Gerd Hoffmann <[email protected]>
Cc: Laszlo Ersek <[email protected]>
Signed-off-by: Zhiguang Liu <[email protected]>
Currently, there are code to set memory attribute in CpuMpPei module.
However, the code doesn't handle the case of 5 level paging.
Use the CpuPageTableLib to set memory attribute for two purpose:
1. Add 5 level paging support
2. Clean up code

Reviewed-by: Ray Ni <[email protected]>
Cc: Rahul Kumar <[email protected]>
Cc: Gerd Hoffmann <[email protected]>
Cc: Laszlo Ersek <[email protected]>
Signed-off-by: Zhiguang Liu <[email protected]>
__builtin_return_address returns a pointer, not a string. Fix
the STACK FAULT message in BaseStackCheckLib appropriately.

Signed-off-by: Jake Garver <[email protected]>
Reviewed-by: Liming Gao <[email protected]>
The DXE & MM standalone variant of AcpiTimerLib defines a global
named mPerformanceCounterFrequency. A global with an identical
name is also present in MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c

Since XhciDxe has a dependency on TimerLib, this can cause link
errors due to the same symbol being defined twice if the platform
DSC chooses to use AcpiTimerLib as the TimerLib implementation for
any given platform.

To resolve this, I have changed made the definition of
mPerformanceCounterFrequency to static and renamed it to
mAcpiTimerLibTscFrequency. Since this variable is not used outside
of the DxeStandaloneMmAcpiTimerLib.c compilation unit, there is no
reason to have it exported as a global.

Reviewed-by: Ray Ni <[email protected]>
Cc: Michael D Kinney <[email protected]>
Signed-off-by: Nate DeSimone <[email protected]>
The DXE & MM standalone variant of AcpiTimerLib defines a global
named mPerformanceCounterFrequency. A global with an identical
name is also present in MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c

Since XhciDxe has a dependency on TimerLib, this can cause link
errors due to the same symbol being defined twice if the platform
DSC chooses to use AcpiTimerLib as the TimerLib implementation for
any given platform.

To resolve this, I noted that some of the globals in Xhci.c are not
used outside of the Xhci.c compilation unit:

- mPerformanceCounterStartValue
- mPerformanceCounterEndValue
- mPerformanceCounterFrequency
- mPerformanceCounterValuesCached

I have changed the definition for all of these to static and added
an Xhci prefix. Since they are not used outside of the Xhci.c
compilation unit, there is no reason to have them exported as
globals.

Reviewed-by: Ray Ni <[email protected]>
Cc: Michael D Kinney <[email protected]>
Signed-off-by: Nate DeSimone <[email protected]>
Signed-off-by: Sheng Wei <[email protected]>
Cc: Eric Dong <[email protected]>
Cc: Ray Ni <[email protected]>
Cc: Laszlo Ersek <[email protected]>
Cc: Wu Jiaxin <[email protected]>
Cc: Tan Dun <[email protected]>
Reviewed-by: Ray Ni <[email protected]>
…asm.

Signed-off-by: Sheng Wei <[email protected]>
Cc: Eric Dong <[email protected]>
Cc: Ray Ni <[email protected]>
Cc: Laszlo Ersek <[email protected]>
Cc: Wu Jiaxin <[email protected]>
Cc: Tan Dun <[email protected]>
Reviewed-by: Ray Ni <[email protected]>
…les.

Signed-off-by: Sheng Wei <[email protected]>
Cc: Eric Dong <[email protected]>
Cc: Ray Ni <[email protected]>
Cc: Laszlo Ersek <[email protected]>
Cc: Wu Jiaxin <[email protected]>
Cc: Tan Dun <[email protected]>
Reviewed-by: Ray Ni <[email protected]>
Signed-off-by: Sheng Wei <[email protected]>
Cc: Eric Dong <[email protected]>
Cc: Ray Ni <[email protected]>
Cc: Laszlo Ersek <[email protected]>
Cc: Wu Jiaxin <[email protected]>
Cc: Tan Dun <[email protected]>
Reviewed-by: Ray Ni <[email protected]>
OS may enable CET-IBT feature by set MSR IA32_U_CET.bit2.
If IA32_U_CET.bit2 is set, CPU is in WAIT_FOR_ENDBRANCH state and
 the next assemble code is not ENDBR, it will trigger #CP exception
 when set CR4.CET bit.
SMI handler needs to backup MSR IA32_U_CET and clear MSR IA32_U_CET
 before set CR4.CET bit,
And SMI handler needs to restore MSR IA32_U_CET when exit SMI handler.

Signed-off-by: Sheng Wei <[email protected]>
Cc: Eric Dong <[email protected]>
Cc: Ray Ni <[email protected]>
Cc: Laszlo Ersek <[email protected]>
Cc: Wu Jiaxin <[email protected]>
Cc: Tan Dun <[email protected]>
Reviewed-by: Ray Ni <[email protected]>
Bhyve uses an io port range of [ 0x2000, 0x10000 ] [1]. At the moment,
EDKII is using a subset of this range [ 0xC000, 0x10000 ] [2]. Even
though the EDKII range doesn't exceed the bhyve range, it's causing
issues on some guests like OpenBSD [3]. We don't know why it's causing
issues yet. However, using the same IO port range in EDKII fixes the
issue and is a good idea anyway.

[1] https://github.com/freebsd/freebsd-src/blob/82ea0132c8b17a7a6067c8a36c6434e587ede6de/usr.sbin/bhyve/pci_emul.c#L133-L134
[2] https://github.com/tianocore/edk2/blob/fb044b7fe893a4545995bfe2701fd38e593355d9/OvmfPkg/Bhyve/PlatformPei/Platform.c#L156-L157
[3] https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=274389

Signed-off-by: Corvin Köhne <[email protected]>
Reviewed-by: Laszlo Ersek <[email protected]>
Reviewed-by: Rebecca Cran <[email protected]>
Cc: Ard Biesheuvel <[email protected]>
Cc: Gerd Hoffmann <[email protected]>
Cc: Liming Gao <[email protected]>
Cc: Jiewen Yao <[email protected]>
We're going to gradually tear down and remove the Compatibility Support
Module (CSM) in OvmfPkg (due to it having no maintainer). Start by making
all platforms that have thus far accepted "-D CSM_ENABLE" reject that
macro, so that mid-series, the partially removed infrastructure cannot be
built or booted.

Insert an !error directive in each DSC file's first "!ifdef $(CSM_ENABLE)"
conditional.

At the end of the series, the !error directive introduced in this patch
will be removed.

Cc: Anthony Perard <[email protected]>
Cc: Ard Biesheuvel <[email protected]>
Cc: Corvin Köhne <[email protected]>
Cc: Gerd Hoffmann <[email protected]>
Cc: Jiewen Yao <[email protected]>
Cc: Rebecca Cran <[email protected]>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=4588
Signed-off-by: Laszlo Ersek <[email protected]>
Message-Id: <[email protected]>
Reviewed-by: Jiewen Yao <[email protected]>
Reviewed-by: Ard Biesheuvel <[email protected]>
Acked-by: Corvin Köhne <[email protected]>
Acked-by: Gerd Hoffmann <[email protected]>
PcdCsmEnable was introduced in commits 50f911d ("OvmfPkg: introduce
PcdCsmEnable feature flag", 2020-02-05) and 75839f9
("OvmfPkg/PlatformPei: detect SMRAM at default SMBASE (for real)",
2020-02-05). Remove it, and substitute constant FALSE wherever it has been
evaluated thus far.

Regression test: after building OVMF IA32X64 with -D SMM_REQUIRE, and
booting it on Q35, the log still contains

> Q35SmramAtDefaultSmbaseInitialization: SMRAM at default SMBASE found

Cc: Anthony Perard <[email protected]>
Cc: Ard Biesheuvel <[email protected]>
Cc: Gerd Hoffmann <[email protected]>
Cc: Jiewen Yao <[email protected]>
https://bugzilla.tianocore.org/show_bug.cgi?id=4588
Signed-off-by: Laszlo Ersek <[email protected]>
Message-Id: <[email protected]>
Reviewed-by: Jiewen Yao <[email protected]>
Reviewed-by: Ard Biesheuvel <[email protected]>
Acked-by: Corvin Köhne <[email protected]>
Acked-by: Gerd Hoffmann <[email protected]>
Don't register the LegacyBmRefreshAllBootOption() and LegacyBmBoot()
functions in BdsDxe and UiApp.

Cc: Anthony Perard <[email protected]>
Cc: Ard Biesheuvel <[email protected]>
Cc: Corvin Köhne <[email protected]>
Cc: Gerd Hoffmann <[email protected]>
Cc: Jiewen Yao <[email protected]>
Cc: Rebecca Cran <[email protected]>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=4588
Signed-off-by: Laszlo Ersek <[email protected]>
Message-Id: <[email protected]>
Reviewed-by: Jiewen Yao <[email protected]>
Reviewed-by: Ard Biesheuvel <[email protected]>
Acked-by: Corvin Köhne <[email protected]>
Acked-by: Gerd Hoffmann <[email protected]>
LegacyBootManagerLib is not used by any platform at this point, remove it.

This patch removes mentions of the following CSM resources from the source
code:

- GUIDs (protocols or otherwise):
  - gEfiLegacyBiosProtocolGuid
  - gEfiLegacyDevOrderVariableGuid

- headers:
  - Guid/LegacyDevOrder.h
  - Protocol/LegacyBios.h

which extends the list of resources scheduled for removal to:

- GUIDs (protocols or otherwise):
  - gEfiLegacyBiosProtocolGuid
  - gEfiLegacyDevOrderVariableGuid

- headers:
  - Guid/LegacyDevOrder.h
  - Protocol/LegacyBios.h

Cc: Ard Biesheuvel <[email protected]>
Cc: Gerd Hoffmann <[email protected]>
Cc: Jiewen Yao <[email protected]>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=4588
Signed-off-by: Laszlo Ersek <[email protected]>
Message-Id: <[email protected]>
Reviewed-by: Jiewen Yao <[email protected]>
Reviewed-by: Ard Biesheuvel <[email protected]>
Acked-by: Corvin Köhne <[email protected]>
Acked-by: Gerd Hoffmann <[email protected]>
LegacyBootMaintUiLib registers a form (HII Config Access Protocol
instance) with UiApp, for configuring legacy boot options; stop plugging
it into UiApp.

Cc: Anthony Perard <[email protected]>
Cc: Ard Biesheuvel <[email protected]>
Cc: Corvin Köhne <[email protected]>
Cc: Gerd Hoffmann <[email protected]>
Cc: Jiewen Yao <[email protected]>
Cc: Rebecca Cran <[email protected]>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=4588
Signed-off-by: Laszlo Ersek <[email protected]>
Message-Id: <[email protected]>
Reviewed-by: Jiewen Yao <[email protected]>
Reviewed-by: Ard Biesheuvel <[email protected]>
Acked-by: Corvin Köhne <[email protected]>
Acked-by: Gerd Hoffmann <[email protected]>
LegacyBootMaintUiLib is not used by any platform at this point, remove it.

This patch removes mentions of the following CSM resources from the source
code:

- GUIDs (protocols or otherwise):
  - gEfiLegacyBiosProtocolGuid
  - gEfiLegacyDevOrderVariableGuid

- headers:
  - Guid/LegacyDevOrder.h
  - Protocol/LegacyBios.h

which extends the list of resources scheduled for removal to:

- GUIDs (protocols or otherwise):
  - gEfiLegacyBiosProtocolGuid
  - gEfiLegacyDevOrderVariableGuid

- headers:
  - Guid/LegacyDevOrder.h
  - Protocol/LegacyBios.h

Cc: Ard Biesheuvel <[email protected]>
Cc: Gerd Hoffmann <[email protected]>
Cc: Jiewen Yao <[email protected]>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=4588
Signed-off-by: Laszlo Ersek <[email protected]>
Message-Id: <[email protected]>
Reviewed-by: Jiewen Yao <[email protected]>
Reviewed-by: Ard Biesheuvel <[email protected]>
Acked-by: Corvin Köhne <[email protected]>
Acked-by: Gerd Hoffmann <[email protected]>
At this point, gEfiLegacyDevOrderVariableGuid is unused; remove it.

This shrinks the list of resources scheduled for removal to:

- GUIDs (protocols or otherwise):
  - gEfiLegacyBiosProtocolGuid

- headers:
  - Protocol/LegacyBios.h

Cc: Ard Biesheuvel <[email protected]>
Cc: Gerd Hoffmann <[email protected]>
Cc: Jiewen Yao <[email protected]>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=4588
Signed-off-by: Laszlo Ersek <[email protected]>
Message-Id: <[email protected]>
Reviewed-by: Jiewen Yao <[email protected]>
Reviewed-by: Ard Biesheuvel <[email protected]>
Acked-by: Corvin Köhne <[email protected]>
Acked-by: Gerd Hoffmann <[email protected]>
The CSM-based VideoDxe driver is a special UEFI_DRIVER module that both
follows and doesn't follow the UEFI driver model.

Namely, in the Supported and Start members of its Driver Binding Protocol
instance, it consumes the Legacy Bios Protocol directly from the UEFI
protocol database, as opposed to (only) opening protocols on the handle
that it is supposed to bind.

Furthermore, the driver "marks" its own image handle with the
NULL-interface "Legacy Bios" (pseudo-protocol) GUID, in order to "inform
back" the provider of the Legacy Bios Protocol, i.e., LegacyBiosDxe, that
VideoDxe is a "BIOS Thunk Driver" in the system.

Quoting "OvmfPkg/Csm/Include/Guid/LegacyBios.h", such a driver follows the
UEFI Driver Model, but still uses the Int86() or FarCall() services of the
Legacy Bios Protocol as the basis for the UEFI protocol it produces.

In a sense, there is a circular dependency between VideoDxe and
LegacyBiosDxe; each knows about the other. However, VideoDxe is a
UEFI_DRIVER, while LegacyBiosDxe is a platform DXE_DRIVER with a very long
DEPEX. Therefore, for keeping dependencies conceptually intact, first
exclude VideoDxe from the OVMF platforms. Always include the
hypervisor-specific real UEFI video driver.

--*--

Note that the pathname
"IntelFrameworkModulePkg/Csm/BiosThunk/VideoDxe/VideoDxe.inf" in the bhyve
platform DSC and FDF files is bogus anyway.

Cc: Anthony Perard <[email protected]>
Cc: Ard Biesheuvel <[email protected]>
Cc: Corvin Köhne <[email protected]>
Cc: Gerd Hoffmann <[email protected]>
Cc: Jiewen Yao <[email protected]>
Cc: Rebecca Cran <[email protected]>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=4588
Signed-off-by: Laszlo Ersek <[email protected]>
Message-Id: <[email protected]>
Reviewed-by: Jiewen Yao <[email protected]>
Reviewed-by: Ard Biesheuvel <[email protected]>
Acked-by: Corvin Köhne <[email protected]>
Acked-by: Gerd Hoffmann <[email protected]>
Csm/BiosThunk/VideoDxe is not used by any platform at this point, remove
it.

This patch removes mentions of the following CSM resources from the source
code:

- GUIDs (protocols or otherwise):
  - gEfiLegacyBiosGuid
  - gEfiLegacyBiosProtocolGuid
  - gEfiVgaMiniPortProtocolGuid

- headers:
  - FrameworkDxe.h
  - Guid/LegacyBios.h
  - Protocol/LegacyBios.h
  - Protocol/VgaMiniPort.h

- PCDs:
  - PcdBiosVideoCheckVbeEnable
  - PcdBiosVideoCheckVgaEnable
  - PcdBiosVideoSetTextVgaModeEnable

which extends the list of resources scheduled for removal to:

- GUIDs (protocols or otherwise):
  - gEfiLegacyBiosGuid
  - gEfiLegacyBiosProtocolGuid
  - gEfiVgaMiniPortProtocolGuid

- headers:
  - FrameworkDxe.h
  - Guid/LegacyBios.h
  - Protocol/LegacyBios.h
  - Protocol/VgaMiniPort.h

- PCDs:
  - PcdBiosVideoCheckVbeEnable
  - PcdBiosVideoCheckVgaEnable
  - PcdBiosVideoSetTextVgaModeEnable

Cc: Ard Biesheuvel <[email protected]>
Cc: Gerd Hoffmann <[email protected]>
Cc: Jiewen Yao <[email protected]>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=4588
Signed-off-by: Laszlo Ersek <[email protected]>
Message-Id: <[email protected]>
Reviewed-by: Jiewen Yao <[email protected]>
Reviewed-by: Ard Biesheuvel <[email protected]>
Acked-by: Corvin Köhne <[email protected]>
Acked-by: Gerd Hoffmann <[email protected]>
tanminger and others added 26 commits January 24, 2024 15:57
REF: UEFI_Spec_2_10_Aug29.pdf page 1694

In 35.5.4 EFI_HII_CONFIG_ACCESS_PROTOCOL.CallBack() parameter
ActionRequest, add EFI_BROWSER_ACTION_REQUEST_QUESTION_APPLY.

Signed-off-by: Ming Tan <[email protected]>
Cc: Michael D Kinney <[email protected]>
Cc: Liming Gao <[email protected]>
Cc: Zhiguang Liu <[email protected]>
Cc: Dandan Bi <[email protected]>
Reviewed-by: Dandan Bi <[email protected]>
Reviewed-by: Liming Gao <[email protected]>
REF: UEFI_Spec_2_10_Aug29.pdf page 1695.

In 35.5.4 EFI_HII_CONFIG_ACCESS_PROTOCOL.CallBack():
If the callback function returns with the ActionRequest set to
_QUESTION_APPLY, then the Forms Browser will write the current modified
question value on the selected form to storage.

Update the SetupBrowserDxe, if callback function return
EFI_BROWSER_ACTION_REQUEST_QUESTION_APPLY, then call SetQuestionValue
with GetSetValueWithHiiDriver to apply the change immediately.

Signed-off-by: Ming Tan <[email protected]>
Cc: Liming Gao <[email protected]>
Cc: Dandan Bi <[email protected]>
Reviewed-by: Dandan Bi <[email protected]>
Reviewed-by: Liming Gao <[email protected]>
REF: UEFI_Spec_2_10_Aug29.pdf page 1695.

In 35.5.4 EFI_HII_CONFIG_ACCESS_PROTOCOL.CallBack():
If the callback function returns with the ActionRequest set to
_QUESTION_APPLY, then the Forms Browser will write the current modified
question value on the selected form to storage.

Update the DriverSampleDxe, add a new question "Question apply test".

Signed-off-by: Ming Tan <[email protected]>
Cc: Liming Gao <[email protected]>
Cc: Dandan Bi <[email protected]>
Reviewed-by: Dandan Bi <[email protected]>
Reviewed-by: Liming Gao <[email protected]>
PciIoMap () need to feedback the status of
mIoMmuProtocol->SetAttribute () return value.

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4652

Reviewed-by: Ray Ni <[email protected]>
Reviewed-by: Huang Jenny <[email protected]>
Cc: Chiang Chris <[email protected]>
Signed-off-by: Sheng Wei <[email protected]>
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4166

Fix integer overflow in various CreateHob instances.
Fixes: CVE-2022-36765

The CreateHob() function aligns the requested size to 8
performing the following operation:
```
HobLength = (UINT16)((HobLength + 0x7) & (~0x7));
```

No checks are performed to ensure this value doesn't
overflow, and could lead to CreateHob() returning a smaller
HOB than requested, which could lead to OOB HOB accesses.

Reported-by: Marc Beatove <[email protected]>
Cc: Leif Lindholm <[email protected]>
Reviewed-by: Ard Biesheuvel <[email protected]>
Cc: Abner Chang <[email protected]>
Cc: John Mathew <[email protected]>
Authored-by: Gerd Hoffmann <[email protected]>
Signed-off-by: Gua Guo <[email protected]>
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4166

Fix integer overflow in various CreateHob instances.
Fixes: CVE-2022-36765

The CreateHob() function aligns the requested size to 8
performing the following operation:
```
HobLength = (UINT16)((HobLength + 0x7) & (~0x7));
```

No checks are performed to ensure this value doesn't
overflow, and could lead to CreateHob() returning a smaller
HOB than requested, which could lead to OOB HOB accesses.

Reported-by: Marc Beatove <[email protected]>
Reviewed-by: Ard Biesheuvel <[email protected]>
Cc: Sami Mujawar <[email protected]>
Reviewed-by: Ray Ni <[email protected]>
Cc: John Mathew <[email protected]>
Authored-by: Gerd Hoffmann <[email protected]>
Signed-off-by: Gua Guo <[email protected]>
Crypto in serveral case will use old version or latest version,
Platform may choose to only update Crypto drivers without updating
whole UPL, in this case the Crypto driver will provide by platform
payload outside the common UPL binary.

Reviewed-by: Chasel Chiu <[email protected]>
Cc: Guo Dong <[email protected]>
Cc: Sean Rhodes <[email protected]>
Reviewed-by: James Lu <[email protected]>
Cc: Gua Guo <[email protected]>
Signed-off-by: Gua Guo <[email protected]>
This reverts commit 049695a.

The referenced commit causes all SEV VMs to fail mapping the boot device
and, therefore, never presents a grub menu which prevents booting of the
guest. Temporarily revert this commit until a solution is found.

Signed-off-by: Tom Lendacky <[email protected]>
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4654

In preparation for running under an SVSM at VMPL1 or higher (higher
numerically, lower privilege), re-organize the way a page state change
is performed in order to free up the GHCB for use by the SVSM support.

Currently, the page state change logic directly uses the GHCB shared
buffer to build the page state change structures. However, this will be
in conflict with the use of the GHCB should an SVSM call be required.

Instead, use a separate buffer (an area in the workarea during SEC and
an allocated page during PEI/DXE) to hold the page state change request
and only update the GHCB shared buffer as needed.

Since the information is copied to, and operated on, in the GHCB shared
buffer this has the added benefit of not requiring to save the start and
end entries for use when validating the memory during the page state
change sequence.

Signed-off-by: Tom Lendacky <[email protected]>
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4654

The Secure VM Service Module specification defines the interfaces needed
to allow multi-VMPL level execution of an SEV-SNP guest.

Define the SVSM related structures for the SVSM Calling Area as well as
the SVSM CAA MSR. The SVSM CAA MSR is an MSR register that is reserved for
software use and will not be implemented in hardware.

Signed-off-by: Tom Lendacky <[email protected]>
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4654

The SVSM specification relies on a specific register calling convention to
hold the parameters that are associated with the SVSM request. The SVSM is
invoked by requesting the hypervisor to run the VMPL0 VMSA of the guest
using the GHCB MSR Protocol or a GHCB NAE event.

Create a new version of the VMGEXIT instruction that will adhere to this
calling convention and load the SVSM function arguments into the proper
register before invoking the VMGEXIT instruction. On return, perform the
atomic exchange on the SVSM call pending value as specified in the SVSM
specification.

Signed-off-by: Tom Lendacky <[email protected]>
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4654

In order to support an SEV-SNP guest running under an SVSM at VMPL1 or
lower, the CcExitLib library must be extended with new intefaces.

This includes an interface to detect if running under an SVSM, an
interface to return the current VMPL, an interface to perform memory
validation and an interface to set or clear the attribute that allows a
page to be used as a VMSA.

Signed-off-by: Tom Lendacky <[email protected]>
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4654

Add initial support for the new CcExitLib interfaces to the OvmfPkg
version of the library. The initial implementation will fully implement
the SVSM presence check API and the SVSM VMPL API, with later patches
fully implementing the other interfaces.

The SVSM presence check, CcExitSnpSvsmPresent(), determines the presence
of an SVSM by checking if an SVSM has been advertised in the SEV-SNP
Secrets Page. The SVSM VMPL API, CcExitSnpGetVmpl(), returns the VMPL
level at which the OVMF is currently running.

Signed-off-by: Tom Lendacky <[email protected]>
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4654

An SVSM requires a calling area page whose address (CAA) is used by the
SVSM to communicate and process the SVSM request.

Add a pre-defined page area to the OvmfPkg and AmdSev packages and define
corresponding PCDs used to communicate the location and size of the area.
Keep the AmdSev package in sync with the OvmfPkg and adjust the AmdSev
launch and hash area memory locations.

Signed-off-by: Tom Lendacky <[email protected]>
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4654

The PVALIDATE instruction can only be performed at VMPL0. An SVSM will
be present when running at VMPL1 or higher.

When an SVSM is present, use the SVSM_CORE_PVALIDATE call to perform
memory validation instead of issuing the PVALIDATE instruction directly.
This moves the current PVALIDATE functionality into the CcExitLib library,
where it can be determined whether an SVSM is present and perform the
proper operation.

Signed-off-by: Tom Lendacky <[email protected]>
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4654

The RMPADJUST instruction is used to alter the VMSA attribute of a page,
but the VMSA attribute can only be changed when running at VMPL0. When
an SVSM is present, use the SVSM_CORE_CREATE_VCPU and SVSM_CORE_DELTE_VCPU
calls to add or remove the VMSA attribute on a page instead of issuing
the RMPADJUST instruction directly.

Implement the CcExitSnpVmsaRmpAdjust() API to perform the proper operation
to update the VMSA attribute.

Signed-off-by: Tom Lendacky <[email protected]>
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4654

The RMPADJUST instruction is used to change the VMSA attribute of a page,
but the VMSA attribute can only be changed when running at VMPL0. When an
SVSM is present, use the SVSM_CORE_CREATE_VCPU and SVSM_CORE_DELTE_VCPU
calls to change the VMSA attribute on a page instead of issuing the
RMPADJUST instruction directly.

Implement the CcExitSnpVmsaRmpAdjust() API to perform the appropriate
operation to change the VMSA attribute based on the presence of an SVSM.

Signed-off-by: Tom Lendacky <[email protected]>
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4654

When an SVSM is present, starting the APs requires knowledge of the APIC
IDs. Create the definitions required to retrieve and hold the APIC ID
information of all the vCPUs present in the guest.

Signed-off-by: Tom Lendacky <[email protected]>
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4654

Create a PCD that can be used to set and get the APIC ID information that
is required for starting APs when an SVSM is present.

Signed-off-by: Tom Lendacky <[email protected]>
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4654

If the hypervisor supports retrieval of the vCPU APIC IDs, retrieve
them before any APs are actually started. The APIC IDs can be used
to start the APs for any SEV-SNP guest, but is a requirement for an
SEV-SNP guest that is running under an SVSM.

After retrieving the APIC IDs, save the address of the APIC ID data
structure in the PcdSevSnpApicIds PCD.

Signed-off-by: Tom Lendacky <[email protected]>
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4654

Currently, the first time an AP is started for an SEV-SNP guest, it relies
on the VMSA as set by the hypervisor. If the list of APIC IDs has been
retrieved, this is not necessary. Instead, use the SEV-SNP AP Create
protocol to start the AP for the first time and thereafter using the VMPL
at which the BSP is running.

Signed-off-by: Tom Lendacky <[email protected]>
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4654

When running under an SVSM, the VMPL level of the APs that are started
must match the VMPL level provided by the SVSM. Additionally, each AP
must have a Calling Area for use with the SVSM protocol. Update the AP
creation to properly support running under an SVSM.

Signed-off-by: Tom Lendacky <[email protected]>
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4654

The SVSM specification documents an alternative method of discovery for
the SVSM using a reserved CPUID bit and a reserved MSR.

For the CPUID support, the #VC handler of an SEV-SNP guest should modify
the returned value in the EAX register for the 0x8000001f CPUID function
by setting bit 28 when an SVSM is present.

For the MSR support, new reserved MSR 0xc001f000 has been defined. A #VC
should be generated when accessing this MSR. The #VC handler is expected
to ignore writes to this MSR and return the physical calling area address
(CAA) on reads of this MSR.

Signed-off-by: Tom Lendacky <[email protected]>
…VMPL0

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4654

Currently, an SEV-SNP guest will terminate if it is not running at VMPL0.
The requirement for running at VMPL0 is removed if an SVSM is present.

Update the current VMPL0 check to additionally check for the presence of
an SVSM is the guest is not running at VMPL0.

Additionally, fix an error in SevSnpIsVmpl0() where the Status variable
should be compared to 0 and not use the EFI_ERROR() function to determine
if an error occurred during AsmRmpAdjust().

Signed-off-by: Tom Lendacky <[email protected]>
Signed-off-by: Joerg Roedel <[email protected]>
Add this as a new device in TPM2DeviceLibDTpm.  The SVSM vTPM has no
physical presence interface, so handle detecting this device before
that check. The detection is done by sending a SVSM_VTPM_QUERY to
the SVSM.

Co-developed-by: Claudio Carvalho <[email protected]>
Signed-off-by: Claudio Carvalho <[email protected]>
Signed-off-by: James Bottomley <[email protected]>
cclaudio pushed a commit to cclaudio/edk2 that referenced this pull request Jun 13, 2024
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=4535

Bug Details:
PixieFail Bug coconut-svsm#2
CVE-2023-45230
CVSS 8.3 : CVSS:3.1/AV:A/AC:L/PR:N/UI:N/S:U/C:H/I:L/A:H
CWE-119 Improper Restriction of Operations within the Bounds
 of a Memory Buffer

Changes Overview:
> -UINT8 *
> +EFI_STATUS
>  Dhcp6AppendOption (
> -  IN OUT UINT8   *Buf,
> -  IN     UINT16  OptType,
> -  IN     UINT16  OptLen,
> -  IN     UINT8   *Data
> +  IN OUT EFI_DHCP6_PACKET  *Packet,
> +  IN OUT UINT8             **PacketCursor,
> +  IN     UINT16            OptType,
> +  IN     UINT16            OptLen,
> +  IN     UINT8             *Data
>    );

Dhcp6AppendOption() and variants can return errors now.  All callsites
are adapted accordingly.

It gets passed in EFI_DHCP6_PACKET as additional parameter ...

> +  //
> +  // Verify the PacketCursor is within the packet
> +  //
> +  if (  (*PacketCursor < Packet->Dhcp6.Option)
> +     || (*PacketCursor >= Packet->Dhcp6.Option +
 (Packet->Size - sizeof (EFI_DHCP6_HEADER))))
> +  {
> +    return EFI_INVALID_PARAMETER;
> +  }

... so it can look at Packet->Size when checking buffer space.
Also to allow Packet->Length updates.

Lots of checks added.

Cc: Saloni Kasbekar <[email protected]>
Cc: Zachary Clark-williams <[email protected]>

Signed-off-by: Doug Flick [MSFT] <[email protected]>
Reviewed-by: Saloni Kasbekar <[email protected]>
stefano-garzarella pushed a commit to stefano-garzarella/edk2 that referenced this pull request Oct 10, 2024
Root cause:
1. Before DisableReadonlyPageWriteProtect() is called, the return
address (coconut-svsm#1) is pushed in shadow stack.
2. CET is disabled.
3. DisableReadonlyPageWriteProtect() returns to coconut-svsm#1.
4. Page table is modified.
5. EnableReadonlyPageWriteProtect() is called, but the return
address (coconut-svsm#2) is not pushed in shadow stack.
6. CET is enabled.
7. EnableReadonlyPageWriteProtect() returns to coconut-svsm#2.
#CP exception happens because the actual return address (coconut-svsm#2)
doesn't match the return address stored in shadow stack (coconut-svsm#1).

Analysis:
Shadow stack will stop update after CET disable (DisableCet() in
DisableReadOnlyPageWriteProtect), but normal smi stack will be
continue updated with the function called and return
(DisableReadOnlyPageWriteProtect & EnableReadOnlyPageWriteProtect),
thus leading stack mismatch after CET re-enabled (EnableCet() in
EnableReadOnlyPageWriteProtect).

According SDM Vol 3, 6.15-Control Protection Exception:
Normal smi stack and shadow stack must be matched when CET enable,
otherwise CP Exception will happen, which is caused by a near RET
instruction.

CET is disabled in DisableCet(), while can be enabled in
EnableCet(). This way won't cause the problem because they are
implemented in a way that return address of DisableCet() is
poped out from shadow stack (Incsspq performs a pop to increases
the shadow stack) and EnableCet() doesn't use "RET" but "JMP" to
return to caller. So calling EnableCet() and DisableCet() doesn't
have the same issue as calling DisableReadonlyPageWriteProtect()
and EnableReadonlyPageWriteProtect().

With above root cause & analysis, define below 2 macros instead of
functions for WP & CET operation:
WRITE_UNPROTECT_RO_PAGES (Wp, Cet)
WRITE_PROTECT_RO_PAGES (Wp, Cet)
Because DisableCet() & EnableCet() must be in the same function
to avoid shadow stack and normal SMI stack mismatch.

Note: WRITE_UNPROTECT_RO_PAGES () must be called pair with
WRITE_PROTECT_RO_PAGES () in same function.

Change-Id: I4e126697efcd8dbfb4887da034d8691bfca969e3
Cc: Eric Dong <[email protected]>
Cc: Ray Ni <[email protected]>
Cc: Zeng Star <[email protected]>
Cc: Gerd Hoffmann <[email protected]>
Cc: Rahul Kumar <[email protected]>
Cc: Laszlo Ersek <[email protected]>
Signed-off-by: Jiaxin Wu <[email protected]>
Reviewed-by: Laszlo Ersek <[email protected]>
Reviewed-by: Ray Ni <[email protected]>
Reviewed-by: Eric Dong <[email protected]>
stefano-garzarella pushed a commit to stefano-garzarella/edk2 that referenced this pull request Nov 5, 2024
This patch does not impact functionality. It aims to clarify the
synchronization flow between the BSP and APs to enhance code
readability and understanding:

Steps tianocore#6 and tianocore#11 are the basic synchronization requirements for all
cases.

Steps coconut-svsm#1 is additional requirements if the MmCpuSyncModeTradition
mode is selected.

Steps coconut-svsm#1, coconut-svsm#2, coconut-svsm#3, coconut-svsm#4, tianocore#5, tianocore#7, tianocore#8, tianocore#9, and tianocore#10 are additional
requirements if the system needs to configure the MTRR.

Steps tianocore#9 and tianocore#10 are additional requirements if the system needs to
support the mSmmDebugAgentSupport.

Signed-off-by: Jiaxin Wu <[email protected]>
stefano-garzarella pushed a commit to stefano-garzarella/edk2 that referenced this pull request Nov 5, 2024
… func

This patch is for PiSmmCpuDxeSmm driver to add one round wait/release sync
for BSP and AP to perform the SMM CPU Platform Hook before executing MMI
Handler: SmmCpuPlatformHookBeforeMmiHandler (). With the function, SMM CPU
driver can perform the platform specific items after one round BSP and AP
sync (to make sure all APs in SMI) and before the MMI handlers.

After the change, steps coconut-svsm#1 and coconut-svsm#2 are additional requirements if the
MmCpuSyncModeTradition mode is selected.

Signed-off-by: Jiaxin Wu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.