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(