Skip to content

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

Open
MichalTarnacki wants to merge 1 commit into
intel:mainfrom
MichalTarnacki:fix/other_issues
Open

fix(migtd): harden BAR handling, logging, and collateral fetches#939
MichalTarnacki wants to merge 1 commit into
intel:mainfrom
MichalTarnacki:fix/other_issues

Conversation

@MichalTarnacki

Copy link
Copy Markdown
Contributor
# Assessed Severity Bug Call Chain / Location Fix Classification Disposition Reason
1 Critical BAR-size wrap -> raw VMM BAR addr -> CommonConfig MemoryRegion in TD private DRAM -> arbitrary mmio_write handle_pre_mig -> setup_transport -> VirtioPciTransport::init -> PciDevice::init -> PciDevice::get_bar_size -> VirtioPciTransport::init -> mem::MemoryRegion::new -> MemoryRegion::mmio_write In MemoryRegion::new reject base ranges overlapping TD private DRAM (query td-payload mm for accepted-private range); in PciDevice::init never fall back to raw VMM BAR value -- fail closed when size==0. true_positive The defect is real and confirmed in code: when the host returns a malformed sizing probe so that get_bar_size yields 0, both init() paths previously fell back to } else { bar } / } else { bar as u64 }, storing the raw host-chosen BAR value into bars[].address and bypassing alloc_mmio32/64, which is the only thing that constrains the device MMIO window to a RAM-disjoint range. That is a genuine missing input validation at the VIRTIO_PCI_CONFIG trust boundary. However, the escalation to "write-what-where in TD private DRAM / MSK theft" is a false positive for the shipped firmware, because production virtio MMIO goes through tdvmcall_mmio_write (a TDVMCALL to the host), which cannot reach private encrypted TD memory; the residual is at most host-directed access to a host-chosen address it already controls, i.e. DoS-class, which is out of the TDX threat model.
2 Low collateral-generator: root_ca_crl HTTP status unchecked; non-200 body becomes enrolled CRL main -> get_collateral -> fetch_data_from_url Check response_code==200 before consuming .data (the dead sibling fetch_root_ca_crl@pcs_client.rs:80 already does this -- wire it in). weakness The collateral generator at collateral.rs:96 consumed fetch_data_from_url(...).await?.data without checking response_code, so a PCS 404/403 error body could be enrolled as the root CA CRL while every sibling fetch function checks for 200. It only affects the build-time collateral artifact, not the firmware binary or runtime TD security: TLS to PCS is enabled, and a corrupt collateral merely makes runtime CRL/TCB lookups fail closed (migrations rejected), giving no memory primitive or trust-boundary bypass. Classifying it as Low/hardening (build-time) is correct, but it is a weakness not true positve.
3 Info create_logarea early-return leaks prior shared pages + Box<[u8;4096]> in static Vec main -> create_logarea -> alloc_shared_pages On Err, drain LOGAREAPTR/PROVISIONAL_LOGAREAPTR and free_shared_pages each entry before returning. weakness In create_logarea the per-vCPU loop pushes a shared page into LOGAREAPTR and a private Box into PROVISIONAL_LOGAREAPTR each iteration, and the original ? on alloc_shared_pages returned immediately on a mid-loop failure, leaking the i shared pages and i private boxes already pushed since logarea_created is never set and nothing frees them later. It is only a weakness because this is a one-time boot-path init called once from main, num_vcpus comes from the trusted TDX module (not VMM-controlled), the leak is bounded and not attacker-amplifiable, and there is no memory primitive or trust-boundary crossing; host-induced resource exhaustion is out of scope for the TDX threat model anyway.
4 Low VMM EnableLogArea sets log::set_max_level without authentication handle_pre_mig -> wait_for_request -> log::set_max_level Clamp VMM-requested level to <=Info, or measure the chosen level into RTMR so it is attestable. true_positive The code path WaitForRequestResponse::EnableLogArea(wfr_info) -> log::set_max_level(u8_to_levelfilter(wfr_info.log_max_level)) takes a VMM-supplied log_max_level and applies it with no authentication and no clamp, so a hostile VMM can raise verbosity to Trace (value 5). Because the default build sets no static release_max_level_* cap, the debug!/trace! calls remain compiled in — including spdmlib's session-key dumps (final_key, exchange data) and traces of transport buffers, policy, and tdreport-related material — and those records are written into the shared, host-readable log area. The threat is therefore a real confidentiality breach: VMM-controlled escalation that changes the class of disclosed data from benign Info-level metadata to cryptographic key material, potentially leaking SPDM session keys. It grants no private-memory write and no code execution, and the leak is bounded to what the code already logs, so it is information-disclosure rather than integrity loss; weighed against the fact that the trigger condition is the default (not an exceptional) build configuration and the attacker is the untrusted VMM
5 Medium VsockStream::accept reads peer credit fields from OUTGOING packet (variable shad -> accept Rename response var; read peer_* from incoming request packet. true_positive In accept() the peer_fwd_cnt and peer_buf_alloc of the new stream were being read from the outgoing OP_RESPONSE packet (which carries our own fwd_cnt = 0 and buf_alloc = VSOCK_BUF_ALLOC) instead of from the inbound request, so the flow-control credit window started from our own values rather than the peer's advertised ones. The connection itself still opened correctly — addresses, ports, state transition and handshake were all fine; only the initial credit accounting was wrong. The error window was transient because the first peer packet (OP_RW or OP_CREDIT_UPDATE) overwrites both fields with the real values in recv_packet_connected(), and the worst case was a brief stall or over-send, i.e. a liveness issue, not memory unsafety or data corruption.
6 Info AzCVMEmu tdcall_report transmutes AzTdReport->TdxReport assuming layout parity attestation::attest_init -> tdreport::tdcall_report -> tdcall_report_emulated Add const_assert!(size_of::()==TD_REPORT_SIZE) and a field-offset spot-check (e.g. offset_of REPORTDATA) so layout drift fails at compile time. weakness The transmute lives only in the AzCVMEmu emulation path, not in shipped TDX firmware, and both AzTdReport and TdxReport are #[repr(C)] views of the same 1024-byte TDX TDREPORT_STRUCT, so the byte-for-byte copy is layout-correct and bounded (copy_size = min(len, 1024), no OOB). The transmute already enforces size_of::() == 1024 at compile time. The only real risk is silent drift in the upstream az-tdx-vtpm source layout, which would merely produce a malformed report that fails attestation (fail-closed), not memory corruption.

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 <[email protected]>
Co-authored-by: GitHub Copilot <[email protected]>
Comment on lines 145 to +165
// 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);
}
};

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?

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants