Skip to content

fix(migtd): harden BAR handling, logging, and collateral fetches #939

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.

Sign up for GitHub

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

Open
MichalTarnacki wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(migtd): harden BAR handling, logging, and collateral fetches #939

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions deps/td-shim-AzCVMEmu/tdx-tdcall/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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::<TdxReport>() == 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::<AzTdReport>() == 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<TdxReport, TdCallError> {
Expand Down
27 changes: 21 additions & 6 deletions src/devices/pci/src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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)
})
}

Expand Down
4 changes: 2 additions & 2 deletions src/devices/vsock/src/stream.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};

Expand Down
46 changes: 42 additions & 4 deletions src/migtd/src/migration/logging.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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);
}
};
Comment on lines 145 to +165

@sgrams sgrams Jul 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about this snippet.
alloc::boxed::Box::from_raw for sure won't allocate shared memory.
How's this a fix?

@MichalTarnacki MichalTarnacki Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

provisional_logareavector stores raw pointers obtained from Box::into_raw(provisional_buffer);

            // Allocate private 4KB page for provisional logging
            let mut provisional_buffer: alloc::boxed::Box<[u8; PAGE_SIZE]> =
                alloc::boxed::Box::new([0; PAGE_SIZE]);
            provisional_buffer[0..size_of::<LogAreaBufferHeader>()]
                .copy_from_slice(&bytes[0..bytes.len()]);
            let provisional_buffer_ptr = alloc::boxed::Box::into_raw(provisional_buffer) as *mut u8;
            provisional_logareavector.push(provisional_buffer_ptr as usize);

During cleanup, ownership is being reconstructed with alloc::boxed::Box::from_raw(...).
When that reconstructed Box goes out of scope, it is dropped, and the provisional heap memory is freed.

logareavector.push(data_buffer);

let data_buffer =
Expand Down Expand Up @@ -232,6 +250,15 @@ pub fn free_provisional_logarea() {
}

pub async fn enable_logarea(log_max_level: u8, request_id: u64, data: &mut Vec<u8>) -> 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);
Expand Down Expand Up @@ -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();
}
Expand Down
4 changes: 2 additions & 2 deletions tools/migtd-collateral-generator/src/collateral.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};

Expand Down Expand Up @@ -93,7 +93,7 @@ pub async fn get_collateral(config: &dyn crate::PcsConfig) -> Result<Collaterals
async {
let (qe_identity, qe_identity_issuer_chain) = fetch_qe_identity(config).await?;
let root_ca_crl_url = get_root_ca_crl_url(qe_identity_issuer_chain.as_str())?;
let root_ca_crl = fetch_data_from_url(&root_ca_crl_url).await?.data;
let root_ca_crl = fetch_root_ca_crl(&root_ca_crl_url).await?;
Ok::<_, anyhow::Error>((qe_identity, qe_identity_issuer_chain, root_ca_crl))
},
fetch_root_ca(config),
Expand Down
Loading