Merge "[test][fix] Sort the keys of device info map in CBOR order" into main
diff --git a/avf_flags.aconfig b/avf_flags.aconfig
index 8abb9ee..589d227 100644
--- a/avf_flags.aconfig
+++ b/avf_flags.aconfig
@@ -2,6 +2,7 @@
flag {
name: "avf_v_test_apis"
+ is_exported: true
namespace: "virtualization"
description: "Only purpose of this flag is to be used in @FlaggedApi in our V test apis"
bug: "325441024"
diff --git a/java/framework/src/android/system/virtualmachine/VirtualMachineConfig.java b/java/framework/src/android/system/virtualmachine/VirtualMachineConfig.java
index 693a7d7..12aeac8 100644
--- a/java/framework/src/android/system/virtualmachine/VirtualMachineConfig.java
+++ b/java/framework/src/android/system/virtualmachine/VirtualMachineConfig.java
@@ -829,12 +829,12 @@
@SystemApi
@NonNull
public Builder setPayloadBinaryName(@NonNull String payloadBinaryName) {
+ requireNonNull(payloadBinaryName, "payloadBinaryName must not be null");
if (payloadBinaryName.contains(File.separator)) {
throw new IllegalArgumentException(
"Invalid binary file name: " + payloadBinaryName);
}
- mPayloadBinaryName =
- requireNonNull(payloadBinaryName, "payloadBinaryName must not be null");
+ mPayloadBinaryName = payloadBinaryName;
return this;
}
diff --git a/microdroid_manager/src/dice.rs b/microdroid_manager/src/dice.rs
index 2469325..a8b88aa 100644
--- a/microdroid_manager/src/dice.rs
+++ b/microdroid_manager/src/dice.rs
@@ -14,11 +14,11 @@
use crate::dice_driver::DiceDriver;
use crate::instance::{ApexData, ApkData};
-use crate::{is_debuggable, is_strict_boot, MicrodroidData};
+use crate::{is_debuggable, MicrodroidData};
use anyhow::{bail, Context, Result};
use ciborium::{cbor, Value};
use coset::CborSerializable;
-use diced_open_dice::{Hidden, OwnedDiceArtifacts, HIDDEN_SIZE};
+use diced_open_dice::OwnedDiceArtifacts;
use microdroid_metadata::PayloadMetadata;
use openssl::sha::{sha512, Sha512};
use std::iter::once;
@@ -53,37 +53,10 @@
let debuggable = is_debuggable()?;
// Send the details to diced
- let hidden = if cfg!(llpvm_changes) {
- hidden_input_from_instance_id()?
- } else {
- instance_data.salt.clone().try_into().unwrap()
- };
+ let hidden = instance_data.salt.clone().try_into().unwrap();
dice.derive(code_hash, &config_descriptor, authority_hash, debuggable, hidden)
}
-// Get the "Hidden input" for DICE derivation.
-// This provides differentiation of secrets for different VM instances with same payload.
-fn hidden_input_from_instance_id() -> Result<Hidden> {
- // For protected VM: this is all 0s, pvmfw ensures differentiation is added early in secrets.
- // For non-protected VM: this is derived from instance_id of the VM instance.
- let hidden_input = if !is_strict_boot() {
- if let Some(id) = super::get_instance_id()? {
- sha512(&id)
- } else {
- // TODO(b/325094712): Absence of instance_id occurs due to missing DT in some
- // x86_64 test devices (such as Cuttlefish). From security perspective, this is
- // acceptable for non-protected VM.
- log::warn!(
- "Instance Id missing, this may lead to 2 non protected VMs having same secrets"
- );
- [0u8; HIDDEN_SIZE]
- }
- } else {
- [0u8; HIDDEN_SIZE]
- };
- Ok(hidden_input)
-}
-
struct Subcomponent {
name: String,
version: u64,
diff --git a/microdroid_manager/src/instance.rs b/microdroid_manager/src/instance.rs
index f42b86d..7a9f0e0 100644
--- a/microdroid_manager/src/instance.rs
+++ b/microdroid_manager/src/instance.rs
@@ -273,8 +273,6 @@
#[derive(Debug, Serialize, Deserialize, PartialEq, Eq)]
pub struct MicrodroidData {
- // `salt` is obsolete, it was used as a differentiator for non-protected VM instances running
- // same payload. Instance-id (present in DT) is used for that now.
pub salt: Vec<u8>, // Should be [u8; 64] but that isn't serializable.
pub apk_data: ApkData,
pub extra_apks_data: Vec<ApkData>,
diff --git a/microdroid_manager/src/main.rs b/microdroid_manager/src/main.rs
index e8017e8..0d67632 100644
--- a/microdroid_manager/src/main.rs
+++ b/microdroid_manager/src/main.rs
@@ -42,7 +42,7 @@
use keystore2_crypto::ZVec;
use libc::VMADDR_CID_HOST;
use log::{error, info};
-use microdroid_metadata::{Metadata, PayloadMetadata};
+use microdroid_metadata::PayloadMetadata;
use microdroid_payload_config::{ApkConfig, OsConfig, Task, TaskType, VmPayloadConfig};
use nix::sys::signal::Signal;
use payload::load_metadata;
@@ -236,12 +236,16 @@
}
}
-fn verify_payload_with_instance_img(
- metadata: &Metadata,
- dice: &DiceDriver,
-) -> Result<MicrodroidData> {
+fn try_run_payload(
+ service: &Strong<dyn IVirtualMachineService>,
+ vm_payload_service_fd: OwnedFd,
+) -> Result<i32> {
+ let metadata = load_metadata().context("Failed to load payload metadata")?;
+ let dice = DiceDriver::new(Path::new("/dev/open-dice0")).context("Failed to load DICE")?;
+
let mut instance = InstanceDisk::new().context("Failed to load instance.img")?;
- let saved_data = instance.read_microdroid_data(dice).context("Failed to read identity data")?;
+ let saved_data =
+ instance.read_microdroid_data(&dice).context("Failed to read identity data")?;
if is_strict_boot() {
// Provisioning must happen on the first boot and never again.
@@ -261,7 +265,7 @@
}
// Verify the payload before using it.
- let extracted_data = verify_payload(metadata, saved_data.as_ref())
+ let extracted_data = verify_payload(&metadata, saved_data.as_ref())
.context("Payload verification failed")
.map_err(|e| MicrodroidError::PayloadVerificationFailed(e.to_string()))?;
@@ -285,28 +289,10 @@
} else {
info!("Saving verified data.");
instance
- .write_microdroid_data(&extracted_data, dice)
+ .write_microdroid_data(&extracted_data, &dice)
.context("Failed to write identity data")?;
extracted_data
};
- Ok(instance_data)
-}
-
-fn try_run_payload(
- service: &Strong<dyn IVirtualMachineService>,
- vm_payload_service_fd: OwnedFd,
-) -> Result<i32> {
- let metadata = load_metadata().context("Failed to load payload metadata")?;
- let dice = DiceDriver::new(Path::new("/dev/open-dice0")).context("Failed to load DICE")?;
-
- // TODO(b/291306122): Checking with host about Secretkeeper support multiple times introduces
- // a whole range of security vulnerability since host can give different answers. Guest should
- // check only once and the same answer should be known to pVM Firmware and Microdroid.
- let instance_data = if let Some(_sk) = vm_secret::is_sk_supported(service)? {
- verify_payload(&metadata, None)?
- } else {
- verify_payload_with_instance_img(&metadata, &dice)?
- };
let payload_metadata = metadata.payload.ok_or_else(|| {
MicrodroidError::PayloadInvalidConfig("No payload config in metadata".to_string())
diff --git a/microdroid_manager/src/verify.rs b/microdroid_manager/src/verify.rs
index 65c32b0..445c1ae 100644
--- a/microdroid_manager/src/verify.rs
+++ b/microdroid_manager/src/verify.rs
@@ -169,14 +169,13 @@
// verified is consistent with the root hash) or because we have the saved APK data which will
// be checked as identical to the data we have verified.
- let salt = if cfg!(llpvm_changes) || is_strict_boot() {
- // Salt is obsolete with llpvm_changes.
- vec![0u8; 64]
- } else if let Some(saved_data) = saved_data {
- // Use the salt from a verified instance.
+ // Use the salt from a verified instance, or generate a salt for a new instance.
+ let salt = if let Some(saved_data) = saved_data {
saved_data.salt.clone()
+ } else if is_strict_boot() {
+ // No need to add more entropy as a previous stage must have used a new, random salt.
+ vec![0u8; 64]
} else {
- // Generate a salt for a new instance.
let mut salt = vec![0u8; 64];
salt.as_mut_slice().try_fill(&mut rand::thread_rng())?;
salt
diff --git a/microdroid_manager/src/vm_secret.rs b/microdroid_manager/src/vm_secret.rs
index 7b65491..5ceedea 100644
--- a/microdroid_manager/src/vm_secret.rs
+++ b/microdroid_manager/src/vm_secret.rs
@@ -279,9 +279,9 @@
anyhow!("{:?}", err)
}
-/// Get the secretkeeper connection if supported. Host can be consulted whether the device supports
-/// secretkeeper but that should be used with caution for protected VM.
-pub fn is_sk_supported(
+// Get the secretkeeper connection if supported. Host can be consulted whether the device supports
+// secretkeeper but that should be used with caution for protected VM.
+fn is_sk_supported(
host: &Strong<dyn IVirtualMachineService>,
) -> Result<Option<Strong<dyn ISecretkeeper>>> {
let sk = if cfg!(llpvm_changes) {
diff --git a/pvmfw/src/device_assignment.rs b/pvmfw/src/device_assignment.rs
index 9c3e566..86ad0f0 100644
--- a/pvmfw/src/device_assignment.rs
+++ b/pvmfw/src/device_assignment.rs
@@ -73,6 +73,8 @@
DuplicatedIommuIds,
/// Duplicated pvIOMMU IDs exist
DuplicatedPvIommuIds,
+ /// Unsupported path format. Only supports full path.
+ UnsupportedPathFormat,
/// Unsupported overlay target syntax. Only supports <target-path> with full path.
UnsupportedOverlayTarget,
/// Unsupported PhysIommu,
@@ -120,6 +122,9 @@
Self::DuplicatedPvIommuIds => {
write!(f, "Duplicated pvIOMMU IDs exist. IDs must unique among iommu node")
}
+ Self::UnsupportedPathFormat => {
+ write!(f, "Unsupported UnsupportedPathFormat. Only supports full path")
+ }
Self::UnsupportedOverlayTarget => {
write!(f, "Unsupported overlay target. Only supports 'target-path = \"/\"'")
}
@@ -140,6 +145,67 @@
pub type Result<T> = core::result::Result<T, DeviceAssignmentError>;
+#[derive(Clone, Default, Ord, PartialOrd, Eq, PartialEq)]
+pub struct DtPathTokens<'a> {
+ tokens: Vec<&'a [u8]>,
+}
+
+impl<'a> fmt::Debug for DtPathTokens<'a> {
+ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+ let mut list = f.debug_list();
+ for token in &self.tokens {
+ let mut bytes = token.to_vec();
+ bytes.push(b'\0');
+ match CString::from_vec_with_nul(bytes) {
+ Ok(string) => list.entry(&string),
+ Err(_) => list.entry(token),
+ };
+ }
+ list.finish()
+ }
+}
+
+impl<'a> DtPathTokens<'a> {
+ fn new(path: &'a CStr) -> Result<Self> {
+ if path.to_bytes().first() != Some(&b'/') {
+ return Err(DeviceAssignmentError::UnsupportedPathFormat);
+ }
+ let tokens: Vec<_> = path
+ .to_bytes()
+ .split(|char| *char == b'/')
+ .filter(|&component| !component.is_empty())
+ .collect();
+ Ok(Self { tokens })
+ }
+
+ fn to_overlay_target_path(&self) -> Result<Self> {
+ if !self.is_overlayable_node() {
+ return Err(DeviceAssignmentError::InvalidDtbo);
+ }
+ Ok(Self { tokens: self.tokens.as_slice()[2..].to_vec() })
+ }
+
+ fn to_cstring(&self) -> CString {
+ if self.tokens.is_empty() {
+ return CString::new(*b"/\0").unwrap();
+ }
+
+ let size = self.tokens.iter().fold(0, |sum, token| sum + token.len() + 1);
+ let mut path = Vec::with_capacity(size + 1);
+ for token in &self.tokens {
+ path.push(b'/');
+ path.extend_from_slice(token);
+ }
+ path.push(b'\0');
+
+ CString::from_vec_with_nul(path).unwrap()
+ }
+
+ fn is_overlayable_node(&self) -> bool {
+ self.tokens.get(1) == Some(&&b"__overlay__"[..])
+ }
+}
+
/// Represents VM DTBO
#[repr(transparent)]
pub struct VmDtbo(Fdt);
@@ -179,14 +245,9 @@
// node and/or properties. The enforcement is for compatibility reason.
fn locate_overlay_target_path(
&self,
- dtbo_node_path: &CStr,
+ dtbo_node_path: &DtPathTokens,
dtbo_node: &FdtNode,
) -> Result<CString> {
- let dtbo_node_path_bytes = dtbo_node_path.to_bytes();
- if dtbo_node_path_bytes.first() != Some(&b'/') {
- return Err(DeviceAssignmentError::UnsupportedOverlayTarget);
- }
-
let fragment_node = dtbo_node.supernode_at_depth(1)?;
let target_path = fragment_node
.getprop_str(cstr!("target-path"))?
@@ -195,22 +256,8 @@
return Err(DeviceAssignmentError::UnsupportedOverlayTarget);
}
- let mut components = dtbo_node_path_bytes
- .split(|char| *char == b'/')
- .filter(|&component| !component.is_empty())
- .skip(1);
- let overlay_node_name = components.next();
- if overlay_node_name != Some(b"__overlay__") {
- return Err(DeviceAssignmentError::InvalidDtbo);
- }
- let mut overlaid_path = Vec::with_capacity(dtbo_node_path_bytes.len());
- for component in components {
- overlaid_path.push(b'/');
- overlaid_path.extend_from_slice(component);
- }
- overlaid_path.push(b'\0');
-
- Ok(CString::from_vec_with_nul(overlaid_path).unwrap())
+ let overlaid_path = dtbo_node_path.to_overlay_target_path()?;
+ Ok(overlaid_path.to_cstring())
}
fn parse_physical_iommus(physical_node: &FdtNode) -> Result<BTreeMap<Phandle, PhysIommu>> {
@@ -281,15 +328,17 @@
let phys_iommus = Self::parse_physical_iommus(&physical_node)?;
Self::parse_physical_devices_with_iommus(&physical_node, &phys_iommus)
}
-}
-fn is_overlayable_node(dtbo_path: &CStr) -> bool {
- dtbo_path
- .to_bytes()
- .split(|char| *char == b'/')
- .filter(|&component| !component.is_empty())
- .nth(1)
- .map_or(false, |name| name == b"__overlay__")
+ fn node(&self, path: &DtPathTokens) -> Result<Option<FdtNode>> {
+ let mut node = self.as_ref().root();
+ for token in &path.tokens {
+ let Some(subnode) = node.subnode_with_name_bytes(token)? else {
+ return Ok(None);
+ };
+ node = subnode;
+ }
+ Ok(Some(node))
+ }
}
fn filter_dangling_symbols(fdt: &mut Fdt) -> Result<()> {
@@ -458,8 +507,6 @@
struct AssignedDeviceInfo {
// Node path of assigned device (e.g. "/rng")
node_path: CString,
- // DTBO node path of the assigned device (e.g. "/fragment@rng/__overlay__/rng")
- dtbo_node_path: CString,
// <reg> property from the crosvm DT
reg: Vec<DeviceReg>,
// <interrupts> property from the crosvm DT
@@ -568,13 +615,13 @@
fn parse(
fdt: &Fdt,
vm_dtbo: &VmDtbo,
- dtbo_node_path: &CStr,
+ dtbo_node_path: &DtPathTokens,
physical_devices: &BTreeMap<Phandle, PhysicalDeviceInfo>,
pviommus: &BTreeMap<Phandle, PvIommu>,
hypervisor: &dyn DeviceAssigningHypervisor,
) -> Result<Option<Self>> {
let dtbo_node =
- vm_dtbo.as_ref().node(dtbo_node_path)?.ok_or(DeviceAssignmentError::InvalidSymbols)?;
+ vm_dtbo.node(dtbo_node_path)?.ok_or(DeviceAssignmentError::InvalidSymbols)?;
let node_path = vm_dtbo.locate_overlay_target_path(dtbo_node_path, &dtbo_node)?;
let Some(node) = fdt.node(&node_path)? else { return Ok(None) };
@@ -595,7 +642,7 @@
let iommus = Self::parse_iommus(&node, pviommus)?;
Self::validate_iommus(&iommus, &physical_device.iommus, hypervisor)?;
- Ok(Some(Self { node_path, dtbo_node_path: dtbo_node_path.into(), reg, interrupts, iommus }))
+ Ok(Some(Self { node_path, reg, interrupts, iommus }))
}
fn patch(&self, fdt: &mut Fdt, pviommu_phandles: &BTreeMap<PvIommu, Phandle>) -> Result<()> {
@@ -681,13 +728,14 @@
let symbol_prop_value = symbol_prop.value()?;
let dtbo_node_path = CStr::from_bytes_with_nul(symbol_prop_value)
.or(Err(DeviceAssignmentError::InvalidSymbols))?;
- if !is_overlayable_node(dtbo_node_path) {
+ let dtbo_node_path = DtPathTokens::new(dtbo_node_path)?;
+ if !dtbo_node_path.is_overlayable_node() {
continue;
}
let assigned_device = AssignedDeviceInfo::parse(
fdt,
vm_dtbo,
- dtbo_node_path,
+ &dtbo_node_path,
&physical_devices,
&pviommus,
hypervisor,
@@ -695,7 +743,7 @@
if let Some(assigned_device) = assigned_device {
assigned_devices.push(assigned_device);
} else {
- filtered_dtbo_paths.push(dtbo_node_path.into());
+ filtered_dtbo_paths.push(dtbo_node_path.to_cstring());
}
}
if assigned_devices.is_empty() {
@@ -922,7 +970,6 @@
let expected = [AssignedDeviceInfo {
node_path: CString::new("/bus0/backlight").unwrap(),
- dtbo_node_path: cstr!("/fragment@0/__overlay__/bus0/backlight").into(),
reg: vec![[0x9, 0xFF].into()],
interrupts: into_fdt_prop(vec![0x0, 0xF, 0x4]),
iommus: vec![],
@@ -946,7 +993,6 @@
let expected = [AssignedDeviceInfo {
node_path: CString::new("/rng").unwrap(),
- dtbo_node_path: cstr!("/fragment@0/__overlay__/rng").into(),
reg: vec![[0x9, 0xFF].into()],
interrupts: into_fdt_prop(vec![0x0, 0xF, 0x4]),
iommus: vec![(PvIommu { id: 0x4 }, Vsid(0xFF0))],
diff --git a/tests/hostside/java/com/android/microdroid/test/MicrodroidHostTests.java b/tests/hostside/java/com/android/microdroid/test/MicrodroidHostTests.java
index 4f502ab..6dd3afe 100644
--- a/tests/hostside/java/com/android/microdroid/test/MicrodroidHostTests.java
+++ b/tests/hostside/java/com/android/microdroid/test/MicrodroidHostTests.java
@@ -793,12 +793,11 @@
assertWithMessage("Incorrect ABI list").that(abis).hasLength(1);
// Check that no denials have happened so far
+ String logText =
+ getDevice().pullFileContents(CONSOLE_PATH) + getDevice().pullFileContents(LOG_PATH);
assertWithMessage("Unexpected denials during VM boot")
- .that(android.tryRun("egrep", "'avc:[[:space:]]{1,2}denied'", LOG_PATH))
- .isNull();
- assertWithMessage("Unexpected denials during VM boot")
- .that(android.tryRun("egrep", "'avc:[[:space:]]{1,2}denied'", CONSOLE_PATH))
- .isNull();
+ .that(logText)
+ .doesNotContainMatch("avc:\s+denied");
assertThat(getDeviceNumCpus(microdroid)).isEqualTo(getDeviceNumCpus(android));
diff --git a/virtualizationmanager/src/aidl.rs b/virtualizationmanager/src/aidl.rs
index ea3a481..278365c 100644
--- a/virtualizationmanager/src/aidl.rs
+++ b/virtualizationmanager/src/aidl.rs
@@ -49,7 +49,7 @@
use android_system_virtualmachineservice::aidl::android::system::virtualmachineservice::IVirtualMachineService::{
BnVirtualMachineService, IVirtualMachineService,
};
-use android_hardware_security_secretkeeper::aidl::android::hardware::security::secretkeeper::ISecretkeeper::{BnSecretkeeper, ISecretkeeper};
+use android_hardware_security_secretkeeper::aidl::android::hardware::security::secretkeeper::ISecretkeeper::ISecretkeeper;
use android_hardware_security_secretkeeper::aidl::android::hardware::security::secretkeeper::SecretId::SecretId;
use android_hardware_security_authgraph::aidl::android::hardware::security::authgraph::{
Arc::Arc as AuthgraphArc, IAuthGraphKeyExchange::IAuthGraphKeyExchange,
@@ -1506,12 +1506,10 @@
}
fn getSecretkeeper(&self) -> binder::Result<Option<Strong<dyn ISecretkeeper>>> {
- let sk = if is_secretkeeper_supported() {
- Some(binder::wait_for_interface(SECRETKEEPER_IDENTIFIER)?)
- } else {
- None
- };
- Ok(sk.map(|s| BnSecretkeeper::new_binder(SecretkeeperProxy(s), BinderFeatures::default())))
+ // TODO(b/327526008): Session establishment wth secretkeeper is failing.
+ // Re-enable this when fixed.
+ let _sk_supported = is_secretkeeper_supported();
+ Ok(None)
}
fn requestAttestation(&self, csr: &[u8], test_mode: bool) -> binder::Result<Vec<Certificate>> {