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

Add a new Ovmf package to include and launch SVSM module in FV #1

Open
wants to merge 16 commits into
base: svsm
Choose a base branch
from

Conversation

roy-hopkins
Copy link

This adds a new target package within OvmfPkg that that takes a pre-built SVSM module binary and bundles it as part of the firmware image. When using the SVSM target package the OvmfPkg/ResetVector module is configured to launch the SVSM module on reset rather than launching OVMF directly.

joergroedel and others added 16 commits February 16, 2023 15:53
Signed-off-by: Joerg Roedel <[email protected]>
In preparation for running under an SVSM at VMPL1 or lower, 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]>
The SVSM specification relies on a specific register calling convention
to hold the parameters that are associated with the SVSM request.

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.

Signed-off-by: Tom Lendacky <[email protected]>
In order to support an SEV-SNP guest running under an SVSM at VMPL1 or
lower, the VmgExitLib library must be extended with new intefaces.

These new interfaces include an interface to detect if running under an
SVSM, an interface to perform memory validation and an interface to set
or clear the attribute that allows the page to be used as a VMSA.

Signed-off-by: Tom Lendacky <[email protected]>
Add initial support for the new VmgExitLib interfaces to the OvmfPkg
version of the library. The initial implementation will fully implement
the SVSM presence check API, with later patches fully implementing the
other interfaces.

The SVSM presence check, VmgExitSvsmPresent(), determines the presence
of an SVSM by checking if an SVSM has been advertised in the SEV-SNP
Secrets Page.

Signed-off-by: Tom Lendacky <[email protected]>
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 define corresponding PCDs
used to communicate the location and size of the area.

Signed-off-by: Tom Lendacky <[email protected]>
The PVALIDATE instruction can only be performed at VMPL0. An SVSM will
be present when running at VMPL1 or lower.

When an SVSM is present, use the SVSM_CORE_PVALIDATE call to perform
memory validation instead of issuing the PVALIDATE instruction directly.

Signed-off-by: Tom Lendacky <[email protected]>
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]>
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]>
…ds is set

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.

To use the AP Create protocol, the page used to hold the VM state must be
updated to set the VMSA attribute. The RMPADJUST instruction is used to do
this, but the VMSA attribute is only set 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.

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

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]>
Add a set of fixed values that will be included in the
metadata for an SVSM module that is packaged as
part of the OVMF firmware volume.

These values will be used in OvmfPkg/ResetVector
to populate OVMF metadata that corresponds to
a packaged SVSM module.

Signed-off-by: Roy Hopkins <[email protected]>
Adds a new OVMF package that bundles a pre-built SVSM
module as part of the firmware volume. The SVSM
module binary is located by default in Svsm/svsm.bin
 but this can be overridden by defining SVSM_MODULE_FILE
at build time.

Currently supports only a 4MB FV with the space
for the SVSM module being provided by reducing the
size of the FVMAIN_COMPACT volume.

Signed-off-by: Roy Hopkins <[email protected]>
Refactor of ResetVectorVtf0.asm to split the the VTF0 and
reset vector code from the OVMF metadata. This will allow
subsequent patches to conditionally modify the metadata
content and layout without having to modify the reset
vector code.

Signed-off-by: Roy Hopkins <[email protected]>
For builds using the OvmfPkgSvsmX64 package, the reset
vector launches a binary SVSM module in the FV instead of
launching OVMF directly.

The  metadata in the top 4K of the FV relates to the SVSM
module and not the OVMF firmware. The SVSM module will
look for the OVMF metadata 4K below this before launching
OVMF at the fixed address of 0xffffefe0.

Signed-off-by: Roy Hopkins <[email protected]>
cclaudio pushed a commit to cclaudio/edk2 that referenced this pull request Feb 20, 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]>
cclaudio pushed a commit to cclaudio/edk2 that referenced this pull request Jun 13, 2024
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4534

Bug Details:
PixieFail Bug coconut-svsm#1
CVE-2023-45229
CVSS 6.5 : CVSS:3.1/AV:A/AC:L/PR:N/UI:N/S:U/C:H/I:N/A:N
CWE-125 Out-of-bounds Read

Change Overview:

Introduce Dhcp6SeekInnerOptionSafe which performs checks before seeking
the Inner Option from a DHCP6 Option.

>
> EFI_STATUS
> Dhcp6SeekInnerOptionSafe (
>  IN  UINT16  IaType,
>  IN  UINT8   *Option,
>  IN  UINT32  OptionLen,
>  OUT UINT8   **IaInnerOpt,
>  OUT UINT16  *IaInnerLen
>  );
>

Lots of code cleanup to improve code readability.

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]>
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.

3 participants