From d154d3a8a1ef991b869e05bf049bc56d5a0dd2a1 Mon Sep 17 00:00:00 2001 From: Michal Tarnacki Date: Wed, 24 Jun 2026 10:38:32 +0200 Subject: [PATCH] fix(migtd): harden BAR handling, logging, and collateral fetches Address several remaining audit findings across the pci, vsock, migtd, collateral-generator and AzCVMEmu support code. - pci BAR handling: fail closed when BAR sizing returns 0 instead of falling back to the raw host-chosen BAR address, and use wrapping arithmetic in get_bar_size so malformed sizing values resolve to 0 without panicking in debug builds. - vsock accept: initialize peer flow-control fields from the incoming request packet rather than the locally generated response packet. - migration logging: free previously allocated shared and provisional log pages if create_logarea fails mid-loop, and cap VMM-controlled runtime/provisional log verbosity to Info in release builds so a hostile host cannot enable Debug or Trace logging into the shared log area and harvest sensitive records. - collateral generator: fetch the root CA CRL through the checked helper that enforces HTTP 200 instead of enrolling arbitrary error bodies as CRL data. - AzCVMEmu tdcall_report: add a compile-time size assertion for the source AzTdReport layout so upstream drift fails the build instead of silently producing a malformed emulated TD report. Signed-off-by: Michal Tarnacki Co-authored-by: GitHub Copilot --- deps/td-shim-AzCVMEmu/tdx-tdcall/src/lib.rs | 8 ++++ src/devices/pci/src/config.rs | 27 ++++++++--- src/devices/vsock/src/stream.rs | 4 +- src/migtd/src/migration/logging.rs | 46 +++++++++++++++++-- .../src/collateral.rs | 4 +- 5 files changed, 75 insertions(+), 14 deletions(-) diff --git a/deps/td-shim-AzCVMEmu/tdx-tdcall/src/lib.rs b/deps/td-shim-AzCVMEmu/tdx-tdcall/src/lib.rs index c5a6c5bc6..03b6de1e2 100644 --- a/deps/td-shim-AzCVMEmu/tdx-tdcall/src/lib.rs +++ b/deps/td-shim-AzCVMEmu/tdx-tdcall/src/lib.rs @@ -91,6 +91,14 @@ pub mod tdreport { TdxReport, TD_REPORT_ADDITIONAL_DATA_SIZE, TD_REPORT_SIZE, TdInfo, }; + // The emulated tdcall_report() copies the az-tdx-vtpm TdReport byte-for-byte + // into a TD_REPORT_SIZE buffer and transmutes it to TdxReport. Both types are + // #[repr(C)] views of the same TDX-module TDREPORT_STRUCT and are 1024 bytes. + // transmute already enforces size_of::() == TD_REPORT_SIZE at compile + // time; this guard ensures the *source* layout cannot silently drift (e.g. an + // az-tdx-vtpm upgrade) and zero-pad into a malformed report. Build fails instead. + const _: () = assert!(core::mem::size_of::() == TD_REPORT_SIZE); + /// Emulated tdcall_report function for AzCVMEmu mode /// Now returns the exact same error type as the original for perfect compatibility pub fn tdcall_report(additional_data: &[u8; 64]) -> Result { diff --git a/src/devices/pci/src/config.rs b/src/devices/pci/src/config.rs index f751f8915..e01360e6f 100644 --- a/src/devices/pci/src/config.rs +++ b/src/devices/pci/src/config.rs @@ -439,12 +439,18 @@ impl PciDevice { 0 => { let size = self.get_bar_size(current_bar_offset)?; + // Only program a BAR whose size we could probe. A malicious + // host emulating PCI config space can return a malformed + // sizing value (size == 0); never fall back to the raw + // host-chosen BAR value, which would place the device MMIO + // region at an unvalidated address outside the allocated + // MMIO window. Treat such a BAR as absent (fail closed). let addr = if size > 0 { let addr = alloc_mmio32(size)?; self.set_bar_addr(current_bar_offset, addr)?; addr } else { - bar + 0 }; self.bars[current_bar].bar_type = PciBarType::MemorySpace32; @@ -459,13 +465,15 @@ impl PciDevice { size = (self.get_bar_size(current_bar_offset + 4)? as u64) << 32; } + // See the 32-bit case above: fail closed instead of + // trusting the raw host-chosen BAR value when sizing fails. let addr = if size > 0 { let addr = alloc_mmio64(size)?; self.set_bar_addr(current_bar_offset, addr as u32)?; self.set_bar_addr(current_bar_offset + 4, (addr >> 32) as u32)?; addr } else { - bar as u64 + 0 }; self.bars[current_bar].address = addr & PCI_MEM64_BASE_ADDRESS_MASK; @@ -517,12 +525,15 @@ impl PciDevice { 0 => { let size = self.read_u32(current_bar_offset)?; + // Fail closed: never fall back to the raw host-chosen + // BAR value when sizing yields 0 (see the production + // `init` for the rationale). let addr = if size > 0 { let addr = alloc_mmio32(size)?; self.set_bar_addr(current_bar_offset, addr)?; addr } else { - bar + 0 }; self.bars[current_bar].bar_type = PciBarType::MemorySpace32; @@ -532,14 +543,14 @@ impl PciDevice { 2 => { self.bars[current_bar].bar_type = PciBarType::MemorySpace64; - let mut size = self.read_u64(current_bar_offset)?; + let size = self.read_u64(current_bar_offset)?; let addr = if size > 0 { let addr = alloc_mmio64(size)?; self.set_bar_addr(current_bar_offset, addr as u32)?; self.set_bar_addr(current_bar_offset + 4, (addr >> 32) as u32)?; addr } else { - bar as u64 + 0 }; self.bars[current_bar].address = addr & PCI_MEM64_BASE_ADDRESS_MASK; @@ -575,7 +586,11 @@ impl PciDevice { Ok(if size == 0 { size } else { - !(size & 0xFFFF_FFF0) + 1 + // `wrapping_add` avoids an overflow panic in debug builds when the + // host returns a malformed sizing value whose low bits are clear + // (e.g. 0x1): `!(0) + 1` would otherwise overflow. The wrapped + // result is 0, which is treated as "no BAR" by the caller. + (!(size & 0xFFFF_FFF0)).wrapping_add(1) }) } diff --git a/src/devices/vsock/src/stream.rs b/src/devices/vsock/src/stream.rs index ff705b0e2..3d35b25bc 100644 --- a/src/devices/vsock/src/stream.rs +++ b/src/devices/vsock/src/stream.rs @@ -174,8 +174,8 @@ impl VsockStream { rx_cnt: 0, tx_cnt: 0, last_fwd_cnt: 0, - peer_fwd_cnt: packet.fwd_cnt(), - peer_buf_alloc: packet.buf_alloc(), + peer_fwd_cnt: request.fwd_cnt(), + peer_buf_alloc: request.buf_alloc(), transport_context: 0, }; diff --git a/src/migtd/src/migration/logging.rs b/src/migtd/src/migration/logging.rs index be45b8322..9ea1c6439 100644 --- a/src/migtd/src/migration/logging.rs +++ b/src/migtd/src/migration/logging.rs @@ -17,7 +17,7 @@ use log::{Level, LevelFilter, Metadata, Record, SetLoggerError}; use raw_cpuid::CpuId; use spin::Mutex; #[cfg(not(test))] -use td_payload::mm::shared::alloc_shared_pages; +use td_payload::mm::shared::{alloc_shared_pages, free_shared_pages}; #[cfg(not(test))] use tdx_tdcall::{td_call, TdcallArgs}; use zerocopy::{transmute_ref, FromBytes, Immutable, IntoBytes}; @@ -143,8 +143,26 @@ pub fn create_logarea() -> Result<()> { let mut provisional_logareavector = PROVISIONAL_LOGAREAPTR.lock(); for index in 0..num_vcpus { // Allocate shared 4KB page for VMM memory logs - let data_buffer = - unsafe { alloc_shared_pages(1).ok_or(MigrationResult::OutOfResource)? }; + let data_buffer = match unsafe { alloc_shared_pages(1) } { + Some(buffer) => buffer, + None => { + // Free everything allocated in previous iterations before + // bailing out, so a mid-loop allocation failure does not leak + // the already-allocated shared and provisional pages. + for buffer_ptr in logareavector.iter() { + unsafe { free_shared_pages(*buffer_ptr, 1) }; + } + logareavector.clear(); + for buffer_ptr in provisional_logareavector.iter() { + unsafe { + let _ = + alloc::boxed::Box::from_raw(*buffer_ptr as *mut [u8; PAGE_SIZE]); + } + } + provisional_logareavector.clear(); + return Err(MigrationResult::OutOfResource); + } + }; logareavector.push(data_buffer); let data_buffer = @@ -232,6 +250,15 @@ pub fn free_provisional_logarea() { } pub async fn enable_logarea(log_max_level: u8, request_id: u64, data: &mut Vec) -> Result<()> { + // Security: the requested verbosity comes from the (untrusted) VMM via the + // EnableLogArea command. The log records are routed into the shared log area + // that the VMM can read. In a production (release) image, refuse to elevate + // the runtime level above Info so a hostile VMM cannot turn on Debug/Trace and + // harvest sensitive material (e.g. SPDM session-key debug dumps) from the + // shared area. Debug builds keep full verbosity for development. + #[cfg(not(debug_assertions))] + let log_max_level = core::cmp::min(log_max_level, 3); // 3 == Info + let padding: u32 = 0; let num_vcpus: u32 = LOGGING_INFORMATION.num_vcpus.load(Ordering::SeqCst); let logarea_created: bool = LOGGING_INFORMATION.logarea_created.load(Ordering::SeqCst); @@ -603,7 +630,18 @@ impl log::Log for VmmLoggerBackend { log_max_level = u8_to_levelfilter(LOGGING_INFORMATION.maxloglevel.load(Ordering::SeqCst)); } else if provisional_logs_enabled { - log_max_level = LevelFilter::Trace; + // Provisional records are captured into private buffers and copied into + // the VMM-readable shared area when EnableLogArea succeeds. In release + // images cap the provisional level at Info as well, so sensitive + // Debug/Trace material cannot reach the shared area through this path. + #[cfg(debug_assertions)] + { + log_max_level = LevelFilter::Trace; + } + #[cfg(not(debug_assertions))] + { + log_max_level = LevelFilter::Info; + } } else { log_max_level = log::max_level(); } diff --git a/tools/migtd-collateral-generator/src/collateral.rs b/tools/migtd-collateral-generator/src/collateral.rs index 74ae08704..8ccf2adc9 100644 --- a/tools/migtd-collateral-generator/src/collateral.rs +++ b/tools/migtd-collateral-generator/src/collateral.rs @@ -7,7 +7,7 @@ use serde::{Deserialize, Serialize}; use x509_parser::prelude::*; use crate::pcs_client::{ - fetch_data_from_url, fetch_pck_crl, fetch_qe_identity, fetch_root_ca, get_platform_tcb_list, + fetch_pck_crl, fetch_qe_identity, fetch_root_ca, fetch_root_ca_crl, get_platform_tcb_list, PlatformTcbRaw, }; @@ -93,7 +93,7 @@ pub async fn get_collateral(config: &dyn crate::PcsConfig) -> Result((qe_identity, qe_identity_issuer_chain, root_ca_crl)) }, fetch_root_ca(config),