From 7b688fd71a8748ba7a191e20cc58ecb543a42bb1 Mon Sep 17 00:00:00 2001 From: Mike Ebersol Date: Wed, 19 Aug 2026 18:59:20 -0700 Subject: [PATCH 01/13] openhcl: preserve UEFI firmware across hibernation via VMGS snapshot Adds a VMGS firmware-image snapshot/restore path so a hibernation-enabled, non-isolated UEFI guest sees an identical firmware binary after resume even when the host cannot overload the firmware itself. This is the scoped-down firmware load/store portion of #3771; the hibernate token lifecycle (#4235) and the host-side GET LoadFirmware overload (#4262) already landed. It complements the overload path: on a firmware-version mismatch resume where the host does not advertise LoadFirmware support, underhill_core restores the exact firmware image snapshotted to VMGS at the original cold boot, rather than resuming on a mismatched firmware. - vmgs_format: add HIBERNATION_FIRMWARE (FileId 19) and VMGS_HIBERNATION_FIRMWARE_MIN_SIZE (32 MB overall-store gate). - vmgs/vmgs_broker: add device_size() on Vmgs, the broker, and the client. - vmgstool: parse the HIBERNATION_FIRMWARE file id. - underhill_core/worker: store the pristine firmware image to VMGS on a cold boot (before write_uefi_config) when the store is large enough, and restore it on resume as a fallback when the host lacks LoadFirmware support. Targets non-isolated VMs; CVM/isolated support is deferred. --- openhcl/underhill_core/src/worker.rs | 198 +++++++++++++++++++++++++-- vm/vmgs/vmgs/src/vmgs_impl.rs | 5 + vm/vmgs/vmgs_broker/src/broker.rs | 2 + vm/vmgs/vmgs_broker/src/client.rs | 8 ++ vm/vmgs/vmgs_format/src/lib.rs | 7 + vm/vmgs/vmgstool/src/main.rs | 1 + 6 files changed, 211 insertions(+), 10 deletions(-) diff --git a/openhcl/underhill_core/src/worker.rs b/openhcl/underhill_core/src/worker.rs index f3fcbb1a46e..0038d02f680 100644 --- a/openhcl/underhill_core/src/worker.rs +++ b/openhcl/underhill_core/src/worker.rs @@ -169,6 +169,7 @@ use vmcore::vmtime::VmTime; use vmcore::vmtime::VmTimeKeeper; use vmgs::Vmgs; use vmgs_broker::spawn_vmgs_broker; +use vmgs_format::VMGS_HIBERNATION_FIRMWARE_MIN_SIZE; use vmgs_format::VmgsProvisioner; use vmgs_format::VmgsProvisioningMarker; use vmgs_format::VmgsProvisioningReason; @@ -3738,16 +3739,56 @@ async fn new_underhill_vm( .management_vtl_features .load_firmware_supported() { - // The host must advertise LoadFirmware support; without it - // we can't overload, so resume on the current firmware. - tracing::warn!( - CVM_ALLOWED, - resume = %token, - "host does not support firmware overload (LoadFirmware); \ - resuming with the current firmware version despite a \ - hibernation token mismatch" - ); - Some(hibernate::Token::CURRENT) + // The host can't overload the firmware, so fall back to + // restoring the exact firmware image snapshotted to VMGS at + // the original cold boot. On success VTL0 runs the hibernated + // firmware (same region, same entry point), so record the + // hibernated version; otherwise resume on the current firmware. + match measured_vtl0_info + .as_ref() + .and_then(|info| info.supports_uefi.as_ref()) + { + Some(uefi_info) => { + match load_firmware_from_vmgs( + gm.vtl0(), + uefi_info.firmware_memory, + vmgs_client, + ) + .await + { + Ok(()) => { + tracing::info!( + CVM_ALLOWED, + resume = %token, + "host does not support firmware overload (LoadFirmware); \ + restored the hibernated firmware image from VMGS" + ); + Some(token) + } + Err(err) => { + tracing::warn!( + CVM_ALLOWED, + error = err.as_ref() as &dyn std::error::Error, + resume = %token, + "host does not support firmware overload (LoadFirmware) \ + and no usable firmware image in VMGS; resuming with the \ + current firmware version despite a hibernation token mismatch" + ); + Some(hibernate::Token::CURRENT) + } + } + } + None => { + tracing::warn!( + CVM_ALLOWED, + resume = %token, + "host does not support firmware overload (LoadFirmware); \ + resuming with the current firmware version despite a \ + hibernation token mismatch" + ); + Some(hibernate::Token::CURRENT) + } + } } else { tracing::info!( CVM_ALLOWED, @@ -3801,6 +3842,47 @@ async fn new_underhill_vm( None }; + // On a cold boot running the current firmware, snapshot the pristine UEFI + // firmware image into VMGS so a future hibernation resume can restore it + // bit-for-bit on a host that cannot overload the firmware itself. This must + // happen before `write_uefi_config` (in `load_firmware`) layers the dynamic + // config on top. A firmware-mismatch resume records a non-CURRENT token and + // is skipped here so the stored image is not overwritten. + if dps.general.hibernation_enabled + && !is_restoring + && current_hibernate_token == Some(hibernate::Token::CURRENT) + { + if let (Some(uefi_info), Some(vmgs_client)) = ( + measured_vtl0_info + .as_ref() + .and_then(|info| info.supports_uefi.as_ref()), + vmgs_client.as_ref(), + ) { + match store_firmware_to_vmgs(gm.vtl0(), uefi_info.firmware_memory, vmgs_client).await { + Ok(StoreFirmwareOutcome::Stored) => {} + Ok(StoreFirmwareOutcome::InsufficientSpace { + firmware_size, + device_size, + }) => { + tracing::warn!( + CVM_ALLOWED, + firmware_size, + device_size, + minimum_size = VMGS_HIBERNATION_FIRMWARE_MIN_SIZE, + "VMGS backing store too small to preserve UEFI firmware across hibernation" + ); + } + Err(err) => { + tracing::error!( + CVM_ALLOWED, + error = err.as_ref() as &dyn std::error::Error, + "failed to store UEFI firmware image for hibernation" + ); + } + } + } + } + // A Some token means hibernation is enabled and populated; pair it with a // VMGS client to drive token persistence at halt time. let hibernate_halt = @@ -4175,6 +4257,102 @@ async fn wait_for_flush_logs(control_send: &Arc anyhow::Result { + let len = firmware_memory.len(); + + // The firmware image can only be stored if the VMGS backing store is large + // enough. Require a minimum overall size so there is room for the firmware + // image alongside the other VMGS files, and also verify the image fits. + // Insufficient space is a normal fallback, not an error. + let device_size = vmgs_client + .device_size() + .await + .context("failed to query VMGS size")?; + if device_size < VMGS_HIBERNATION_FIRMWARE_MIN_SIZE || len > device_size { + return Ok(StoreFirmwareOutcome::InsufficientSpace { + firmware_size: len, + device_size, + }); + } + + let firmware_len = usize::try_from(len).context("firmware image size does not fit in usize")?; + let mut firmware = vec![0u8; firmware_len]; + gm.read_at(firmware_memory.start(), &mut firmware) + .context("failed to read UEFI firmware image from VTL0 memory")?; + vmgs_client + .write_file(vmgs::FileId::HIBERNATION_FIRMWARE, firmware) + .await + .context("failed to write UEFI firmware snapshot to VMGS")?; + tracing::info!( + CVM_ALLOWED, + size_bytes = len, + "stored UEFI firmware image to VMGS for hibernation" + ); + Ok(StoreFirmwareOutcome::Stored) +} + +/// Loads a previously stored UEFI firmware image from VMGS back into VTL0 guest +/// memory. Used when resuming a hibernated guest on a host that cannot overload +/// the firmware, so that the firmware binary matches what was present when the +/// guest hibernated. +/// +/// This must be called *before* `write_uefi_config`, so that the current +/// dynamic config is layered on top of the restored firmware image. +async fn load_firmware_from_vmgs( + gm: &GuestMemory, + firmware_memory: MemoryRange, + vmgs_client: &vmgs_broker::VmgsClient, +) -> anyhow::Result<()> { + let firmware = vmgs_client + .read_file(vmgs::FileId::HIBERNATION_FIRMWARE) + .await + .context("failed to read UEFI firmware snapshot from VMGS")?; + let region_len = firmware_memory.len(); + if firmware.len() as u64 != region_len { + anyhow::bail!( + "stored firmware image size {} does not match firmware region size {region_len}", + firmware.len(), + ); + } + gm.write_at(firmware_memory.start(), &firmware) + .context("failed to write UEFI firmware image into VTL0 memory")?; + tracing::info!( + CVM_ALLOWED, + size_bytes = region_len, + "restored UEFI firmware image from VMGS for hibernation resume" + ); + Ok(()) +} + /// Ask the host to overwrite VTL0's firmware image in guest RAM with the version /// identified by `token` (used on a hibernation resume under a different firmware /// version). On success, updates the measured UEFI context so VTL0 starts at the diff --git a/vm/vmgs/vmgs/src/vmgs_impl.rs b/vm/vmgs/vmgs/src/vmgs_impl.rs index 9a4e7692bd9..45040323f1b 100644 --- a/vm/vmgs/vmgs/src/vmgs_impl.rs +++ b/vm/vmgs/vmgs/src/vmgs_impl.rs @@ -641,6 +641,11 @@ impl Vmgs { Ok(vmgs) } + /// Returns the total size, in bytes, of the underlying VMGS backing store. + pub fn device_size(&self) -> u64 { + self.storage.sector_count() * self.storage.sector_size() as u64 + } + /// Get allocated and valid bytes from File Control Block for file_id. /// /// When reading data from a file, the buffer must be at least `valid_bytes` long. diff --git a/vm/vmgs/vmgs_broker/src/broker.rs b/vm/vmgs/vmgs_broker/src/broker.rs index 40ebb9a531a..8f0491f441b 100644 --- a/vm/vmgs/vmgs_broker/src/broker.rs +++ b/vm/vmgs/vmgs_broker/src/broker.rs @@ -50,6 +50,7 @@ impl From for FileId { pub enum VmgsBrokerRpc { Inspect(inspect::Deferred), GetFileInfo(Rpc>), + DeviceSize(Rpc<(), u64>), ReadFile(Rpc, VmgsBrokerError>>), WriteFile(Rpc<(BrokerFileId, Vec), Result<(), VmgsBrokerError>>), #[cfg(feature = "encryption")] @@ -84,6 +85,7 @@ impl VmgsBrokerTask { } VmgsBrokerRpc::GetFileInfo(rpc) => rpc .handle_sync(|file_id| self.vmgs.get_file_info(file_id.into()).map_err(Into::into)), + VmgsBrokerRpc::DeviceSize(rpc) => rpc.handle_sync(|()| self.vmgs.device_size()), VmgsBrokerRpc::ReadFile(rpc) => { rpc.handle(async |file_id| { self.vmgs diff --git a/vm/vmgs/vmgs_broker/src/client.rs b/vm/vmgs/vmgs_broker/src/client.rs index 42feeb4f41e..0b1d583c781 100644 --- a/vm/vmgs/vmgs_broker/src/client.rs +++ b/vm/vmgs/vmgs_broker/src/client.rs @@ -63,6 +63,14 @@ impl VmgsClient { Ok(res) } + /// Returns the total size, in bytes, of the underlying VMGS backing store. + #[instrument(skip_all)] + pub async fn device_size(&self) -> Result { + let res = self.control.call(VmgsBrokerRpc::DeviceSize, ()).await?; + + Ok(res) + } + /// Reads the specified `file_id`. #[instrument(skip_all, fields(file_id = %file_id))] pub async fn read_file(&self, file_id: FileId) -> Result, VmgsClientError> { diff --git a/vm/vmgs/vmgs_format/src/lib.rs b/vm/vmgs/vmgs_format/src/lib.rs index 837bec09b99..d4640e92ebc 100644 --- a/vm/vmgs/vmgs_format/src/lib.rs +++ b/vm/vmgs/vmgs_format/src/lib.rs @@ -58,6 +58,7 @@ open_enum! { PROVENANCE_DOC = 16, TPM_NVRAM_BACKUP = 17, PROVISIONING_MARKER = 18, + HIBERNATION_FIRMWARE = 19, EXTENDED_FILE_TABLE = 63, } @@ -69,6 +70,12 @@ impl Display for FileId { } } +/// The minimum overall VMGS backing-store size, in bytes, required before a UEFI +/// firmware image snapshot ([`FileId::HIBERNATION_FIRMWARE`]) is stored for +/// hibernation. This is a minimum overall size so that the firmware image fits +/// alongside the other VMGS files, not just a tight fit of the image itself. +pub const VMGS_HIBERNATION_FIRMWARE_MIN_SIZE: u64 = 32 * 1024 * 1024; + pub const VMGS_VERSION_2_0: u32 = 0x00020000; pub const VMGS_VERSION_3_0: u32 = 0x00030000; diff --git a/vm/vmgs/vmgstool/src/main.rs b/vm/vmgs/vmgstool/src/main.rs index 3661dd3b2ef..954111fee4d 100644 --- a/vm/vmgs/vmgstool/src/main.rs +++ b/vm/vmgs/vmgstool/src/main.rs @@ -373,6 +373,7 @@ fn parse_file_id(file_id: &str) -> Result { "PLATFORM_SEED" => FileId::PLATFORM_SEED, "PROVENANCE_DOC" => FileId::PROVENANCE_DOC, "TPM_NVRAM_BACKUP" => FileId::TPM_NVRAM_BACKUP, + "HIBERNATION_FIRMWARE" => FileId::HIBERNATION_FIRMWARE, "EXTENDED_FILE_TABLE" => FileId::EXTENDED_FILE_TABLE, v => FileId(v.parse::()?), }) From 92dcf637e7d48df17f87c444cf7ffe2fe1125c66 Mon Sep 17 00:00:00 2001 From: Mike Ebersol Date: Thu, 20 Aug 2026 09:18:50 -0700 Subject: [PATCH 02/13] vmgs_format: move HIBERNATION_FIRMWARE to file id 20 (19 reserved for TPM_185_NVRAM) --- vm/vmgs/vmgs_format/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vm/vmgs/vmgs_format/src/lib.rs b/vm/vmgs/vmgs_format/src/lib.rs index d4640e92ebc..9b7b52ae3dd 100644 --- a/vm/vmgs/vmgs_format/src/lib.rs +++ b/vm/vmgs/vmgs_format/src/lib.rs @@ -58,7 +58,7 @@ open_enum! { PROVENANCE_DOC = 16, TPM_NVRAM_BACKUP = 17, PROVISIONING_MARKER = 18, - HIBERNATION_FIRMWARE = 19, + HIBERNATION_FIRMWARE = 20, EXTENDED_FILE_TABLE = 63, } From 61cf5c905e117289e391a368c490dba43fed7847 Mon Sep 17 00:00:00 2001 From: Mike Ebersol Date: Thu, 20 Aug 2026 09:32:42 -0700 Subject: [PATCH 03/13] openhcl: prefer VMGS firmware restore and rebase x64 RIP - Snapshot the firmware image on any non-restore boot after any overload/ restore has run (not only when the token is CURRENT), so the image the guest will actually run is preserved for the next hibernation. - On a firmware-version-mismatch resume, prefer restoring the snapshotted image from VMGS over the host LoadFirmware overload, falling back to overload, then to the current firmware. - On x86_64, rebase VTL0's RIP to the restored image's SEC entry point, since it may differ from the cold-boot image. Exposes loader::uefi::get_sec_entry_point_offset to compute it from the image. --- openhcl/underhill_core/src/worker.rs | 204 ++++++++++++++++----------- vm/loader/src/uefi/mod.rs | 8 +- 2 files changed, 132 insertions(+), 80 deletions(-) diff --git a/openhcl/underhill_core/src/worker.rs b/openhcl/underhill_core/src/worker.rs index 0038d02f680..6dc6591b6e5 100644 --- a/openhcl/underhill_core/src/worker.rs +++ b/openhcl/underhill_core/src/worker.rs @@ -3734,62 +3734,26 @@ async fn new_underhill_vm( Some(token @ hibernate::Token::Hibernated { .. }) if token != hibernate::Token::CURRENT => { - if !dps + // Prefer restoring the exact firmware image snapshotted to VMGS + // at the original cold boot: it reproduces the hibernated + // firmware bit-for-bit without needing host support. Fall back to + // a host firmware overload if no usable image is stored, then to + // resuming on the current firmware. + if restore_vtl0_firmware_from_vmgs(gm.vtl0(), &mut measured_vtl0_info, vmgs_client) + .await + { + tracing::info!( + CVM_ALLOWED, + resume = %token, + "hibernation resume under a different firmware version; \ + restored the hibernated firmware image from VMGS" + ); + Some(token) + } else if dps .general .management_vtl_features .load_firmware_supported() { - // The host can't overload the firmware, so fall back to - // restoring the exact firmware image snapshotted to VMGS at - // the original cold boot. On success VTL0 runs the hibernated - // firmware (same region, same entry point), so record the - // hibernated version; otherwise resume on the current firmware. - match measured_vtl0_info - .as_ref() - .and_then(|info| info.supports_uefi.as_ref()) - { - Some(uefi_info) => { - match load_firmware_from_vmgs( - gm.vtl0(), - uefi_info.firmware_memory, - vmgs_client, - ) - .await - { - Ok(()) => { - tracing::info!( - CVM_ALLOWED, - resume = %token, - "host does not support firmware overload (LoadFirmware); \ - restored the hibernated firmware image from VMGS" - ); - Some(token) - } - Err(err) => { - tracing::warn!( - CVM_ALLOWED, - error = err.as_ref() as &dyn std::error::Error, - resume = %token, - "host does not support firmware overload (LoadFirmware) \ - and no usable firmware image in VMGS; resuming with the \ - current firmware version despite a hibernation token mismatch" - ); - Some(hibernate::Token::CURRENT) - } - } - } - None => { - tracing::warn!( - CVM_ALLOWED, - resume = %token, - "host does not support firmware overload (LoadFirmware); \ - resuming with the current firmware version despite a \ - hibernation token mismatch" - ); - Some(hibernate::Token::CURRENT) - } - } - } else { tracing::info!( CVM_ALLOWED, resume = %token, @@ -3809,6 +3773,15 @@ async fn new_underhill_vm( } else { Some(hibernate::Token::CURRENT) } + } else { + tracing::warn!( + CVM_ALLOWED, + resume = %token, + "no stored firmware image and host does not support firmware \ + overload (LoadFirmware); resuming with the current firmware \ + version despite a hibernation token mismatch" + ); + Some(hibernate::Token::CURRENT) } } Some(hibernate::Token::Hibernated { .. }) => { @@ -3842,16 +3815,15 @@ async fn new_underhill_vm( None }; - // On a cold boot running the current firmware, snapshot the pristine UEFI - // firmware image into VMGS so a future hibernation resume can restore it - // bit-for-bit on a host that cannot overload the firmware itself. This must - // happen before `write_uefi_config` (in `load_firmware`) layers the dynamic - // config on top. A firmware-mismatch resume records a non-CURRENT token and - // is skipped here so the stored image is not overwritten. - if dps.general.hibernation_enabled - && !is_restoring - && current_hibernate_token == Some(hibernate::Token::CURRENT) - { + // Snapshot the pristine UEFI firmware image now present in VTL0 memory into + // VMGS so a future hibernation resume can restore it bit-for-bit on a host + // that cannot overload the firmware itself. This runs after any firmware + // overload/restore above (so it captures the image the guest will actually + // run) and before `write_uefi_config` (in `load_firmware`) layers the + // dynamic config on top, regardless of the resolved token. A servicing + // restore is skipped: its firmware is already running with dynamic config + // applied and is no longer pristine. + if dps.general.hibernation_enabled && !is_restoring { if let (Some(uefi_info), Some(vmgs_client)) = ( measured_vtl0_info .as_ref() @@ -4320,37 +4292,111 @@ async fn store_firmware_to_vmgs( Ok(StoreFirmwareOutcome::Stored) } -/// Loads a previously stored UEFI firmware image from VMGS back into VTL0 guest -/// memory. Used when resuming a hibernated guest on a host that cannot overload -/// the firmware, so that the firmware binary matches what was present when the -/// guest hibernated. +/// Restores a previously stored UEFI firmware image from VMGS back into VTL0 +/// guest memory when resuming a hibernated guest, so the firmware binary matches +/// what was present when the guest hibernated. Preferred over a host firmware +/// overload because it needs no host support. On x86_64 the restored image's SEC +/// entry point may differ from the cold-boot image's, so VTL0's RIP is rebased +/// to it (`load_firmware` later applies the updated measured UEFI context). +/// Returns `true` if an image was restored. /// -/// This must be called *before* `write_uefi_config`, so that the current -/// dynamic config is layered on top of the restored firmware image. -async fn load_firmware_from_vmgs( +/// This must be called *before* `write_uefi_config`, so that the current dynamic +/// config is layered on top of the restored firmware image. Best-effort: on any +/// failure the current firmware is left in place and `false` is returned. +async fn restore_vtl0_firmware_from_vmgs( gm: &GuestMemory, - firmware_memory: MemoryRange, + measured_vtl0_info: &mut Option, vmgs_client: &vmgs_broker::VmgsClient, -) -> anyhow::Result<()> { - let firmware = vmgs_client +) -> bool { + let Some(firmware_memory) = measured_vtl0_info + .as_ref() + .and_then(|info| info.supports_uefi.as_ref()) + .map(|uefi_info| uefi_info.firmware_memory) + else { + // Not a UEFI boot; nothing to restore. + return false; + }; + + let firmware = match vmgs_client .read_file(vmgs::FileId::HIBERNATION_FIRMWARE) .await - .context("failed to read UEFI firmware snapshot from VMGS")?; + { + Ok(firmware) => firmware, + Err(err) => { + tracing::warn!( + CVM_ALLOWED, + error = &err as &dyn std::error::Error, + "no UEFI firmware image to restore from VMGS on hibernation resume" + ); + return false; + } + }; + let region_len = firmware_memory.len(); if firmware.len() as u64 != region_len { - anyhow::bail!( - "stored firmware image size {} does not match firmware region size {region_len}", - firmware.len(), + tracing::warn!( + CVM_ALLOWED, + stored_size = firmware.len(), + region_size = region_len, + "stored UEFI firmware image size does not match the firmware region; \ + not restoring" + ); + return false; + } + + // On x86_64, compute the restored image's SEC entry point before touching + // guest memory so a malformed image is a clean fallback (nothing written). + #[cfg(guest_arch = "x86_64")] + let new_rip = match loader::uefi::get_sec_entry_point_offset(&firmware) { + Some(offset) => firmware_memory.start() + offset, + None => { + tracing::warn!( + CVM_ALLOWED, + "could not find a SEC entry point in the stored UEFI firmware image; \ + not restoring" + ); + return false; + } + }; + + if let Err(err) = gm.write_at(firmware_memory.start(), &firmware) { + tracing::error!( + CVM_ALLOWED, + error = &err as &dyn std::error::Error, + "failed to write restored UEFI firmware image into VTL0 memory" ); + return false; } - gm.write_at(firmware_memory.start(), &firmware) - .context("failed to write UEFI firmware image into VTL0 memory")?; + + // Rebase VTL0's RIP to the restored image's entry point; `load_firmware` + // applies the updated measured UEFI context to VTL0. + #[cfg(guest_arch = "x86_64")] + if let Some(uefi_info) = measured_vtl0_info + .as_mut() + .and_then(|info| info.supports_uefi.as_mut()) + { + let crate::loader::VpContext::Vbs(registers) = &mut uefi_info.vp_context; + for reg in registers { + if let loader::importer::X86Register::Rip(rip) = reg { + if *rip != new_rip { + tracing::info!( + CVM_ALLOWED, + old_rip = *rip, + new_rip, + "rebased VTL0 RIP for restored firmware" + ); + *rip = new_rip; + } + } + } + } + tracing::info!( CVM_ALLOWED, size_bytes = region_len, "restored UEFI firmware image from VMGS for hibernation resume" ); - Ok(()) + true } /// Ask the host to overwrite VTL0's firmware image in guest RAM with the version diff --git a/vm/loader/src/uefi/mod.rs b/vm/loader/src/uefi/mod.rs index 0c66b3682d6..fffd98f0b5e 100644 --- a/vm/loader/src/uefi/mod.rs +++ b/vm/loader/src/uefi/mod.rs @@ -215,7 +215,13 @@ struct EFI_COMMON_SECTION_HEADER { const EFI_SECTION_PE32: u8 = 0x10; /// Get the SEC entry point offset from the firmware base. -fn get_sec_entry_point_offset(image: &[u8]) -> Option { +/// +/// On x86_64 the initial VTL0 RIP is the firmware base plus this offset. It is +/// exposed so callers that swap the firmware image at runtime (e.g. restoring a +/// hibernated firmware image) can recompute the entry point from the new image. +/// Returns `None` if the image has no recognizable SEC firmware volume / entry +/// point. +pub fn get_sec_entry_point_offset(image: &[u8]) -> Option { // Skip to SEC volume start. let mut image_offset = SEC_FIRMWARE_VOLUME_OFFSET; From 81a9a6aeec7e7fdafd92cdd65860e62744ba60b4 Mon Sep 17 00:00:00 2001 From: Mike Ebersol Date: Thu, 20 Aug 2026 13:46:06 -0700 Subject: [PATCH 04/13] openhcl: extract shared set_vtl0_uefi_rip helper The overload and VMGS-restore paths both walked the VBS vp_context registers to rebase VTL0's RIP. Factor that into a single x86_64 set_vtl0_uefi_rip helper; each caller just computes new_rip and delegates. --- openhcl/underhill_core/src/worker.rs | 81 ++++++++++++++-------------- 1 file changed, 41 insertions(+), 40 deletions(-) diff --git a/openhcl/underhill_core/src/worker.rs b/openhcl/underhill_core/src/worker.rs index 6dc6591b6e5..04f41ee1c67 100644 --- a/openhcl/underhill_core/src/worker.rs +++ b/openhcl/underhill_core/src/worker.rs @@ -4371,25 +4371,7 @@ async fn restore_vtl0_firmware_from_vmgs( // Rebase VTL0's RIP to the restored image's entry point; `load_firmware` // applies the updated measured UEFI context to VTL0. #[cfg(guest_arch = "x86_64")] - if let Some(uefi_info) = measured_vtl0_info - .as_mut() - .and_then(|info| info.supports_uefi.as_mut()) - { - let crate::loader::VpContext::Vbs(registers) = &mut uefi_info.vp_context; - for reg in registers { - if let loader::importer::X86Register::Rip(rip) = reg { - if *rip != new_rip { - tracing::info!( - CVM_ALLOWED, - old_rip = *rip, - new_rip, - "rebased VTL0 RIP for restored firmware" - ); - *rip = new_rip; - } - } - } - } + set_vtl0_uefi_rip(measured_vtl0_info, new_rip); tracing::info!( CVM_ALLOWED, @@ -4399,6 +4381,34 @@ async fn restore_vtl0_firmware_from_vmgs( true } +/// Rebase VTL0's initial RIP in the measured UEFI context to `new_rip`, so +/// `load_firmware` starts VTL0 at the entry point of a firmware image swapped in +/// at runtime (host overload or VMGS restore). A no-op if there is no measured +/// UEFI context or RIP register. +#[cfg(guest_arch = "x86_64")] +fn set_vtl0_uefi_rip(measured_vtl0_info: &mut Option, new_rip: u64) { + let Some(uefi_info) = measured_vtl0_info + .as_mut() + .and_then(|info| info.supports_uefi.as_mut()) + else { + return; + }; + let crate::loader::VpContext::Vbs(registers) = &mut uefi_info.vp_context; + for reg in registers { + if let loader::importer::X86Register::Rip(rip) = reg { + if *rip != new_rip { + tracing::info!( + CVM_ALLOWED, + old_rip = *rip, + new_rip, + "rebased VTL0 RIP for hibernation firmware swap" + ); + *rip = new_rip; + } + } + } +} + /// Ask the host to overwrite VTL0's firmware image in guest RAM with the version /// identified by `token` (used on a hibernation resume under a different firmware /// version). On success, updates the measured UEFI context so VTL0 starts at the @@ -4435,13 +4445,17 @@ async fn overload_vtl0_firmware( // so point the measured UEFI context's RIP at the host-computed offset; // load_firmware later applies it to VTL0. #[cfg(guest_arch = "x86_64")] - if let Some(uefi_info) = measured_vtl0_info - .as_mut() - .and_then(|info| info.supports_uefi.as_mut()) - { - if offset != 0 { - let base = uefi_info.firmware_memory.start(); - let len = uefi_info.firmware_memory.len(); + if offset != 0 { + if let Some((base, len)) = measured_vtl0_info + .as_ref() + .and_then(|info| info.supports_uefi.as_ref()) + .map(|uefi_info| { + ( + uefi_info.firmware_memory.start(), + uefi_info.firmware_memory.len(), + ) + }) + { // `offset` is host-provided (untrusted): the entry point must land // inside the measured firmware region and must not overflow. let Some(new_rip) = base.checked_add(offset).filter(|_| offset < len) else { @@ -4454,20 +4468,7 @@ async fn overload_vtl0_firmware( ); return false; }; - let crate::loader::VpContext::Vbs(registers) = &mut uefi_info.vp_context; - for reg in registers { - if let loader::importer::X86Register::Rip(rip) = reg { - if *rip != new_rip { - tracing::info!( - CVM_ALLOWED, - old_rip = *rip, - new_rip, - "rebased VTL0 RIP for overloaded firmware" - ); - *rip = new_rip; - } - } - } + set_vtl0_uefi_rip(measured_vtl0_info, new_rip); } } #[cfg(guest_arch = "aarch64")] From 210fbacf7c10b395b150238ad8abb4035a083541 Mon Sep 17 00:00:00 2001 From: Mike Ebersol Date: Thu, 20 Aug 2026 13:46:56 -0700 Subject: [PATCH 05/13] openhcl: log insufficient VMGS space for firmware as info, not warn --- openhcl/underhill_core/src/worker.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openhcl/underhill_core/src/worker.rs b/openhcl/underhill_core/src/worker.rs index 04f41ee1c67..a48af362a2f 100644 --- a/openhcl/underhill_core/src/worker.rs +++ b/openhcl/underhill_core/src/worker.rs @@ -3836,7 +3836,7 @@ async fn new_underhill_vm( firmware_size, device_size, }) => { - tracing::warn!( + tracing::info!( CVM_ALLOWED, firmware_size, device_size, From 2698d6e304c4a76e2643485a71b33ee9acaf78fd Mon Sep 17 00:00:00 2001 From: Mike Ebersol Date: Thu, 20 Aug 2026 13:53:38 -0700 Subject: [PATCH 06/13] openhcl: skip re-storing firmware after VMGS restore; quiet missing-image case - Don't re-snapshot the firmware to VMGS when it was just restored from VMGS on resume; the image is already saved there. - Treat an absent HIBERNATION_FIRMWARE file as the usual no-stored-image case and fall back without logging a warning; only warn on genuine read errors. --- openhcl/underhill_core/src/worker.rs | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/openhcl/underhill_core/src/worker.rs b/openhcl/underhill_core/src/worker.rs index a48af362a2f..836cf48924f 100644 --- a/openhcl/underhill_core/src/worker.rs +++ b/openhcl/underhill_core/src/worker.rs @@ -3702,6 +3702,10 @@ async fn new_underhill_vm( // firmware is overloaded (via the GET `LoadFirmware` host request) with the // hibernated version before it runs; on success that version becomes the // recorded token. + // + // Set when a hibernated firmware image is restored from VMGS on resume, so + // the cold-boot snapshot below can skip re-storing an image already in VMGS. + let mut firmware_restored_from_vmgs = false; let current_hibernate_token = if !dps.general.hibernation_enabled || !matches!(firmware_type, FirmwareType::Uefi) { @@ -3742,6 +3746,7 @@ async fn new_underhill_vm( if restore_vtl0_firmware_from_vmgs(gm.vtl0(), &mut measured_vtl0_info, vmgs_client) .await { + firmware_restored_from_vmgs = true; tracing::info!( CVM_ALLOWED, resume = %token, @@ -3820,10 +3825,11 @@ async fn new_underhill_vm( // that cannot overload the firmware itself. This runs after any firmware // overload/restore above (so it captures the image the guest will actually // run) and before `write_uefi_config` (in `load_firmware`) layers the - // dynamic config on top, regardless of the resolved token. A servicing - // restore is skipped: its firmware is already running with dynamic config - // applied and is no longer pristine. - if dps.general.hibernation_enabled && !is_restoring { + // dynamic config on top, regardless of the resolved token. Skipped for a + // servicing restore (its firmware already has dynamic config applied and is + // no longer pristine) and when the image was just restored from VMGS (it is + // already saved there). + if dps.general.hibernation_enabled && !is_restoring && !firmware_restored_from_vmgs { if let (Some(uefi_info), Some(vmgs_client)) = ( measured_vtl0_info .as_ref() @@ -4322,11 +4328,16 @@ async fn restore_vtl0_firmware_from_vmgs( .await { Ok(firmware) => firmware, + // No stored image is the usual case (e.g. first hibernation, or the VMGS + // was too small to snapshot one); fall back without logging. + Err(vmgs_broker::VmgsClientError::Vmgs( + vmgs_broker::VmgsBrokerError::FileInfoNotAllocated, + )) => return false, Err(err) => { tracing::warn!( CVM_ALLOWED, error = &err as &dyn std::error::Error, - "no UEFI firmware image to restore from VMGS on hibernation resume" + "failed to read UEFI firmware image from VMGS on hibernation resume" ); return false; } From 7b745043095c544bb6690fbe081212e76531f23d Mon Sep 17 00:00:00 2001 From: Mike Ebersol Date: Thu, 20 Aug 2026 14:04:46 -0700 Subject: [PATCH 07/13] openhcl: harden firmware restore against corrupt VMGS input (PR review) Addresses PR #4294 review comments: - loader::uefi::get_sec_entry_point_offset (and its PE helper) now use bounds-checked slicing, checked arithmetic, and zero-size guards so a corrupt/untrusted firmware image returns None instead of panicking or looping. - restore_vtl0_firmware_from_vmgs validates the restored image's SEC entry point is within the firmware region and does not overflow before rebasing RIP, mirroring overload_vtl0_firmware. - Vmgs::device_size() delegates to the (now overflow-safe, saturating) storage capacity(), clamped to VMGS_MAX_CAPACITY_BYTES. --- openhcl/underhill_core/src/worker.rs | 20 ++++--- vm/loader/src/uefi/mod.rs | 80 ++++++++++++++++------------ vm/vmgs/vmgs/src/storage.rs | 6 ++- vm/vmgs/vmgs/src/vmgs_impl.rs | 5 +- 4 files changed, 66 insertions(+), 45 deletions(-) diff --git a/openhcl/underhill_core/src/worker.rs b/openhcl/underhill_core/src/worker.rs index 836cf48924f..1899b1a7894 100644 --- a/openhcl/underhill_core/src/worker.rs +++ b/openhcl/underhill_core/src/worker.rs @@ -4357,17 +4357,25 @@ async fn restore_vtl0_firmware_from_vmgs( // On x86_64, compute the restored image's SEC entry point before touching // guest memory so a malformed image is a clean fallback (nothing written). + // The image is untrusted (may be corrupt): the entry point must land inside + // the firmware region and must not overflow. #[cfg(guest_arch = "x86_64")] - let new_rip = match loader::uefi::get_sec_entry_point_offset(&firmware) { - Some(offset) => firmware_memory.start() + offset, - None => { + let new_rip = { + let Some(new_rip) = + loader::uefi::get_sec_entry_point_offset(&firmware).and_then(|offset| { + firmware_memory + .start() + .checked_add(offset) + .filter(|_| offset < region_len) + }) + else { tracing::warn!( CVM_ALLOWED, - "could not find a SEC entry point in the stored UEFI firmware image; \ - not restoring" + "stored UEFI firmware image has no valid SEC entry point; not restoring" ); return false; - } + }; + new_rip }; if let Err(err) = gm.write_at(firmware_memory.start(), &firmware) { diff --git a/vm/loader/src/uefi/mod.rs b/vm/loader/src/uefi/mod.rs index fffd98f0b5e..fb5f7800129 100644 --- a/vm/loader/src/uefi/mod.rs +++ b/vm/loader/src/uefi/mod.rs @@ -150,27 +150,25 @@ fn pe_get_entry_point_offset(pe32_data: &[u8]) -> Option { let dos_header = ImageDosHeader::read_from_prefix(pe32_data).ok()?.0; // TODO: zerocopy: use-rest-of-range, option-to-error (https://github.com/microsoft/openvmm/issues/759) let nt_headers_offset = if dos_header.e_magic == IMAGE_DOS_SIGNATURE { // DOS image header is present, so read the PE header after the DOS image header. - dos_header.e_lfanew as usize + usize::try_from(dos_header.e_lfanew).ok()? } else { // DOS image header is not present, so PE header is at the image base. 0 }; - let signature = u32::read_from_prefix(&pe32_data[nt_headers_offset..]) - .ok()? - .0; // TODO: zerocopy: use-rest-of-range, option-to-error (https://github.com/microsoft/openvmm/issues/759) + // Bounds-checked: the image may be corrupt/untrusted (e.g. restored from VMGS). + let nt_headers = pe32_data.get(nt_headers_offset..)?; + let signature = u32::read_from_prefix(nt_headers).ok()?.0; // TODO: zerocopy: use-rest-of-range, option-to-error (https://github.com/microsoft/openvmm/issues/759) // Calculate the entry point relative to the start of the image. // AddressOfEntryPoint is common for PE32 & PE32+ if signature as u16 == TE_IMAGE_HEADER_SIGNATURE { - let te = TeImageHeader::read_from_prefix(&pe32_data[nt_headers_offset..]) - .ok()? - .0; // TODO: zerocopy: use-rest-of-range, option-to-error (https://github.com/microsoft/openvmm/issues/759) - Some(te.address_of_entry_point + size_of_val(&te) as u32 - te.stripped_size as u32) + let te = TeImageHeader::read_from_prefix(nt_headers).ok()?.0; // TODO: zerocopy: use-rest-of-range, option-to-error (https://github.com/microsoft/openvmm/issues/759) + te.address_of_entry_point + .checked_add(size_of_val(&te) as u32)? + .checked_sub(te.stripped_size as u32) } else if signature == IMAGE_NT_SIGNATURE { - let pe = ImageNtHeaders32::read_from_prefix(&pe32_data[nt_headers_offset..]) - .ok()? - .0; // TODO: zerocopy: use-rest-of-range, option-to-error (https://github.com/microsoft/openvmm/issues/759) + let pe = ImageNtHeaders32::read_from_prefix(nt_headers).ok()?.0; // TODO: zerocopy: use-rest-of-range, option-to-error (https://github.com/microsoft/openvmm/issues/759) Some(pe.optional_header.address_of_entry_point) } else { None @@ -221,12 +219,16 @@ const EFI_SECTION_PE32: u8 = 0x10; /// hibernated firmware image) can recompute the entry point from the new image. /// Returns `None` if the image has no recognizable SEC firmware volume / entry /// point. +/// +/// All indexing is bounds-checked and arithmetic overflow-checked so a +/// malformed/corrupt image (e.g. restored from VMGS) returns `None` rather than +/// panicking or looping. pub fn get_sec_entry_point_offset(image: &[u8]) -> Option { // Skip to SEC volume start. let mut image_offset = SEC_FIRMWARE_VOLUME_OFFSET; // Expect a firmware volume header for SEC volume. - let fvh = EFI_FIRMWARE_VOLUME_HEADER::read_from_prefix(&image[image_offset as usize..]) + let fvh = EFI_FIRMWARE_VOLUME_HEADER::read_from_prefix(image.get(image_offset as usize..)?) .ok()? .0; // TODO: zerocopy: use-rest-of-range, option-to-error (https://github.com/microsoft/openvmm/issues/759) if fvh.signature != EFI_FVH_SIGNATURE { @@ -234,18 +236,17 @@ pub fn get_sec_entry_point_offset(image: &[u8]) -> Option { } // Skip past firmware volume header to beginning of firmware volume. - image_offset += fvh.header_length as u64; + image_offset = image_offset.checked_add(fvh.header_length as u64)?; // Find the first SEC CORE file type. let mut sec_core_file_header = None; let mut volume_offset = 0; while volume_offset < fvh.fv_length { - let new_volume_offset = (volume_offset + 7) & !7; - if new_volume_offset > volume_offset { - image_offset += new_volume_offset - volume_offset; - volume_offset = new_volume_offset; - } - let fh = EFI_FFS_FILE_HEADER::read_from_prefix(&image[image_offset as usize..]) + let new_volume_offset = volume_offset.checked_add(7)? & !7; + image_offset = image_offset.checked_add(new_volume_offset - volume_offset)?; + volume_offset = new_volume_offset; + + let fh = EFI_FFS_FILE_HEADER::read_from_prefix(image.get(image_offset as usize..)?) .ok()? .0; // TODO: zerocopy: use-rest-of-range, option-to-error (https://github.com/microsoft/openvmm/issues/759) if fh.typ == EFI_FV_FILETYPE_SECURITY_CORE { @@ -253,8 +254,13 @@ pub fn get_sec_entry_point_offset(image: &[u8]) -> Option { break; } - image_offset += expand_3byte_integer(fh.size); - volume_offset += expand_3byte_integer(fh.size); + // A zero-size file would not advance the scan; treat as malformed. + let file_size = expand_3byte_integer(fh.size); + if file_size == 0 { + return None; + } + image_offset = image_offset.checked_add(file_size)?; + volume_offset = volume_offset.checked_add(file_size)?; } // There should always be a Security Core file. @@ -262,8 +268,9 @@ pub fn get_sec_entry_point_offset(image: &[u8]) -> Option { let sec_core_file_size = expand_3byte_integer(sec_core_file_header.size); // Move past the firmware file header. - image_offset += size_of::() as u64; - volume_offset += size_of::() as u64; + let file_header_size = size_of::() as u64; + image_offset = image_offset.checked_add(file_header_size)?; + volume_offset = volume_offset.checked_add(file_header_size)?; // Loop through the firmware file sections looking for PE section. let mut file_offset = volume_offset; @@ -271,24 +278,27 @@ pub fn get_sec_entry_point_offset(image: &[u8]) -> Option { // // Section headers are 8 byte aligned with respect to the beginning of the file stream. // - let new_file_offset = (file_offset + 3) & !3; - if new_file_offset > file_offset { - image_offset += new_file_offset - file_offset; - file_offset += new_file_offset - file_offset; - } + let new_file_offset = file_offset.checked_add(3)? & !3; + image_offset = image_offset.checked_add(new_file_offset - file_offset)?; + file_offset = new_file_offset; - let sh = EFI_COMMON_SECTION_HEADER::read_from_prefix(&image[image_offset as usize..]) + let sh = EFI_COMMON_SECTION_HEADER::read_from_prefix(image.get(image_offset as usize..)?) .ok()? .0; // TODO: zerocopy: use-rest-of-range, option-to-error (https://github.com/microsoft/openvmm/issues/759) if sh.typ == EFI_SECTION_PE32 { - let pe_offset = pe_get_entry_point_offset( - &image[image_offset as usize + size_of::()..], - )?; - image_offset += size_of::() as u64 + pe_offset as u64; + let section_header_size = size_of::() as u64; + let pe_data_offset = image_offset.checked_add(section_header_size)?; + let pe_offset = pe_get_entry_point_offset(image.get(pe_data_offset as usize..)?)?; + image_offset = pe_data_offset.checked_add(pe_offset as u64)?; break; } - image_offset += expand_3byte_integer(sh.size); - file_offset += expand_3byte_integer(sh.size); + // A zero-size section would not advance the scan; treat as malformed. + let section_size = expand_3byte_integer(sh.size); + if section_size == 0 { + return None; + } + image_offset = image_offset.checked_add(section_size)?; + file_offset = file_offset.checked_add(section_size)?; } Some(image_offset) diff --git a/vm/vmgs/vmgs/src/storage.rs b/vm/vmgs/vmgs/src/storage.rs index 8f252a2e9cd..71720502631 100644 --- a/vm/vmgs/vmgs/src/storage.rs +++ b/vm/vmgs/vmgs/src/storage.rs @@ -160,8 +160,10 @@ impl VmgsStorage { self.disk.sector_count() } - fn capacity(&self) -> u64 { - (self.sector_count() * self.sector_size() as u64).min(vmgs_format::VMGS_MAX_CAPACITY_BYTES) + pub fn capacity(&self) -> u64 { + self.sector_count() + .saturating_mul(self.sector_size() as u64) + .min(vmgs_format::VMGS_MAX_CAPACITY_BYTES) } /// Capacity in VMGS blocks. diff --git a/vm/vmgs/vmgs/src/vmgs_impl.rs b/vm/vmgs/vmgs/src/vmgs_impl.rs index 45040323f1b..3bb1a5bc93f 100644 --- a/vm/vmgs/vmgs/src/vmgs_impl.rs +++ b/vm/vmgs/vmgs/src/vmgs_impl.rs @@ -641,9 +641,10 @@ impl Vmgs { Ok(vmgs) } - /// Returns the total size, in bytes, of the underlying VMGS backing store. + /// Returns the total size, in bytes, of the underlying VMGS backing store, + /// clamped to the maximum VMGS capacity. pub fn device_size(&self) -> u64 { - self.storage.sector_count() * self.storage.sector_size() as u64 + self.storage.capacity() } /// Get allocated and valid bytes from File Control Block for file_id. From 9233177fa65cb0f8c49843feb3fa319150c6998a Mon Sep 17 00:00:00 2001 From: Mike Ebersol Date: Thu, 20 Aug 2026 16:31:58 -0700 Subject: [PATCH 08/13] loader: trim get_sec_entry_point_offset doc comment --- vm/loader/src/uefi/mod.rs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/vm/loader/src/uefi/mod.rs b/vm/loader/src/uefi/mod.rs index fb5f7800129..d23b844692e 100644 --- a/vm/loader/src/uefi/mod.rs +++ b/vm/loader/src/uefi/mod.rs @@ -219,10 +219,6 @@ const EFI_SECTION_PE32: u8 = 0x10; /// hibernated firmware image) can recompute the entry point from the new image. /// Returns `None` if the image has no recognizable SEC firmware volume / entry /// point. -/// -/// All indexing is bounds-checked and arithmetic overflow-checked so a -/// malformed/corrupt image (e.g. restored from VMGS) returns `None` rather than -/// panicking or looping. pub fn get_sec_entry_point_offset(image: &[u8]) -> Option { // Skip to SEC volume start. let mut image_offset = SEC_FIRMWARE_VOLUME_OFFSET; From e23b1dca2b5cdb004225011a9055304710ebf7e2 Mon Sep 17 00:00:00 2001 From: Mike Ebersol Date: Thu, 20 Aug 2026 16:33:43 -0700 Subject: [PATCH 09/13] openhcl: rename firmware_restored_from_vmgs to firmware_loaded_from_vmgs --- openhcl/underhill_core/src/worker.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/openhcl/underhill_core/src/worker.rs b/openhcl/underhill_core/src/worker.rs index 1899b1a7894..a6edb457ed5 100644 --- a/openhcl/underhill_core/src/worker.rs +++ b/openhcl/underhill_core/src/worker.rs @@ -3705,7 +3705,7 @@ async fn new_underhill_vm( // // Set when a hibernated firmware image is restored from VMGS on resume, so // the cold-boot snapshot below can skip re-storing an image already in VMGS. - let mut firmware_restored_from_vmgs = false; + let mut firmware_loaded_from_vmgs = false; let current_hibernate_token = if !dps.general.hibernation_enabled || !matches!(firmware_type, FirmwareType::Uefi) { @@ -3746,7 +3746,7 @@ async fn new_underhill_vm( if restore_vtl0_firmware_from_vmgs(gm.vtl0(), &mut measured_vtl0_info, vmgs_client) .await { - firmware_restored_from_vmgs = true; + firmware_loaded_from_vmgs = true; tracing::info!( CVM_ALLOWED, resume = %token, @@ -3829,7 +3829,7 @@ async fn new_underhill_vm( // servicing restore (its firmware already has dynamic config applied and is // no longer pristine) and when the image was just restored from VMGS (it is // already saved there). - if dps.general.hibernation_enabled && !is_restoring && !firmware_restored_from_vmgs { + if dps.general.hibernation_enabled && !is_restoring && !firmware_loaded_from_vmgs { if let (Some(uefi_info), Some(vmgs_client)) = ( measured_vtl0_info .as_ref() From 91cfd7c2d77acb72f0b75184bbff625005d76827 Mon Sep 17 00:00:00 2001 From: Mike Ebersol Date: Thu, 20 Aug 2026 16:41:39 -0700 Subject: [PATCH 10/13] openhcl: try VMGS firmware restore first for any resume token; delete image on power off/reset - For any hibernated/unrecognized resume token, attempt restore_vtl0_firmware_from_vmgs first; only on failure fall back to the version-based logic (overload, then current firmware). NotHibernated / no token skip the load entirely. - Delete the HIBERNATION_FIRMWARE image on power off / reset so a later boot cannot restore a stale image. --- openhcl/underhill_core/src/worker.rs | 156 ++++++++++++++++----------- 1 file changed, 96 insertions(+), 60 deletions(-) diff --git a/openhcl/underhill_core/src/worker.rs b/openhcl/underhill_core/src/worker.rs index a6edb457ed5..cfff851c0fa 100644 --- a/openhcl/underhill_core/src/worker.rs +++ b/openhcl/underhill_core/src/worker.rs @@ -3735,14 +3735,15 @@ async fn new_underhill_vm( // Consume any token, valid or corrupt, so it is not re-read next boot. hibernate::delete_token(vmgs_client).await; match resume_token { - Some(token @ hibernate::Token::Hibernated { .. }) - if token != hibernate::Token::CURRENT => - { + // Clean prior power-off (NotHibernated) or no/unreadable token + // (None): a normal cold boot on the current firmware; no load needed. + Some(hibernate::Token::NotHibernated) | None => Some(hibernate::Token::CURRENT), + Some(token) => { // Prefer restoring the exact firmware image snapshotted to VMGS // at the original cold boot: it reproduces the hibernated - // firmware bit-for-bit without needing host support. Fall back to - // a host firmware overload if no usable image is stored, then to - // resuming on the current firmware. + // firmware bit-for-bit without needing host support. Only if no + // usable image is stored do we fall back to the version-based + // logic (host overload, then the current firmware). if restore_vtl0_firmware_from_vmgs(gm.vtl0(), &mut measured_vtl0_info, vmgs_client) .await { @@ -3750,65 +3751,72 @@ async fn new_underhill_vm( tracing::info!( CVM_ALLOWED, resume = %token, - "hibernation resume under a different firmware version; \ - restored the hibernated firmware image from VMGS" + "restored the hibernated UEFI firmware image from VMGS" ); Some(token) - } else if dps - .general - .management_vtl_features - .load_firmware_supported() - { - tracing::info!( - CVM_ALLOWED, - resume = %token, - current = %hibernate::Token::CURRENT, - "hibernation resume under a different firmware version; requesting firmware overload" - ); - // If the overload succeeds VTL0 now runs the hibernated - // version, so record it; otherwise fall back to the current one. - if overload_vtl0_firmware( - &get_client, - u64::from(token), - &mut measured_vtl0_info, - ) - .await - { - Some(token) - } else { - Some(hibernate::Token::CURRENT) - } } else { - tracing::warn!( - CVM_ALLOWED, - resume = %token, - "no stored firmware image and host does not support firmware \ - overload (LoadFirmware); resuming with the current firmware \ - version despite a hibernation token mismatch" - ); - Some(hibernate::Token::CURRENT) + match token { + token @ hibernate::Token::Hibernated { .. } + if token != hibernate::Token::CURRENT => + { + if dps + .general + .management_vtl_features + .load_firmware_supported() + { + tracing::info!( + CVM_ALLOWED, + resume = %token, + current = %hibernate::Token::CURRENT, + "hibernation resume under a different firmware version; requesting firmware overload" + ); + // If the overload succeeds VTL0 now runs the + // hibernated version, so record it; otherwise fall + // back to the current one. + if overload_vtl0_firmware( + &get_client, + u64::from(token), + &mut measured_vtl0_info, + ) + .await + { + Some(token) + } else { + Some(hibernate::Token::CURRENT) + } + } else { + tracing::warn!( + CVM_ALLOWED, + resume = %token, + "no stored firmware image and host does not support firmware \ + overload (LoadFirmware); resuming with the current firmware \ + version despite a hibernation token mismatch" + ); + Some(hibernate::Token::CURRENT) + } + } + hibernate::Token::Hibernated { .. } => { + // The token matches the current firmware version. + tracing::info!( + CVM_ALLOWED, + "hibernation resume under the current firmware version" + ); + Some(hibernate::Token::CURRENT) + } + hibernate::Token::Other(raw) => { + // An out-of-range/corrupt token value; ignore it. + tracing::warn!( + CVM_ALLOWED, + raw, + "ignoring unrecognized hibernate token; using the current firmware version" + ); + Some(hibernate::Token::CURRENT) + } + // NotHibernated is handled by the outer arm. + hibernate::Token::NotHibernated => Some(hibernate::Token::CURRENT), + } } } - Some(hibernate::Token::Hibernated { .. }) => { - // Guarded above: the token matches the current firmware version. - tracing::info!( - CVM_ALLOWED, - "hibernation resume under the current firmware version" - ); - Some(hibernate::Token::CURRENT) - } - Some(hibernate::Token::Other(raw)) => { - // An out-of-range/corrupt token value; ignore it. - tracing::warn!( - CVM_ALLOWED, - raw, - "ignoring unrecognized hibernate token; using the current firmware version" - ); - Some(hibernate::Token::CURRENT) - } - // Clean prior power-off, or no/unreadable token (e.g. first boot): - // a normal cold boot on the current firmware. - Some(hibernate::Token::NotHibernated) | None => Some(hibernate::Token::CURRENT), } } else { // Hibernation was requested but there is no VMGS to persist the token, @@ -4191,6 +4199,9 @@ async fn halt_task( hibernate::Token::NotHibernated, ) .await; + // Drop any stored firmware image so a later boot does not + // restore a stale one. + delete_firmware_from_vmgs(&hibernate_halt.vmgs_client).await; } get_client.send_power_off() } @@ -4202,6 +4213,9 @@ async fn halt_task( hibernate::Token::NotHibernated, ) .await; + // Drop any stored firmware image so a later boot does not + // restore a stale one. + delete_firmware_from_vmgs(&hibernate_halt.vmgs_client).await; } get_client.send_reset() } @@ -4298,6 +4312,28 @@ async fn store_firmware_to_vmgs( Ok(StoreFirmwareOutcome::Stored) } +/// Best-effort deletion of any stored hibernation firmware image, used on a +/// clean power off / reset so a later boot does not restore a stale image. A +/// missing image is the common case and is not logged. +async fn delete_firmware_from_vmgs(vmgs_client: &vmgs_broker::VmgsClient) { + match vmgs_client + .delete_file(vmgs::FileId::HIBERNATION_FIRMWARE) + .await + { + Ok(()) + | Err(vmgs_broker::VmgsClientError::Vmgs( + vmgs_broker::VmgsBrokerError::FileInfoNotAllocated, + )) => {} + Err(err) => { + tracing::error!( + CVM_ALLOWED, + error = &err as &dyn std::error::Error, + "failed to delete stored UEFI firmware image from VMGS" + ); + } + } +} + /// Restores a previously stored UEFI firmware image from VMGS back into VTL0 /// guest memory when resuming a hibernated guest, so the firmware binary matches /// what was present when the guest hibernated. Preferred over a host firmware From 03057e2d0c523beb023fe6d0c2a71254ed3998df Mon Sep 17 00:00:00 2001 From: Mike Ebersol Date: Thu, 20 Aug 2026 17:04:35 -0700 Subject: [PATCH 11/13] openhcl: move firmware VMGS store/read/delete helpers into hibernate module Move the VMGS hibernation-state helpers next to the token helpers: - StoreFirmwareOutcome, store_firmware, read_firmware, delete_firmware now live in hibernate.rs (with unit tests for the store/read/delete round-trip). - worker.rs keeps the VTL0/loader wiring (restore_vtl0_firmware_from_vmgs and set_vtl0_uefi_rip), calling hibernate::read_firmware for the bytes. --- openhcl/underhill_core/src/hibernate.rs | 157 +++++++++++++++++++++++- openhcl/underhill_core/src/worker.rs | 117 ++---------------- 2 files changed, 163 insertions(+), 111 deletions(-) diff --git a/openhcl/underhill_core/src/hibernate.rs b/openhcl/underhill_core/src/hibernate.rs index d8e395e678e..838ca0887f9 100644 --- a/openhcl/underhill_core/src/hibernate.rs +++ b/openhcl/underhill_core/src/hibernate.rs @@ -1,14 +1,19 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. -//! OpenHCL hibernate token handling: the [`Token`] recorded in -//! [`vmgs::FileId::HIBERNATION_TOKEN`] and helpers to read, write, and delete -//! it. +//! OpenHCL hibernate state stored in VMGS: the [`Token`] recorded in +//! [`vmgs::FileId::HIBERNATION_TOKEN`] and the UEFI firmware image snapshot in +//! [`vmgs::FileId::HIBERNATION_FIRMWARE`], with helpers to read, write, and +//! delete both. +use anyhow::Context; use cvm_tracing::CVM_ALLOWED; +use guestmem::GuestMemory; +use memory_range::MemoryRange; use std::fmt; use vmgs_broker::VmgsBrokerError; use vmgs_broker::VmgsClientError; +use vmgs_format::VMGS_HIBERNATION_FIRMWARE_MIN_SIZE; /// The hibernate marker recorded in [`vmgs::FileId::HIBERNATION_TOKEN`], /// decoupled from its on-disk encoding. @@ -154,6 +159,110 @@ pub async fn read_token(vmgs_client: &vmgs_broker::VmgsClient) -> Option } } +/// The result of attempting to store a firmware image into VMGS for hibernation. +pub enum StoreFirmwareOutcome { + /// The firmware image was stored to VMGS. + Stored, + /// The VMGS backing store is too small to hold the firmware image. This is + /// not an error: hibernation falls back to not preserving the firmware + /// image across resume. + InsufficientSpace { + firmware_size: u64, + device_size: u64, + }, +} + +/// Stores the pristine UEFI firmware image out of VTL0 guest memory and into +/// [`vmgs::FileId::HIBERNATION_FIRMWARE`] so it can be restored identically on a +/// hibernation resume where the host cannot overload the firmware itself. +/// +/// This must be called *before* any dynamic configuration is written into the +/// firmware region (i.e. before `write_uefi_config`), so that the stored image +/// matches the measured firmware as loaded from the IGVM file. +/// +/// The image is only stored if the VMGS backing store is large enough to hold +/// it; otherwise [`StoreFirmwareOutcome::InsufficientSpace`] is returned and the +/// caller falls back to not preserving the firmware across hibernation. An +/// `Err` is only returned for an actual failure to query or write VMGS. +pub async fn store_firmware( + gm: &GuestMemory, + firmware_memory: MemoryRange, + vmgs_client: &vmgs_broker::VmgsClient, +) -> anyhow::Result { + let len = firmware_memory.len(); + + // The firmware image can only be stored if the VMGS backing store is large + // enough. Require a minimum overall size so there is room for the firmware + // image alongside the other VMGS files, and also verify the image fits. + // Insufficient space is a normal fallback, not an error. + let device_size = vmgs_client + .device_size() + .await + .context("failed to query VMGS size")?; + if device_size < VMGS_HIBERNATION_FIRMWARE_MIN_SIZE || len > device_size { + return Ok(StoreFirmwareOutcome::InsufficientSpace { + firmware_size: len, + device_size, + }); + } + + let firmware_len = usize::try_from(len).context("firmware image size does not fit in usize")?; + let mut firmware = vec![0u8; firmware_len]; + gm.read_at(firmware_memory.start(), &mut firmware) + .context("failed to read UEFI firmware image from VTL0 memory")?; + vmgs_client + .write_file(vmgs::FileId::HIBERNATION_FIRMWARE, firmware) + .await + .context("failed to write UEFI firmware snapshot to VMGS")?; + tracing::info!( + CVM_ALLOWED, + size_bytes = len, + "stored UEFI firmware image to VMGS for hibernation" + ); + Ok(StoreFirmwareOutcome::Stored) +} + +/// Reads a stored UEFI firmware image from VMGS, returning `None` if none is +/// stored (the usual case, e.g. first hibernation or a VMGS too small to hold +/// one) or the read fails. +pub async fn read_firmware(vmgs_client: &vmgs_broker::VmgsClient) -> Option> { + match vmgs_client + .read_file(vmgs::FileId::HIBERNATION_FIRMWARE) + .await + { + Ok(firmware) => Some(firmware), + // No stored image is the usual case; not an error. + Err(VmgsClientError::Vmgs(VmgsBrokerError::FileInfoNotAllocated)) => None, + Err(err) => { + tracing::warn!( + CVM_ALLOWED, + error = &err as &dyn std::error::Error, + "failed to read UEFI firmware image from VMGS on hibernation resume" + ); + None + } + } +} + +/// Best-effort deletion of any stored hibernation firmware image, used on a +/// clean power off / reset so a later boot does not restore a stale image. A +/// missing image is the common case and is not logged. +pub async fn delete_firmware(vmgs_client: &vmgs_broker::VmgsClient) { + match vmgs_client + .delete_file(vmgs::FileId::HIBERNATION_FIRMWARE) + .await + { + Ok(()) | Err(VmgsClientError::Vmgs(VmgsBrokerError::FileInfoNotAllocated)) => {} + Err(err) => { + tracing::error!( + CVM_ALLOWED, + error = &err as &dyn std::error::Error, + "failed to delete stored UEFI firmware image from VMGS" + ); + } + } +} + #[cfg(test)] mod tests { use super::*; @@ -193,6 +302,48 @@ mod tests { assert_eq!(read_token(&client).await, Some(Token::NotHibernated)); } + #[async_test] + async fn firmware_store_read_delete(driver: DefaultDriver) { + // The store is gated on VMGS_HIBERNATION_FIRMWARE_MIN_SIZE, so use a + // backing store larger than that minimum. + let disk = ram_disk(48 * 1024 * 1024, false).unwrap(); + let vmgs = Vmgs::format_new(disk, None).await.unwrap(); + let (client, _task) = spawn_vmgs_broker(driver.clone(), vmgs); + + // No image stored initially. + assert!(read_firmware(&client).await.is_none()); + + let gm = GuestMemory::allocate(0x2000); + let firmware_memory = MemoryRange::new(0x1000..0x2000); + let image: Vec = (0..0x1000u32).map(|i| i as u8).collect(); + gm.write_at(firmware_memory.start(), &image).unwrap(); + + assert!(matches!( + store_firmware(&gm, firmware_memory, &client).await.unwrap(), + StoreFirmwareOutcome::Stored + )); + assert_eq!( + read_firmware(&client).await.as_deref(), + Some(image.as_slice()) + ); + + delete_firmware(&client).await; + assert!(read_firmware(&client).await.is_none()); + } + + #[async_test] + async fn firmware_store_insufficient_space(driver: DefaultDriver) { + // `new_client` uses a 4 MB disk, below VMGS_HIBERNATION_FIRMWARE_MIN_SIZE. + let (client, _task) = new_client(&driver).await; + let gm = GuestMemory::allocate(0x1000); + let firmware_memory = MemoryRange::new(0..0x1000); + assert!(matches!( + store_firmware(&gm, firmware_memory, &client).await.unwrap(), + StoreFirmwareOutcome::InsufficientSpace { .. } + )); + assert!(read_firmware(&client).await.is_none()); + } + #[test] fn constants_encode_as_expected() { assert_eq!(u64::from(Token::NotHibernated), 0); diff --git a/openhcl/underhill_core/src/worker.rs b/openhcl/underhill_core/src/worker.rs index cfff851c0fa..baf99b50821 100644 --- a/openhcl/underhill_core/src/worker.rs +++ b/openhcl/underhill_core/src/worker.rs @@ -3844,9 +3844,10 @@ async fn new_underhill_vm( .and_then(|info| info.supports_uefi.as_ref()), vmgs_client.as_ref(), ) { - match store_firmware_to_vmgs(gm.vtl0(), uefi_info.firmware_memory, vmgs_client).await { - Ok(StoreFirmwareOutcome::Stored) => {} - Ok(StoreFirmwareOutcome::InsufficientSpace { + match hibernate::store_firmware(gm.vtl0(), uefi_info.firmware_memory, vmgs_client).await + { + Ok(hibernate::StoreFirmwareOutcome::Stored) => {} + Ok(hibernate::StoreFirmwareOutcome::InsufficientSpace { firmware_size, device_size, }) => { @@ -4201,7 +4202,7 @@ async fn halt_task( .await; // Drop any stored firmware image so a later boot does not // restore a stale one. - delete_firmware_from_vmgs(&hibernate_halt.vmgs_client).await; + hibernate::delete_firmware(&hibernate_halt.vmgs_client).await; } get_client.send_power_off() } @@ -4215,7 +4216,7 @@ async fn halt_task( .await; // Drop any stored firmware image so a later boot does not // restore a stale one. - delete_firmware_from_vmgs(&hibernate_halt.vmgs_client).await; + hibernate::delete_firmware(&hibernate_halt.vmgs_client).await; } get_client.send_reset() } @@ -4249,91 +4250,6 @@ async fn wait_for_flush_logs(control_send: &Arc anyhow::Result { - let len = firmware_memory.len(); - - // The firmware image can only be stored if the VMGS backing store is large - // enough. Require a minimum overall size so there is room for the firmware - // image alongside the other VMGS files, and also verify the image fits. - // Insufficient space is a normal fallback, not an error. - let device_size = vmgs_client - .device_size() - .await - .context("failed to query VMGS size")?; - if device_size < VMGS_HIBERNATION_FIRMWARE_MIN_SIZE || len > device_size { - return Ok(StoreFirmwareOutcome::InsufficientSpace { - firmware_size: len, - device_size, - }); - } - - let firmware_len = usize::try_from(len).context("firmware image size does not fit in usize")?; - let mut firmware = vec![0u8; firmware_len]; - gm.read_at(firmware_memory.start(), &mut firmware) - .context("failed to read UEFI firmware image from VTL0 memory")?; - vmgs_client - .write_file(vmgs::FileId::HIBERNATION_FIRMWARE, firmware) - .await - .context("failed to write UEFI firmware snapshot to VMGS")?; - tracing::info!( - CVM_ALLOWED, - size_bytes = len, - "stored UEFI firmware image to VMGS for hibernation" - ); - Ok(StoreFirmwareOutcome::Stored) -} - -/// Best-effort deletion of any stored hibernation firmware image, used on a -/// clean power off / reset so a later boot does not restore a stale image. A -/// missing image is the common case and is not logged. -async fn delete_firmware_from_vmgs(vmgs_client: &vmgs_broker::VmgsClient) { - match vmgs_client - .delete_file(vmgs::FileId::HIBERNATION_FIRMWARE) - .await - { - Ok(()) - | Err(vmgs_broker::VmgsClientError::Vmgs( - vmgs_broker::VmgsBrokerError::FileInfoNotAllocated, - )) => {} - Err(err) => { - tracing::error!( - CVM_ALLOWED, - error = &err as &dyn std::error::Error, - "failed to delete stored UEFI firmware image from VMGS" - ); - } - } -} - /// Restores a previously stored UEFI firmware image from VMGS back into VTL0 /// guest memory when resuming a hibernated guest, so the firmware binary matches /// what was present when the guest hibernated. Preferred over a host firmware @@ -4359,24 +4275,9 @@ async fn restore_vtl0_firmware_from_vmgs( return false; }; - let firmware = match vmgs_client - .read_file(vmgs::FileId::HIBERNATION_FIRMWARE) - .await - { - Ok(firmware) => firmware, - // No stored image is the usual case (e.g. first hibernation, or the VMGS - // was too small to snapshot one); fall back without logging. - Err(vmgs_broker::VmgsClientError::Vmgs( - vmgs_broker::VmgsBrokerError::FileInfoNotAllocated, - )) => return false, - Err(err) => { - tracing::warn!( - CVM_ALLOWED, - error = &err as &dyn std::error::Error, - "failed to read UEFI firmware image from VMGS on hibernation resume" - ); - return false; - } + let Some(firmware) = hibernate::read_firmware(vmgs_client).await else { + // No usable stored image; leave the current firmware in place. + return false; }; let region_len = firmware_memory.len(); From eff92fa97861535a6d4816c1712558c72d642c42 Mon Sep 17 00:00:00 2001 From: Mike Ebersol Date: Thu, 20 Aug 2026 17:27:02 -0700 Subject: [PATCH 12/13] openhcl: move VMGS_HIBERNATION_FIRMWARE_MIN_SIZE to hibernate module It is a hibernation policy threshold, not a VMGS format invariant, and is only consumed by the hibernate module and its worker caller. --- openhcl/underhill_core/src/hibernate.rs | 7 ++++++- openhcl/underhill_core/src/worker.rs | 3 +-- vm/vmgs/vmgs_format/src/lib.rs | 6 ------ 3 files changed, 7 insertions(+), 9 deletions(-) diff --git a/openhcl/underhill_core/src/hibernate.rs b/openhcl/underhill_core/src/hibernate.rs index 838ca0887f9..2fe54667c22 100644 --- a/openhcl/underhill_core/src/hibernate.rs +++ b/openhcl/underhill_core/src/hibernate.rs @@ -13,7 +13,6 @@ use memory_range::MemoryRange; use std::fmt; use vmgs_broker::VmgsBrokerError; use vmgs_broker::VmgsClientError; -use vmgs_format::VMGS_HIBERNATION_FIRMWARE_MIN_SIZE; /// The hibernate marker recorded in [`vmgs::FileId::HIBERNATION_TOKEN`], /// decoupled from its on-disk encoding. @@ -159,6 +158,12 @@ pub async fn read_token(vmgs_client: &vmgs_broker::VmgsClient) -> Option } } +/// The minimum overall VMGS backing-store size, in bytes, required before a UEFI +/// firmware image snapshot ([`vmgs::FileId::HIBERNATION_FIRMWARE`]) is stored +/// for hibernation. A minimum overall size so the image fits alongside the other +/// VMGS files, not just a tight fit of the image itself. +pub const VMGS_HIBERNATION_FIRMWARE_MIN_SIZE: u64 = 32 * 1024 * 1024; + /// The result of attempting to store a firmware image into VMGS for hibernation. pub enum StoreFirmwareOutcome { /// The firmware image was stored to VMGS. diff --git a/openhcl/underhill_core/src/worker.rs b/openhcl/underhill_core/src/worker.rs index baf99b50821..930608f9d6e 100644 --- a/openhcl/underhill_core/src/worker.rs +++ b/openhcl/underhill_core/src/worker.rs @@ -169,7 +169,6 @@ use vmcore::vmtime::VmTime; use vmcore::vmtime::VmTimeKeeper; use vmgs::Vmgs; use vmgs_broker::spawn_vmgs_broker; -use vmgs_format::VMGS_HIBERNATION_FIRMWARE_MIN_SIZE; use vmgs_format::VmgsProvisioner; use vmgs_format::VmgsProvisioningMarker; use vmgs_format::VmgsProvisioningReason; @@ -3855,7 +3854,7 @@ async fn new_underhill_vm( CVM_ALLOWED, firmware_size, device_size, - minimum_size = VMGS_HIBERNATION_FIRMWARE_MIN_SIZE, + minimum_size = hibernate::VMGS_HIBERNATION_FIRMWARE_MIN_SIZE, "VMGS backing store too small to preserve UEFI firmware across hibernation" ); } diff --git a/vm/vmgs/vmgs_format/src/lib.rs b/vm/vmgs/vmgs_format/src/lib.rs index 9b7b52ae3dd..e8472f2b31f 100644 --- a/vm/vmgs/vmgs_format/src/lib.rs +++ b/vm/vmgs/vmgs_format/src/lib.rs @@ -70,12 +70,6 @@ impl Display for FileId { } } -/// The minimum overall VMGS backing-store size, in bytes, required before a UEFI -/// firmware image snapshot ([`FileId::HIBERNATION_FIRMWARE`]) is stored for -/// hibernation. This is a minimum overall size so that the firmware image fits -/// alongside the other VMGS files, not just a tight fit of the image itself. -pub const VMGS_HIBERNATION_FIRMWARE_MIN_SIZE: u64 = 32 * 1024 * 1024; - pub const VMGS_VERSION_2_0: u32 = 0x00020000; pub const VMGS_VERSION_3_0: u32 = 0x00030000; From 4ac5b9b5068f0a8631c11e12db2c6ce056cb4b84 Mon Sep 17 00:00:00 2001 From: Mike Ebersol Date: Thu, 20 Aug 2026 17:43:53 -0700 Subject: [PATCH 13/13] openhcl: address PR review (broker wire compat, SEC validation, firmware read size gate) - vmgs_broker: append the DeviceSize RPC variant so MeshPayload field numbers of existing variants are unchanged across revisions. - loader: get_sec_entry_point_offset rejects a zero-size SEC core file and requires a PE section be found, returning None otherwise. - hibernate::read_firmware takes an expected length and verifies it via get_file_info before read_file, avoiding a large allocation on a corrupt entry and folding in the exact-size check the restore path needs. --- openhcl/underhill_core/src/hibernate.rs | 53 ++++++++++++++++++++----- openhcl/underhill_core/src/worker.rs | 17 ++------ vm/loader/src/uefi/mod.rs | 14 +++++-- vm/vmgs/vmgs_broker/src/broker.rs | 4 +- 4 files changed, 62 insertions(+), 26 deletions(-) diff --git a/openhcl/underhill_core/src/hibernate.rs b/openhcl/underhill_core/src/hibernate.rs index 2fe54667c22..0428f948e7e 100644 --- a/openhcl/underhill_core/src/hibernate.rs +++ b/openhcl/underhill_core/src/hibernate.rs @@ -227,17 +227,50 @@ pub async fn store_firmware( Ok(StoreFirmwareOutcome::Stored) } -/// Reads a stored UEFI firmware image from VMGS, returning `None` if none is +/// Reads a stored UEFI firmware image from VMGS, returning `None` if no image is /// stored (the usual case, e.g. first hibernation or a VMGS too small to hold -/// one) or the read fails. -pub async fn read_firmware(vmgs_client: &vmgs_broker::VmgsClient) -> Option> { +/// one) or if the stored image's size is not exactly `expected_len`. The size is +/// checked via the file metadata *before* reading, so a corrupt or oversized +/// entry cannot force a large allocation. +pub async fn read_firmware( + vmgs_client: &vmgs_broker::VmgsClient, + expected_len: u64, +) -> Option> { + let info = match vmgs_client + .get_file_info(vmgs::FileId::HIBERNATION_FIRMWARE) + .await + { + Ok(info) => info, + // No stored image is the usual case; not an error. + Err(VmgsClientError::Vmgs(VmgsBrokerError::FileInfoNotAllocated)) => return None, + Err(err) => { + tracing::warn!( + CVM_ALLOWED, + error = &err as &dyn std::error::Error, + "failed to query stored UEFI firmware image metadata" + ); + return None; + } + }; + + // A resumed image is only usable if it is bit-for-bit the same size as the + // firmware region; requiring an exact match here also bounds the allocation + // `read_file` makes below. + if info.valid_bytes != expected_len { + tracing::warn!( + CVM_ALLOWED, + stored_size = info.valid_bytes, + expected_size = expected_len, + "stored UEFI firmware image size does not match the firmware region; not restoring" + ); + return None; + } + match vmgs_client .read_file(vmgs::FileId::HIBERNATION_FIRMWARE) .await { Ok(firmware) => Some(firmware), - // No stored image is the usual case; not an error. - Err(VmgsClientError::Vmgs(VmgsBrokerError::FileInfoNotAllocated)) => None, Err(err) => { tracing::warn!( CVM_ALLOWED, @@ -316,7 +349,7 @@ mod tests { let (client, _task) = spawn_vmgs_broker(driver.clone(), vmgs); // No image stored initially. - assert!(read_firmware(&client).await.is_none()); + assert!(read_firmware(&client, 0x1000).await.is_none()); let gm = GuestMemory::allocate(0x2000); let firmware_memory = MemoryRange::new(0x1000..0x2000); @@ -328,12 +361,14 @@ mod tests { StoreFirmwareOutcome::Stored )); assert_eq!( - read_firmware(&client).await.as_deref(), + read_firmware(&client, 0x1000).await.as_deref(), Some(image.as_slice()) ); + // A size mismatch is rejected without loading. + assert!(read_firmware(&client, 0x800).await.is_none()); delete_firmware(&client).await; - assert!(read_firmware(&client).await.is_none()); + assert!(read_firmware(&client, 0x1000).await.is_none()); } #[async_test] @@ -346,7 +381,7 @@ mod tests { store_firmware(&gm, firmware_memory, &client).await.unwrap(), StoreFirmwareOutcome::InsufficientSpace { .. } )); - assert!(read_firmware(&client).await.is_none()); + assert!(read_firmware(&client, 0x1000).await.is_none()); } #[test] diff --git a/openhcl/underhill_core/src/worker.rs b/openhcl/underhill_core/src/worker.rs index 930608f9d6e..61766af5dfd 100644 --- a/openhcl/underhill_core/src/worker.rs +++ b/openhcl/underhill_core/src/worker.rs @@ -4274,23 +4274,14 @@ async fn restore_vtl0_firmware_from_vmgs( return false; }; - let Some(firmware) = hibernate::read_firmware(vmgs_client).await else { + // `read_firmware` requires the stored image to be exactly `region_len`, so + // no separate size check is needed here. + let region_len = firmware_memory.len(); + let Some(firmware) = hibernate::read_firmware(vmgs_client, region_len).await else { // No usable stored image; leave the current firmware in place. return false; }; - let region_len = firmware_memory.len(); - if firmware.len() as u64 != region_len { - tracing::warn!( - CVM_ALLOWED, - stored_size = firmware.len(), - region_size = region_len, - "stored UEFI firmware image size does not match the firmware region; \ - not restoring" - ); - return false; - } - // On x86_64, compute the restored image's SEC entry point before touching // guest memory so a malformed image is a clean fallback (nothing written). // The image is untrusted (may be corrupt): the entry point must land inside diff --git a/vm/loader/src/uefi/mod.rs b/vm/loader/src/uefi/mod.rs index d23b844692e..73c3cc1b4ae 100644 --- a/vm/loader/src/uefi/mod.rs +++ b/vm/loader/src/uefi/mod.rs @@ -262,14 +262,21 @@ pub fn get_sec_entry_point_offset(image: &[u8]) -> Option { // There should always be a Security Core file. let sec_core_file_header = sec_core_file_header?; let sec_core_file_size = expand_3byte_integer(sec_core_file_header.size); + // A zero-size SEC file has no sections; reject it rather than returning an + // offset that is not an entry point. + if sec_core_file_size == 0 { + return None; + } // Move past the firmware file header. let file_header_size = size_of::() as u64; image_offset = image_offset.checked_add(file_header_size)?; volume_offset = volume_offset.checked_add(file_header_size)?; - // Loop through the firmware file sections looking for PE section. + // Loop through the firmware file sections looking for the PE section, which + // holds the entry point. let mut file_offset = volume_offset; + let mut pe_entry = None; while file_offset < sec_core_file_size { // // Section headers are 8 byte aligned with respect to the beginning of the file stream. @@ -285,7 +292,7 @@ pub fn get_sec_entry_point_offset(image: &[u8]) -> Option { let section_header_size = size_of::() as u64; let pe_data_offset = image_offset.checked_add(section_header_size)?; let pe_offset = pe_get_entry_point_offset(image.get(pe_data_offset as usize..)?)?; - image_offset = pe_data_offset.checked_add(pe_offset as u64)?; + pe_entry = Some(pe_data_offset.checked_add(pe_offset as u64)?); break; } // A zero-size section would not advance the scan; treat as malformed. @@ -297,7 +304,8 @@ pub fn get_sec_entry_point_offset(image: &[u8]) -> Option { file_offset = file_offset.checked_add(section_size)?; } - Some(image_offset) + // No PE section means no entry point. + pe_entry } /// Definitions shared by UEFI and the loader when loaded with parameters passed in IGVM format. diff --git a/vm/vmgs/vmgs_broker/src/broker.rs b/vm/vmgs/vmgs_broker/src/broker.rs index 8f0491f441b..cd9b0bf00b8 100644 --- a/vm/vmgs/vmgs_broker/src/broker.rs +++ b/vm/vmgs/vmgs_broker/src/broker.rs @@ -50,13 +50,15 @@ impl From for FileId { pub enum VmgsBrokerRpc { Inspect(inspect::Deferred), GetFileInfo(Rpc>), - DeviceSize(Rpc<(), u64>), ReadFile(Rpc, VmgsBrokerError>>), WriteFile(Rpc<(BrokerFileId, Vec), Result<(), VmgsBrokerError>>), #[cfg(feature = "encryption")] WriteFileEncrypted(Rpc<(BrokerFileId, Vec), Result<(), VmgsBrokerError>>), Save(Rpc<(), vmgs::save_restore::state::SavedVmgsState>), DeleteFile(Rpc>), + // N.B. `MeshPayload` numbers fields by declaration order; append new RPCs + // here so existing ones keep their wire numbers across revisions. + DeviceSize(Rpc<(), u64>), } pub struct VmgsBrokerTask {