From b22ddb312c61739142cc619186095e02f7a61cd6 Mon Sep 17 00:00:00 2001 From: Sven Groot Date: Wed, 26 Feb 2025 17:40:50 -0800 Subject: [PATCH 1/3] vmbus_client: don't request confidential channel support. --- vm/devices/vmbus/vmbus_client/src/lib.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/vm/devices/vmbus/vmbus_client/src/lib.rs b/vm/devices/vmbus/vmbus_client/src/lib.rs index d01c250311..d39a0110da 100644 --- a/vm/devices/vmbus/vmbus_client/src/lib.rs +++ b/vm/devices/vmbus/vmbus_client/src/lib.rs @@ -55,7 +55,11 @@ use zerocopy::KnownLayout; const SINT: u8 = 2; const VTL: u8 = 0; const SUPPORTED_VERSIONS: &[Version] = &[Version::Iron, Version::Copper]; -const SUPPORTED_FEATURE_FLAGS: FeatureFlags = FeatureFlags::all(); +const SUPPORTED_FEATURE_FLAGS: FeatureFlags = FeatureFlags::new() + .with_guest_specified_signal_parameters(true) + .with_channel_interrupt_redirection(true) + .with_modify_connection(true) + .with_client_id(true); /// The client interface synic events. pub trait SynicEventClient: Send + Sync { From 8c8e2eeca32005e54e3b6f7addb6f5db312b40c5 Mon Sep 17 00:00:00 2001 From: Sven Groot Date: Thu, 27 Feb 2025 10:59:51 -0800 Subject: [PATCH 2/3] Decentralize supported feature flags. --- vm/devices/vmbus/vmbus_client/src/lib.rs | 18 +++++----- .../vmbus/vmbus_client/src/saved_state.rs | 3 +- vm/devices/vmbus/vmbus_core/src/protocol.rs | 14 ++------ vm/devices/vmbus/vmbus_server/src/channels.rs | 33 ++++++++++++------- .../vmbus_server/src/channels/saved_state.rs | 3 +- vm/devices/vmbus/vmbus_server/src/lib.rs | 2 +- 6 files changed, 38 insertions(+), 35 deletions(-) diff --git a/vm/devices/vmbus/vmbus_client/src/lib.rs b/vm/devices/vmbus/vmbus_client/src/lib.rs index d39a0110da..8cd807eb56 100644 --- a/vm/devices/vmbus/vmbus_client/src/lib.rs +++ b/vm/devices/vmbus/vmbus_client/src/lib.rs @@ -1532,13 +1532,13 @@ mod tests { padding: 0, selected_version_or_connection_id: 0, }, - supported_features: FeatureFlags::all().into(), + supported_features: SUPPORTED_FEATURE_FLAGS.into(), }, )); let version = recv.await.unwrap().unwrap(); assert_eq!(version.version, Version::Copper); - assert_eq!(version.feature_flags, FeatureFlags::all()); + assert_eq!(version.feature_flags, SUPPORTED_FEATURE_FLAGS); } async fn get_channel(&mut self, client: &mut VmbusClient) -> OfferInfo { @@ -1700,7 +1700,7 @@ mod tests { interrupt_page_or_target_info: TargetInfo::new() .with_sint(2) .with_vtl(0) - .with_feature_flags(FeatureFlags::all().into()) + .with_feature_flags(SUPPORTED_FEATURE_FLAGS.into()) .into(), parent_to_child_monitor_page_gpa: 0, child_to_parent_monitor_page_gpa: 0, @@ -1727,7 +1727,7 @@ mod tests { interrupt_page_or_target_info: TargetInfo::new() .with_sint(2) .with_vtl(0) - .with_feature_flags(FeatureFlags::all().into()) + .with_feature_flags(SUPPORTED_FEATURE_FLAGS.into()) .into(), parent_to_child_monitor_page_gpa: 0, child_to_parent_monitor_page_gpa: 0, @@ -1745,14 +1745,14 @@ mod tests { padding: 0, selected_version_or_connection_id: 0, }, - supported_features: FeatureFlags::all().into_bits(), + supported_features: SUPPORTED_FEATURE_FLAGS.into_bits(), }, )); let version = recv.await.unwrap().unwrap(); assert_eq!(version.version, Version::Copper); - assert_eq!(version.feature_flags, FeatureFlags::all()); + assert_eq!(version.feature_flags, SUPPORTED_FEATURE_FLAGS); } #[async_test] @@ -1772,7 +1772,7 @@ mod tests { interrupt_page_or_target_info: TargetInfo::new() .with_sint(2) .with_vtl(0) - .with_feature_flags(FeatureFlags::all().into()) + .with_feature_flags(SUPPORTED_FEATURE_FLAGS.into()) .into(), parent_to_child_monitor_page_gpa: 0, child_to_parent_monitor_page_gpa: 0, @@ -1826,7 +1826,7 @@ mod tests { interrupt_page_or_target_info: TargetInfo::new() .with_sint(2) .with_vtl(0) - .with_feature_flags(FeatureFlags::all().into()) + .with_feature_flags(SUPPORTED_FEATURE_FLAGS.into()) .into(), parent_to_child_monitor_page_gpa: 0, child_to_parent_monitor_page_gpa: 0, @@ -1853,7 +1853,7 @@ mod tests { interrupt_page_or_target_info: TargetInfo::new() .with_sint(2) .with_vtl(0) - .with_feature_flags(FeatureFlags::all().into()) + .with_feature_flags(SUPPORTED_FEATURE_FLAGS.into()) .into(), parent_to_child_monitor_page_gpa: 0, child_to_parent_monitor_page_gpa: 0, diff --git a/vm/devices/vmbus/vmbus_client/src/saved_state.rs b/vm/devices/vmbus/vmbus_client/src/saved_state.rs index 0dc8cc3c11..f52b14fa1e 100644 --- a/vm/devices/vmbus/vmbus_client/src/saved_state.rs +++ b/vm/devices/vmbus/vmbus_client/src/saved_state.rs @@ -4,6 +4,7 @@ use crate::OfferInfo; use crate::RestoreError; use crate::RestoredChannel; +use crate::SUPPORTED_FEATURE_FLAGS; use guid::Guid; use mesh::payload::Protobuf; use vmbus_channel::bus::OfferKey; @@ -198,7 +199,7 @@ impl TryFrom for super::ClientState { .ok_or(RestoreError::UnsupportedVersion(version))?; let feature_flags = FeatureFlags::from(feature_flags); - if feature_flags.contains_unsupported_bits() { + if !SUPPORTED_FEATURE_FLAGS.contains(feature_flags) { return Err(RestoreError::UnsupportedFeatureFlags(feature_flags.into())); } diff --git a/vm/devices/vmbus/vmbus_core/src/protocol.rs b/vm/devices/vmbus/vmbus_core/src/protocol.rs index eeec68a66c..6a4905e872 100644 --- a/vm/devices/vmbus/vmbus_core/src/protocol.rs +++ b/vm/devices/vmbus/vmbus_core/src/protocol.rs @@ -175,17 +175,9 @@ pub struct FeatureFlags { } impl FeatureFlags { - pub const fn all() -> Self { - Self::new() - .with_guest_specified_signal_parameters(true) - .with_channel_interrupt_redirection(true) - .with_modify_connection(true) - .with_client_id(true) - .with_confidential_channels(true) - } - - pub fn contains_unsupported_bits(&self) -> bool { - u32::from(*self) & !u32::from(Self::all()) != 0 + /// Returns true if `other` contains only flags that are also set in `self`. + pub fn contains(&self, other: FeatureFlags) -> bool { + self.into_bits() & other.into_bits() == other.into_bits() } } diff --git a/vm/devices/vmbus/vmbus_server/src/channels.rs b/vm/devices/vmbus/vmbus_server/src/channels.rs index f6a1ad3280..dbd38dcd69 100644 --- a/vm/devices/vmbus/vmbus_server/src/channels.rs +++ b/vm/devices/vmbus/vmbus_server/src/channels.rs @@ -1196,6 +1196,14 @@ static SUPPORTED_VERSIONS: &[Version] = &[ Version::Copper, ]; +// Feature flags that are always supported. +// N.B. Confidential channels are conditionally supported if running in the paravisor. +const SUPPORTED_FEATURE_FLAGS: FeatureFlags = FeatureFlags::new() + .with_guest_specified_signal_parameters(true) + .with_channel_interrupt_redirection(true) + .with_modify_connection(true) + .with_client_id(true); + /// An error that occurred while mapping the interrupt page. #[derive(Error, Debug)] pub enum InterruptPageError { @@ -2247,16 +2255,17 @@ impl<'a, N: 'a + Notifier> ServerWithNotifier<'a, N> { // The max version and features may be limited in order to test older protocol versions. // // N.B. Confidential channels should only be enabled if the connection is trusted. + let max_supported_flags = + SUPPORTED_FEATURE_FLAGS.with_confidential_channels(request.trusted); + if let Some(max_version) = self.inner.max_version { if version as u32 > max_version.version { return None; } - max_version.feature_flags.with_confidential_channels( - max_version.feature_flags.confidential_channels() && request.trusted, - ) + max_supported_flags & max_version.feature_flags } else { - FeatureFlags::all().with_confidential_channels(request.trusted) + max_supported_flags } } else { FeatureFlags::new() @@ -3931,7 +3940,7 @@ mod tests { server.with_notifier(notifier).complete_initiate_contact( ModifyConnectionResponse::Supported( protocol::ConnectionState::SUCCESSFUL, - FeatureFlags::all(), + SUPPORTED_FEATURE_FLAGS, ), ); @@ -4196,7 +4205,7 @@ mod tests { .with_notifier(&mut notifier) .complete_initiate_contact(ModifyConnectionResponse::Supported( protocol::ConnectionState::SUCCESSFUL, - FeatureFlags::all(), + SUPPORTED_FEATURE_FLAGS, )); let version_response = protocol::VersionResponse { @@ -4393,7 +4402,7 @@ mod tests { .with_notifier(&mut notifier) .complete_initiate_contact(ModifyConnectionResponse::Supported( protocol::ConnectionState::SUCCESSFUL, - FeatureFlags::all(), + SUPPORTED_FEATURE_FLAGS, )); // Discard the version response message. @@ -4479,7 +4488,7 @@ mod tests { self.c() .complete_modify_connection(ModifyConnectionResponse::Supported( protocol::ConnectionState::SUCCESSFUL, - FeatureFlags::all(), + SUPPORTED_FEATURE_FLAGS, )); } @@ -4676,7 +4685,7 @@ mod tests { self.c() .complete_initiate_contact(ModifyConnectionResponse::Supported( protocol::ConnectionState::SUCCESSFUL, - FeatureFlags::all(), + SUPPORTED_FEATURE_FLAGS, )); let version = self.version.unwrap(); @@ -4727,7 +4736,7 @@ mod tests { env.c() .complete_initiate_contact(ModifyConnectionResponse::Supported( protocol::ConnectionState::SUCCESSFUL, - FeatureFlags::all(), + SUPPORTED_FEATURE_FLAGS, )); let offer_id3 = env.offer(3); env.c().handle_request_offers().unwrap(); @@ -5008,7 +5017,7 @@ mod tests { env.c() .complete_modify_connection(ModifyConnectionResponse::Supported( protocol::ConnectionState::SUCCESSFUL, - FeatureFlags::all(), + SUPPORTED_FEATURE_FLAGS, )); env.notifier @@ -5166,7 +5175,7 @@ mod tests { env.c() .complete_modify_connection(ModifyConnectionResponse::Supported( protocol::ConnectionState::FAILED_UNKNOWN_FAILURE, - FeatureFlags::all(), + SUPPORTED_FEATURE_FLAGS, )); env.notifier diff --git a/vm/devices/vmbus/vmbus_server/src/channels/saved_state.rs b/vm/devices/vmbus/vmbus_server/src/channels/saved_state.rs index 6bc6f9da98..482b4c7286 100644 --- a/vm/devices/vmbus/vmbus_server/src/channels/saved_state.rs +++ b/vm/devices/vmbus/vmbus_server/src/channels/saved_state.rs @@ -5,6 +5,7 @@ use super::OfferError; use super::OfferParamsInternal; use super::OfferedInfo; use super::RestoreState; +use super::SUPPORTED_FEATURE_FLAGS; use guid::Guid; use mesh::payload::Protobuf; use std::fmt::Display; @@ -323,7 +324,7 @@ impl VersionInfo { .ok_or(RestoreError::UnsupportedVersion(self.version))?; let feature_flags = FeatureFlags::from(self.feature_flags); - if feature_flags.contains_unsupported_bits() { + if !SUPPORTED_FEATURE_FLAGS.contains(feature_flags) { return Err(RestoreError::UnsupportedFeatureFlags(feature_flags.into())); } diff --git a/vm/devices/vmbus/vmbus_server/src/lib.rs b/vm/devices/vmbus/vmbus_server/src/lib.rs index 4c2815db44..693f45b26c 100644 --- a/vm/devices/vmbus/vmbus_server/src/lib.rs +++ b/vm/devices/vmbus/vmbus_server/src/lib.rs @@ -447,7 +447,7 @@ impl<'a, T: Spawn> VmbusServerBuilder<'a, T> { .map(|_| { ModifyConnectionResponse::Supported( protocol::ConnectionState::SUCCESSFUL, - protocol::FeatureFlags::all(), + protocol::FeatureFlags::from_bits(u32::MAX), ) }) .boxed() From 4ce1dde6d086423e510f8d491bf0a355c2e65291 Mon Sep 17 00:00:00 2001 From: Sven Groot Date: Thu, 27 Feb 2025 12:12:57 -0800 Subject: [PATCH 3/3] Fix confidential channel flag when restoring. --- .../vmbus/vmbus_server/src/channels/saved_state.rs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/vm/devices/vmbus/vmbus_server/src/channels/saved_state.rs b/vm/devices/vmbus/vmbus_server/src/channels/saved_state.rs index 482b4c7286..4679f63905 100644 --- a/vm/devices/vmbus/vmbus_server/src/channels/saved_state.rs +++ b/vm/devices/vmbus/vmbus_server/src/channels/saved_state.rs @@ -316,7 +316,7 @@ impl VersionInfo { } } - fn restore(self) -> Result { + fn restore(self, trusted: bool) -> Result { let version = super::SUPPORTED_VERSIONS .iter() .find(|v| self.version == **v as u32) @@ -324,7 +324,8 @@ impl VersionInfo { .ok_or(RestoreError::UnsupportedVersion(self.version))?; let feature_flags = FeatureFlags::from(self.feature_flags); - if !SUPPORTED_FEATURE_FLAGS.contains(feature_flags) { + let supported_flags = SUPPORTED_FEATURE_FLAGS.with_confidential_channels(trusted); + if !supported_flags.contains(feature_flags) { return Err(RestoreError::UnsupportedFeatureFlags(feature_flags.into())); } @@ -430,7 +431,7 @@ impl Connection { trusted, } => super::ConnectionState::Connecting { info: super::ConnectionInfo { - version: version.restore()?, + version: version.restore(trusted)?, trusted, interrupt_page, monitor_page: monitor_page.map(MonitorPageGpas::restore), @@ -451,7 +452,7 @@ impl Connection { client_id, trusted, } => super::ConnectionState::Connected(super::ConnectionInfo { - version: version.restore()?, + version: version.restore(trusted)?, trusted, offers_sent, interrupt_page, @@ -806,7 +807,9 @@ impl ReservedState { } fn restore(&self) -> Result { - let version = self.version.clone().restore().map_err(|e| match e { + // We don't know if the connection when the channel was reserved was trusted, so assume it + // was for what feature flags are accepted here; it doesn't affect any actual behavior. + let version = self.version.clone().restore(true).map_err(|e| match e { RestoreError::UnsupportedVersion(v) => RestoreError::UnsupportedReserveVersion(v), RestoreError::UnsupportedFeatureFlags(f) => { RestoreError::UnsupportedReserveFeatureFlags(f)