From 409eadddb284b92c1b193b663383d7f75c8fd8a4 Mon Sep 17 00:00:00 2001 From: "James O. D. Hunt" Date: Thu, 12 Oct 2023 10:40:27 +0100 Subject: [PATCH 1/2] runtime-rs: ch: Improve readability of guest protection checks Improve the way `handle_guest_protection()` is structured by inverting the logic and checking the value of the `confidential_guest` setting before checking the guest protection. This makes the code easier to understand. > **Notes:** > > - This change also unconditionally saves the available guest protection > (where previously it was only saved when `confidential_guest=true`). > This explains the minor unit test fix. > > - This changes also errors if the CH driver finds an unexpected > protection (since only Intel TDX is currently tested). Signed-off-by: James O. D. Hunt --- .../hypervisor/src/ch/inner_hypervisor.rs | 48 ++++++++++++++----- 1 file changed, 37 insertions(+), 11 deletions(-) diff --git a/src/runtime-rs/crates/hypervisor/src/ch/inner_hypervisor.rs b/src/runtime-rs/crates/hypervisor/src/ch/inner_hypervisor.rs index 8e48040087..d16857015b 100644 --- a/src/runtime-rs/crates/hypervisor/src/ch/inner_hypervisor.rs +++ b/src/runtime-rs/crates/hypervisor/src/ch/inner_hypervisor.rs @@ -55,6 +55,10 @@ pub enum GuestProtectionError { // "true". #[error("TDX guest protection available and must be used with Cloud Hypervisor (set 'confidential_guest=true')")] TDXProtectionMustBeUsedWithCH, + + // TDX is the only tested CH protection currently. + #[error("Expected TDX protection, found {0}")] + ExpectedTDXProtection(GuestProtection), } impl CloudHypervisorInner { @@ -482,17 +486,25 @@ impl CloudHypervisorInner { task::spawn_blocking(|| -> Result { get_guest_protection() }) .await??; - if protection == GuestProtection::NoProtection { - if confidential_guest { - return Err(anyhow!(GuestProtectionError::NoProtectionAvailable)); - } else { - debug!(sl!(), "no guest protection available"); - } - } else if confidential_guest { - self.guest_protection_to_use = protection.clone(); + self.guest_protection_to_use = protection.clone(); - info!(sl!(), "guest protection available and requested"; "guest-protection" => protection.to_string()); + info!(sl!(), "guest protection {:?}", protection.to_string()); + + if confidential_guest { + if protection == GuestProtection::NoProtection { + // User wants protection, but none available. + return Err(anyhow!(GuestProtectionError::NoProtectionAvailable)); + } else if let GuestProtection::Tdx(_) = protection { + info!(sl!(), "guest protection available and requested"; "guest-protection" => protection.to_string()); + } else { + return Err(anyhow!(GuestProtectionError::ExpectedTDXProtection( + protection + ))); + } + } else if protection == GuestProtection::NoProtection { + debug!(sl!(), "no guest protection available"); } else if let GuestProtection::Tdx(_) = protection { + // CH requires TDX protection to be used. return Err(anyhow!(GuestProtectionError::TDXProtectionMustBeUsedWithCH)); } else { info!(sl!(), "guest protection available but not requested"; "guest-protection" => protection.to_string()); @@ -899,13 +911,27 @@ mod tests { confidential_guest: false, available_protection: Some(GuestProtection::Tdx(tdx_details.clone())), result: Err(anyhow!(GuestProtectionError::TDXProtectionMustBeUsedWithCH)), - guest_protection_to_use: GuestProtection::NoProtection, + guest_protection_to_use: GuestProtection::Tdx(tdx_details.clone()), }, TestData { confidential_guest: true, available_protection: Some(GuestProtection::Tdx(tdx_details.clone())), result: Ok(()), - guest_protection_to_use: GuestProtection::Tdx(tdx_details.clone()), + guest_protection_to_use: GuestProtection::Tdx(tdx_details), + }, + TestData { + confidential_guest: false, + available_protection: Some(GuestProtection::Pef), + result: Ok(()), + guest_protection_to_use: GuestProtection::NoProtection, + }, + TestData { + confidential_guest: true, + available_protection: Some(GuestProtection::Pef), + result: Err(anyhow!(GuestProtectionError::ExpectedTDXProtection( + GuestProtection::Pef + ))), + guest_protection_to_use: GuestProtection::Pef, }, ]; From 0e0867f15d2d39bb126bc11b4177be6a21e0d958 Mon Sep 17 00:00:00 2001 From: "James O. D. Hunt" Date: Thu, 12 Oct 2023 11:18:07 +0100 Subject: [PATCH 2/2] runtime-rs: ch: Add TDX CH features check If you attempt to create a container (a TD) on a TDX system using a custom build of Cloud Hypervisor (CH) that was not built with the `tdx` CH feature, Kata will report the following, somewhat cryptic, CH error: ``` ApiError(VmBoot(InvalidPayload)) ``` Newer versions of CH now report their build-time features in the ping API response message so we now use that, if available, to detect this scenario and generate a user-friendly error message instead. This changes improves the readability of `handle_guest_protection()` and adds a couple of additional tests for that method. Fixes: #8152. Signed-off-by: James O. D. Hunt --- .../crates/hypervisor/src/ch/inner.rs | 7 +++ .../hypervisor/src/ch/inner_hypervisor.rs | 55 +++++++++++++++++++ 2 files changed, 62 insertions(+) diff --git a/src/runtime-rs/crates/hypervisor/src/ch/inner.rs b/src/runtime-rs/crates/hypervisor/src/ch/inner.rs index 13f86a969d..dfab644d15 100644 --- a/src/runtime-rs/crates/hypervisor/src/ch/inner.rs +++ b/src/runtime-rs/crates/hypervisor/src/ch/inner.rs @@ -67,6 +67,12 @@ pub struct CloudHypervisorInner { // The cloud-hypervisor device-id is later looked up and used while // removing the device. pub(crate) device_ids: HashMap, + + // List of Cloud Hypervisor features enabled at Cloud Hypervisor build-time. + // + // If the version of CH does not provide these details, the value will be + // None. + pub(crate) ch_features: Option>, } const CH_DEFAULT_TIMEOUT_SECS: u32 = 10; @@ -104,6 +110,7 @@ impl CloudHypervisorInner { shutdown_rx: Some(rx), tasks: None, guest_protection_to_use: GuestProtection::NoProtection, + ch_features: None, } } diff --git a/src/runtime-rs/crates/hypervisor/src/ch/inner_hypervisor.rs b/src/runtime-rs/crates/hypervisor/src/ch/inner_hypervisor.rs index d16857015b..765d2da161 100644 --- a/src/runtime-rs/crates/hypervisor/src/ch/inner_hypervisor.rs +++ b/src/runtime-rs/crates/hypervisor/src/ch/inner_hypervisor.rs @@ -23,6 +23,8 @@ use kata_sys_util::protection::{available_guest_protection, GuestProtection}; use kata_types::capabilities::{Capabilities, CapabilityBits}; use kata_types::config::default::DEFAULT_CH_ROOTFS_TYPE; use lazy_static::lazy_static; +use serde::{Deserialize, Serialize}; +use serde_json::Value; use std::convert::TryFrom; use std::fs::create_dir_all; use std::os::unix::net::UnixStream; @@ -42,6 +44,20 @@ const CH_NAME: &str = "cloud-hypervisor"; /// Number of milliseconds to wait before retrying a CH operation. const CH_POLL_TIME_MS: u64 = 50; +// The name of the CH JSON key for the build-time features list. +const CH_FEATURES_KEY: &str = "features"; + +// The name of the CH build-time feature for Intel TDX. +const CH_FEATURE_TDX: &str = "tdx"; + +#[derive(Clone, Deserialize, Serialize)] +pub struct VmmPingResponse { + pub build_version: String, + pub version: String, + pub pid: i64, + pub features: Vec, +} + #[derive(thiserror::Error, Debug, PartialEq)] pub enum GuestProtectionError { #[error("guest protection requested but no guest protection available")] @@ -75,6 +91,14 @@ impl CloudHypervisorInner { .await .context("hypervisor running check failed")?; + if guest_protection_is_tdx(self.guest_protection_to_use.clone()) { + if let Some(features) = &self.ch_features { + if !features.contains(&CH_FEATURE_TDX.to_string()) { + return Err(anyhow!("Cloud Hypervisor is not built with TDX support")); + } + } + } + self.state = VmmState::VmmServerReady; Ok(()) @@ -424,6 +448,33 @@ impl CloudHypervisorInner { Ok(()) } + // Check the specified ping API response to see if it contains CH's + // build-time features list. If so, save them. + async fn handle_ch_build_features(&mut self, ping_response: &str) -> Result<()> { + let v: Value = serde_json::from_str(ping_response)?; + + let got = &v[CH_FEATURES_KEY]; + + if got.is_null() { + return Ok(()); + } + + let features_list = got + .as_array() + .ok_or("expected CH to return array of features") + .map_err(|e| anyhow!(e))?; + + let features: Vec = features_list + .iter() + .map(Value::to_string) + .map(|s| s.trim_start_matches('"').trim_end_matches('"').to_string()) + .collect(); + + self.ch_features = Some(features); + + Ok(()) + } + async fn cloud_hypervisor_ping_until_ready(&mut self, _poll_time_ms: u64) -> Result<()> { let socket = self .api_socket @@ -439,7 +490,11 @@ impl CloudHypervisorInner { if let Ok(response) = response { if let Some(detail) = response { + // Check for a list of built-in features, returned by this + // API call in newer versions of CH. debug!(sl!(), "ping response: {:?}", detail); + + self.handle_ch_build_features(&detail).await?; } break; }