Merge "Add Microdroid GKI boot time benchmarks" into main
diff --git a/libs/cborutil/src/lib.rs b/libs/cborutil/src/lib.rs
index 6e834f1..4d308c1 100644
--- a/libs/cborutil/src/lib.rs
+++ b/libs/cborutil/src/lib.rs
@@ -21,7 +21,7 @@
use alloc::string::String;
use alloc::vec::Vec;
use ciborium::value::{Integer, Value};
-use coset::{CoseError, CoseKey, Label, Result};
+use coset::{CborSerializable, CoseError, CoseKey, Label, Result};
use log::error;
use serde::{de::DeserializeOwned, Serialize};
@@ -43,6 +43,11 @@
}
}
+/// Parses the given CBOR-encoded byte slice as a value array.
+pub fn parse_value_array(data: &[u8], context: &'static str) -> Result<Vec<Value>> {
+ value_to_array(Value::from_slice(data)?, context)
+}
+
/// Converts the provided value `v` to a value array.
pub fn value_to_array(v: Value, context: &'static str) -> Result<Vec<Value>> {
v.into_array().map_err(|e| to_unexpected_item_error(&e, "array", context))
diff --git a/microdroid/Android.bp b/microdroid/Android.bp
index f98f3af..ae89f79 100644
--- a/microdroid/Android.bp
+++ b/microdroid/Android.bp
@@ -394,16 +394,6 @@
partitions: ["microdroid_vendor"],
},
},
- // TODO(b/312809093): Remove hard-coded property after figuring out the
- // long-term solution for microdroid vendor partition SPL. The hard-coded
- // value is the minimum value of SPL that microdroid vendor partition will
- // have. It's for passing the check 'IsStandaloneImageRollback'.
- avb_properties: [
- {
- key: "com.android.build.microdroid-vendor.security_patch",
- value: "2023-12-05",
- },
- ],
}
prebuilt_etc {
diff --git a/pvmfw/src/config.rs b/pvmfw/src/config.rs
index 3f78a88..39b7421 100644
--- a/pvmfw/src/config.rs
+++ b/pvmfw/src/config.rs
@@ -84,6 +84,7 @@
const MAGIC: u32 = u32::from_ne_bytes(*b"pvmf");
const VERSION_1_0: Version = Version { major: 1, minor: 0 };
const VERSION_1_1: Version = Version { major: 1, minor: 1 };
+ const VERSION_1_2: Version = Version { major: 1, minor: 2 };
pub fn total_size(&self) -> usize {
self.total_size as usize
@@ -105,8 +106,9 @@
let last_entry = match self.version {
Self::VERSION_1_0 => Entry::DebugPolicy,
Self::VERSION_1_1 => Entry::VmDtbo,
+ Self::VERSION_1_2 => Entry::VmBaseDtbo,
v @ Version { major: 1, .. } => {
- const LATEST: Version = Header::VERSION_1_1;
+ const LATEST: Version = Header::VERSION_1_2;
warn!("Parsing unknown config data version {v} as version {LATEST}");
return Ok(Entry::COUNT);
}
@@ -122,6 +124,7 @@
Bcc,
DebugPolicy,
VmDtbo,
+ VmBaseDtbo,
#[allow(non_camel_case_types)] // TODO: Use mem::variant_count once stable.
_VARIANT_COUNT,
}
@@ -129,7 +132,8 @@
impl Entry {
const COUNT: usize = Self::_VARIANT_COUNT as usize;
- const ALL_ENTRIES: [Entry; Self::COUNT] = [Self::Bcc, Self::DebugPolicy, Self::VmDtbo];
+ const ALL_ENTRIES: [Entry; Self::COUNT] =
+ [Self::Bcc, Self::DebugPolicy, Self::VmDtbo, Self::VmBaseDtbo];
}
#[derive(Default)]
@@ -137,6 +141,7 @@
pub bcc: &'a mut [u8],
pub debug_policy: Option<&'a [u8]>,
pub vm_dtbo: Option<&'a mut [u8]>,
+ pub vm_base_dtbo: Option<&'a mut [u8]>,
}
#[repr(packed)]
@@ -285,13 +290,13 @@
entries[i] = Some(chunk);
}
}
- let [bcc, debug_policy, vm_dtbo] = entries;
+ let [bcc, debug_policy, vm_dtbo, vm_base_dtbo] = entries;
// The platform BCC has always been required.
let bcc = bcc.unwrap();
// We have no reason to mutate so drop the `mut`.
let debug_policy = debug_policy.map(|x| &*x);
- Entries { bcc, debug_policy, vm_dtbo }
+ Entries { bcc, debug_policy, vm_dtbo, vm_base_dtbo }
}
}
diff --git a/pvmfw/src/entry.rs b/pvmfw/src/entry.rs
index 8c4396d..c740d1b 100644
--- a/pvmfw/src/entry.rs
+++ b/pvmfw/src/entry.rs
@@ -88,6 +88,7 @@
kernel: usize,
kernel_size: usize,
vm_dtbo: Option<&mut [u8]>,
+ vm_base_dtbo: Option<&mut [u8]>,
) -> Result<Self, RebootReason> {
let fdt_size = NonZeroUsize::new(crosvm::FDT_MAX_SIZE).unwrap();
// TODO - Only map the FDT as read-only, until we modify it right before jump_to_payload()
@@ -101,7 +102,7 @@
// SAFETY: The tracker validated the range to be in main memory, mapped, and not overlap.
let fdt = unsafe { slice::from_raw_parts_mut(range.start as *mut u8, range.len()) };
- let info = fdt::sanitize_device_tree(fdt, vm_dtbo)?;
+ let info = fdt::sanitize_device_tree(fdt, vm_dtbo, vm_base_dtbo)?;
let fdt = libfdt::Fdt::from_mut_slice(fdt).map_err(|e| {
error!("Failed to load sanitized FDT: {e}");
RebootReason::InvalidFdt
@@ -227,7 +228,13 @@
Some(memory::appended_payload_range()),
));
- let slices = MemorySlices::new(fdt, payload, payload_size, config_entries.vm_dtbo)?;
+ let slices = MemorySlices::new(
+ fdt,
+ payload,
+ payload_size,
+ config_entries.vm_dtbo,
+ config_entries.vm_base_dtbo,
+ )?;
// This wrapper allows main() to be blissfully ignorant of platform details.
let next_bcc = crate::main(
diff --git a/pvmfw/src/fdt.rs b/pvmfw/src/fdt.rs
index b53e452..44e55dd 100644
--- a/pvmfw/src/fdt.rs
+++ b/pvmfw/src/fdt.rs
@@ -19,6 +19,7 @@
use crate::helpers::GUEST_PAGE_SIZE;
use crate::Box;
use crate::RebootReason;
+use alloc::collections::BTreeMap;
use alloc::ffi::CString;
use alloc::vec::Vec;
use core::cmp::max;
@@ -200,27 +201,47 @@
Ok(())
}
-fn read_vendor_hashtree_descriptor_root_digest_from(fdt: &Fdt) -> libfdt::Result<Option<Vec<u8>>> {
+/// Read candidate properties' names from DT which could be overlaid
+fn parse_vm_base_dtbo(fdt: &Fdt) -> libfdt::Result<BTreeMap<CString, Vec<u8>>> {
+ let mut property_map = BTreeMap::new();
if let Some(avf_node) = fdt.node(cstr!("/avf"))? {
- if let Some(vendor_hashtree_descriptor_root_digest) =
- avf_node.getprop(cstr!("vendor_hashtree_descriptor_root_digest"))?
- {
- return Ok(Some(vendor_hashtree_descriptor_root_digest.to_vec()));
+ for property in avf_node.properties()? {
+ let name = property.name()?;
+ let value = property.value()?;
+ property_map.insert(
+ CString::new(name.to_bytes()).map_err(|_| FdtError::BadValue)?,
+ value.to_vec(),
+ );
}
}
- Ok(None)
+ Ok(property_map)
}
-fn patch_vendor_hashtree_descriptor_root_digest(
- fdt: &mut Fdt,
- vendor_hashtree_descriptor_root_digest: &[u8],
+/// Overlay VM base DTBO into VM DT based on the props_info. Property is overlaid in vm_dt only
+/// when it exists both in vm_base_dtbo and props_info. If the values mismatch, it returns error.
+fn apply_vm_base_dtbo(
+ vm_dt: &mut Fdt,
+ vm_base_dtbo: &Fdt,
+ props_info: &BTreeMap<CString, Vec<u8>>,
) -> libfdt::Result<()> {
- let mut root_node = fdt.root_mut()?;
- let mut avf_node = root_node.add_subnode(cstr!("/avf"))?;
- avf_node.setprop(
- cstr!("vendor_hashtree_descriptor_root_digest"),
- vendor_hashtree_descriptor_root_digest,
- )?;
+ let mut root_vm_dt = vm_dt.root_mut()?;
+ let mut avf_vm_dt = root_vm_dt.add_subnode(cstr!("avf"))?;
+ // TODO(b/318431677): Validate nodes beyond /fragment@0/__overlay__/avf and use apply_overlay.
+ let avf_vm_base_dtbo =
+ vm_base_dtbo.node(cstr!("/fragment@0/__overlay__/avf"))?.ok_or(FdtError::NotFound)?;
+ for (name, value) in props_info.iter() {
+ if let Some(value_in_vm_base_dtbo) = avf_vm_base_dtbo.getprop(name)? {
+ if value != value_in_vm_base_dtbo {
+ error!(
+ "Property mismatches while applying overlay VM base DTBO. \
+ Name:{:?}, Value from host as hex:{:x?}, Value from VM base DTBO as hex:{:x?}",
+ name, value, value_in_vm_base_dtbo
+ );
+ return Err(FdtError::BadValue);
+ }
+ avf_vm_dt.setprop(name, value_in_vm_base_dtbo)?;
+ }
+ }
Ok(())
}
@@ -616,7 +637,7 @@
serial_info: SerialInfo,
pub swiotlb_info: SwiotlbInfo,
device_assignment: Option<DeviceAssignmentInfo>,
- vendor_hashtree_descriptor_root_digest: Option<Vec<u8>>,
+ vm_base_dtbo_props_info: BTreeMap<CString, Vec<u8>>,
}
impl DeviceTreeInfo {
@@ -630,6 +651,7 @@
pub fn sanitize_device_tree(
fdt: &mut [u8],
vm_dtbo: Option<&mut [u8]>,
+ vm_base_dtbo: Option<&mut [u8]>,
) -> Result<DeviceTreeInfo, RebootReason> {
let fdt = Fdt::from_mut_slice(fdt).map_err(|e| {
error!("Failed to load FDT: {e}");
@@ -673,6 +695,18 @@
}
}
+ if let Some(vm_base_dtbo) = vm_base_dtbo {
+ let vm_base_dtbo = Fdt::from_mut_slice(vm_base_dtbo).map_err(|e| {
+ error!("Failed to load VM base DTBO: {e}");
+ RebootReason::InvalidFdt
+ })?;
+
+ apply_vm_base_dtbo(fdt, vm_base_dtbo, &info.vm_base_dtbo_props_info).map_err(|e| {
+ error!("Failed to apply VM base DTBO: {e}");
+ RebootReason::InvalidFdt
+ })?;
+ }
+
patch_device_tree(fdt, &info)?;
// TODO(b/317201360): Ensure no overlapping in <reg> among devices
@@ -746,19 +780,10 @@
None => None,
};
- // TODO(b/285854379) : A temporary solution lives. This is for enabling
- // microdroid vendor partition for non-protected VM as well. When passing
- // DT path containing vendor_hashtree_descriptor_root_digest via fstab, init
- // stage will check if vendor_hashtree_descriptor_root_digest exists in the
- // init stage, regardless the protection. Adding this temporary solution
- // will prevent fatal in init stage for protected VM. However, this data is
- // not trustable without validating root digest of vendor hashtree
- // descriptor comes from ABL.
- let vendor_hashtree_descriptor_root_digest =
- read_vendor_hashtree_descriptor_root_digest_from(fdt).map_err(|e| {
- error!("Failed to read vendor_hashtree_descriptor_root_digest from DT: {e}");
- RebootReason::InvalidFdt
- })?;
+ let vm_base_dtbo_props_info = parse_vm_base_dtbo(fdt).map_err(|e| {
+ error!("Failed to read names of properties under /avf from DT: {e}");
+ RebootReason::InvalidFdt
+ })?;
Ok(DeviceTreeInfo {
kernel_range,
@@ -770,7 +795,7 @@
serial_info,
swiotlb_info,
device_assignment,
- vendor_hashtree_descriptor_root_digest,
+ vm_base_dtbo_props_info,
})
}
@@ -823,15 +848,6 @@
RebootReason::InvalidFdt
})?;
}
- if let Some(vendor_hashtree_descriptor_root_digest) =
- &info.vendor_hashtree_descriptor_root_digest
- {
- patch_vendor_hashtree_descriptor_root_digest(fdt, vendor_hashtree_descriptor_root_digest)
- .map_err(|e| {
- error!("Failed to patch vendor_hashtree_descriptor_root_digest to DT: {e}");
- RebootReason::InvalidFdt
- })?;
- }
Ok(())
}
diff --git a/service_vm/requests/src/client_vm.rs b/service_vm/requests/src/client_vm.rs
index 6ebed50..d4474cf 100644
--- a/service_vm/requests/src/client_vm.rs
+++ b/service_vm/requests/src/client_vm.rs
@@ -22,7 +22,7 @@
use crate::keyblob::decrypt_private_key;
use alloc::vec::Vec;
use bssl_avf::{rand_bytes, sha256, Digester, EcKey, PKey};
-use cbor_util::value_to_array;
+use cbor_util::parse_value_array;
use ciborium::value::Value;
use core::result;
use coset::{AsCborValue, CborSerializable, CoseSign, CoseSign1};
@@ -53,10 +53,8 @@
// Validates the prefix of the Client VM DICE chain in the CSR.
let service_vm_dice_chain =
dice_artifacts.bcc().ok_or(RequestProcessingError::MissingDiceChain)?;
- let service_vm_dice_chain =
- value_to_array(Value::from_slice(service_vm_dice_chain)?, "service_vm_dice_chain")?;
- let client_vm_dice_chain =
- value_to_array(Value::from_slice(&csr.dice_cert_chain)?, "client_vm_dice_chain")?;
+ let service_vm_dice_chain = parse_value_array(service_vm_dice_chain, "service_vm_dice_chain")?;
+ let client_vm_dice_chain = parse_value_array(&csr.dice_cert_chain, "client_vm_dice_chain")?;
validate_client_vm_dice_chain_prefix_match(&client_vm_dice_chain, &service_vm_dice_chain)?;
// Validates the signatures in the Client VM DICE chain and extracts the partially decoded
// DiceChainEntryPayloads.
diff --git a/tests/hostside/java/com/android/microdroid/test/MicrodroidHostTests.java b/tests/hostside/java/com/android/microdroid/test/MicrodroidHostTests.java
index a54a22a..1fa0976 100644
--- a/tests/hostside/java/com/android/microdroid/test/MicrodroidHostTests.java
+++ b/tests/hostside/java/com/android/microdroid/test/MicrodroidHostTests.java
@@ -435,7 +435,7 @@
@Test
@CddTest(requirements = {"9.17/C-2-1", "9.17/C-2-2", "9.17/C-2-6"})
- public void protectedVmWithImageSignedWithDifferentKeyRunsPvmfw() throws Exception {
+ public void protectedVmWithImageSignedWithDifferentKeyFailsToVerifyPayload() throws Exception {
// Arrange
assumeProtectedVm();
File key = findTestFile("test.com.android.virt.pem");
@@ -452,8 +452,9 @@
vmInfo.mProcess.waitFor(5L, TimeUnit.SECONDS);
String consoleLog = getDevice().pullFileContents(CONSOLE_PATH);
assertWithMessage("pvmfw should start").that(consoleLog).contains("pVM firmware");
- // TODO(b/256148034): Asserts that pvmfw run fails when this verification is implemented.
- // Also rename the test.
+ assertWithMessage("pvmfw should fail to verify the payload")
+ .that(consoleLog)
+ .contains("Failed to verify the payload");
vmInfo.mProcess.destroy();
}
diff --git a/virtualizationmanager/src/aidl.rs b/virtualizationmanager/src/aidl.rs
index d775555..12b8f88 100644
--- a/virtualizationmanager/src/aidl.rs
+++ b/virtualizationmanager/src/aidl.rs
@@ -1392,17 +1392,12 @@
}
fn getSecretkeeper(&self) -> binder::Result<Option<Strong<dyn ISecretkeeper>>> {
- let sk = match binder::get_interface(SECRETKEEPER_IDENTIFIER) {
- Ok(sk) => {
- Some(BnSecretkeeper::new_binder(SecretkeeperProxy(sk), BinderFeatures::default()))
- }
- Err(StatusCode::NAME_NOT_FOUND) => None,
- Err(e) => {
- error!("unexpected error while fetching connection to Secretkeeper {:?}", e);
- return Err(e.into());
- }
+ let sk = if is_secretkeeper_present() {
+ Some(binder::wait_for_interface(SECRETKEEPER_IDENTIFIER)?)
+ } else {
+ None
};
- Ok(sk)
+ Ok(sk.map(|s| BnSecretkeeper::new_binder(SecretkeeperProxy(s), BinderFeatures::default())))
}
fn requestAttestation(&self, csr: &[u8]) -> binder::Result<Vec<Certificate>> {
@@ -1410,6 +1405,11 @@
}
}
+fn is_secretkeeper_present() -> bool {
+ binder::is_declared(SECRETKEEPER_IDENTIFIER)
+ .expect("Could not check for declared Secretkeeper interface")
+}
+
impl VirtualMachineService {
fn new_binder(state: Arc<Mutex<State>>, cid: Cid) -> Strong<dyn IVirtualMachineService> {
BnVirtualMachineService::new_binder(